Bug 135448 – URLs in srcset attributes are not made absolute upon copy and paste

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

URLs in srcset attributes are not made absolute upon copy and paste

[Myles C. Maxfield]

Created attachment 235796 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/17769264>

[Ryosuke Niwa]

Comment on attachment 235796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review

> Source/WebCore/html/HTMLImageElement.cpp:369
> +                result.append(String::format(" %fx", candidate.density));

It looks more efficient to call append(' '), appendNumber(candidate.density), and then append('x').

> Source/WebCore/html/HTMLImageElement.cpp:371
> +                result.append(String::format(" %dw", candidate.resourceWidth));

Ditto.

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:8
> +TEST COMPLETE

If we really need to wait until the load event, you need to set jsTestIsAsync? or whatever flag we have to true
so that this line appears at the end of the test.

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:9
> +PASS valueBeforeCopy is not image.getAttribute('srcset')

Instead of asserting that values are different, we should make sure the values are what we expect.
e.g. you can use location.href to get the base URL and then manually compute the value in JS.

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:4
> +<script src="../../resources/js-test-pre.js"></script>

We don't need to put this in a head element.

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:28
> +<iframe src="resources/img-srcset-copy-paste-canonicalization-iframe.html"></iframe>

Why do we need to have this in an iframe?  I don't see why this would bug only reproduces inside an iframe.

[Ryosuke Niwa]

Comment on attachment 235796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:20
> +        destination.focus();
> +        selectAllCommand();
> +        pasteCommand();

If you're pasting the image in a different document, an interesting thing to check will be whether load or error events fire on the pasted img element.

[Build Bot]

Comment on attachment 235796 [details]
Patch

Attachment 235796 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6535841962786816

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

Created attachment 235804 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 235796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review

review- because MarkupAccumulator::appendAttribute won’t work

> Source/WebCore/dom/Element.h:396
> +    virtual String completeUrlAttributeValue(const URL& base, const Attribute&) const;

The URL acronym should not be spelled “Url”.

> Source/WebCore/html/HTMLImageElement.cpp:351
> +        || attribute.name() == srcsetAttr

Changing this to return true will have effects in others places, not just in the completeURLs function that is updated in this patch.

We will also have to figure out what to do inside MarkupAccumulator::appendAttribute, which also calls isURLAttribute, and make sure we have a test case that covers that path.

Also, the function isJavaScriptURLAttribute calls isURLAttribute, and then assumes it can process the attribute as a URL, which would be wrong for srcset. Maybe we’ll get lucky and it will return false, but I think it’s sloppy to let it try to look for a protocol in a srcset. Especially since the reason this function exists is for security purposes.

It’s also an error to call getURLAttribute or getNonEmptyURLAttribute on srcset, but the assertions in those functions won’t fire any more because isURLAttribute will return true. OK, but not great.

> Source/WebCore/html/HTMLImageElement.cpp:362
> +        parseImageCandidatesFromSrcsetAttribute(StringView(attribute.value()), imageCandidates);

Why do we have to specify StringView() explicitly?

> Source/WebCore/html/HTMLImageElement.cpp:366
> +                result.append(", ");

Should be appendLiteral.

>> Source/WebCore/html/HTMLImageElement.cpp:369
>> +                result.append(String::format(" %fx", candidate.density));
> 
> It looks more efficient to call append(' '), appendNumber(candidate.density), and then append('x').

We should also double check on number formatting. I’m pretty sure that %f puts in lots of extra zeros after the decimal point. I think appendNumber does what you’d want.

> Source/WebCore/html/parser/HTMLSrcsetParser.h:107
> +void parseImageCandidatesFromSrcsetAttribute(StringView attribute, Vector<ImageCandidate>& result);

Should probably change this to return the vector as a return value. No reason we need to use an out argument for this.

[Myles C. Maxfield]

Comment on attachment 235796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review

>> Source/WebCore/dom/Element.h:396
>> +    virtual String completeUrlAttributeValue(const URL& base, const Attribute&) const;
> 
> The URL acronym should not be spelled “Url”.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:351
>> +        || attribute.name() == srcsetAttr
> 
> Changing this to return true will have effects in others places, not just in the completeURLs function that is updated in this patch.
> 
> We will also have to figure out what to do inside MarkupAccumulator::appendAttribute, which also calls isURLAttribute, and make sure we have a test case that covers that path.
> 
> Also, the function isJavaScriptURLAttribute calls isURLAttribute, and then assumes it can process the attribute as a URL, which would be wrong for srcset. Maybe we’ll get lucky and it will return false, but I think it’s sloppy to let it try to look for a protocol in a srcset. Especially since the reason this function exists is for security purposes.
> 
> It’s also an error to call getURLAttribute or getNonEmptyURLAttribute on srcset, but the assertions in those functions won’t fire any more because isURLAttribute will return true. OK, but not great.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:362
>> +        parseImageCandidatesFromSrcsetAttribute(StringView(attribute.value()), imageCandidates);
> 
> Why do we have to specify StringView() explicitly?

Because value() returns an AtomicString, not a String. I tried adding another constructor to StringView that takes an AtomicString, but lots of places in WebCore were getting compiler errors about ambiguous function calls (since other functions had overrides which took StringViews and AtomicStrings, it didn't know which override to choose). I didn't want to fix each call site since that would be outside the scope of this patch.

>> Source/WebCore/html/HTMLImageElement.cpp:366
>> +                result.append(", ");
> 
> Should be appendLiteral.

Done.

>>> Source/WebCore/html/HTMLImageElement.cpp:369
>>> +                result.append(String::format(" %fx", candidate.density));
>> 
>> It looks more efficient to call append(' '), appendNumber(candidate.density), and then append('x').
> 
> We should also double check on number formatting. I’m pretty sure that %f puts in lots of extra zeros after the decimal point. I think appendNumber does what you’d want.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:371
>> +                result.append(String::format(" %dw", candidate.resourceWidth));
> 
> Ditto.

Done.

>> Source/WebCore/html/parser/HTMLSrcsetParser.h:107
>> +void parseImageCandidatesFromSrcsetAttribute(StringView attribute, Vector<ImageCandidate>& result);
> 
> Should probably change this to return the vector as a return value. No reason we need to use an out argument for this.

Done.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:8
>> +TEST COMPLETE
> 
> If we really need to wait until the load event, you need to set jsTestIsAsync? or whatever flag we have to true
> so that this line appears at the end of the test.

Done.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:9
>> +PASS valueBeforeCopy is not image.getAttribute('srcset')
> 
> Instead of asserting that values are different, we should make sure the values are what we expect.
> e.g. you can use location.href to get the base URL and then manually compute the value in JS.

Done.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:4
>> +<script src="../../resources/js-test-pre.js"></script>
> 
> We don't need to put this in a head element.

Done.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:20
>> +        pasteCommand();
> 
> If you're pasting the image in a different document, an interesting thing to check will be whether load or error events fire on the pasted img element.

Done.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization.html:28
>> +<iframe src="resources/img-srcset-copy-paste-canonicalization-iframe.html"></iframe>
> 
> Why do we need to have this in an iframe?  I don't see why this would bug only reproduces inside an iframe.

See the ChangeLog

[Myles C. Maxfield]

Created attachment 235834 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 235834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235834&action=review

> Source/WebCore/html/HTMLImageElement.cpp:373
> +                result.appendLiteral(" ");

Use append(' ') instead. appendLiteral is only useful when you're appending multiple characters.

> Source/WebCore/html/HTMLImageElement.cpp:375
> +                result.appendLiteral("x");

Ditto.

> Source/WebCore/html/HTMLImageElement.cpp:378
> +                result.appendLiteral(" ");

Ditto.

> Source/WebCore/html/HTMLImageElement.cpp:380
> +                result.appendLiteral("x");

Ditto.

> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:167
> +    Vector<ImageCandidate> result;

I would prefer continuing to call this variable imageCandidates to be self-explanatory.

> LayoutTests/ChangeLog:17
> +        * editing/pasteboard/resources/img-srcset-copy-paste-canonicalization-iframe.html:
> +        This has to be an iframe because we don't perform any url canonicalization if we
> +        are copying and pasting from a document into itself.

We should be able to copy from the main document and then paste into a newly created iframe.

> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:1
> +The following test does a copy and a paste of an image with the srcset attribute. The test is successful if the value of the srcset attribute has been canonicalized.

We should add an instruction on how to run this test inside a browser.
This is also why reverting the relationship between the document and the iframe helps because
then you can say, "copy the image below and paste into an iframe".

Note that we don't need a separate HTML file for the iframe since you can simply turn on designMode by document.designMode = 'on'.

[Ryosuke Niwa]

Comment on attachment 235834 [details]
Patch

r=me assuming all of my comments above are addressed.

[Myles C. Maxfield]

Comment on attachment 235834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235834&action=review

>> Source/WebCore/html/HTMLImageElement.cpp:373
>> +                result.appendLiteral(" ");
> 
> Use append(' ') instead. appendLiteral is only useful when you're appending multiple characters.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:375
>> +                result.appendLiteral("x");
> 
> Ditto.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:378
>> +                result.appendLiteral(" ");
> 
> Ditto.

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:380
>> +                result.appendLiteral("x");
> 
> Ditto.

Done.

>> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:167
>> +    Vector<ImageCandidate> result;
> 
> I would prefer continuing to call this variable imageCandidates to be self-explanatory.

Done.

>> LayoutTests/ChangeLog:17
>> +        are copying and pasting from a document into itself.
> 
> We should be able to copy from the main document and then paste into a newly created iframe.

See below.

>> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:1
>> +The following test does a copy and a paste of an image with the srcset attribute. The test is successful if the value of the srcset attribute has been canonicalized.
> 
> We should add an instruction on how to run this test inside a browser.
> This is also why reverting the relationship between the document and the iframe helps because
> then you can say, "copy the image below and paste into an iframe".
> 
> Note that we don't need a separate HTML file for the iframe since you can simply turn on designMode by document.designMode = 'on'.

This is bad for two reasons:
1) The iframe's window's href will be undefined, meaning I can't compute what the canonicalized img src should be in JavaScript.
2) This means that paths relative to the iframe will match paths relative to the main document, which means that the test will erroneously pass if you run the test manually on a build without this patch applied. (Meaning: the image will load successfully, by coincidence, even though its src hasn't been canonicalized)
I'm going to keep the iframe source in resources/ for these two reasons.

[Myles C. Maxfield]

https://trac.webkit.org/r171941

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 235834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235834&action=review
>
> >> LayoutTests/editing/pasteboard/img-srcset-copy-paste-canonicalization-expected.txt:1
> >> +The following test does a copy and a paste of an image with the srcset attribute. The test is successful if the value of the srcset attribute has been canonicalized.
> > 
> > We should add an instruction on how to run this test inside a browser.
> > This is also why reverting the relationship between the document and the iframe helps because
> > then you can say, "copy the image below and paste into an iframe".
> > 
> > Note that we don't need a separate HTML file for the iframe since you can simply turn on designMode by document.designMode = 'on'.
> 
> This is bad for two reasons:
> 1) The iframe's window's href will be undefined, meaning I can't compute what the canonicalized img src should be in JavaScript.
> 2) This means that paths relative to the iframe will match paths relative to the main document, which means that the test will erroneously pass if you run the test manually on a build without this patch applied. (Meaning: the image will load successfully, by coincidence, even though its src hasn't been canonicalized)
> I'm going to keep the iframe source in resources/ for these two reasons.

All we have to do is to use data URL or set base URL by inserting a meta element.

[Darin Adler]

Comment on attachment 235796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review

>>> Source/WebCore/html/HTMLImageElement.cpp:362
>>> +        parseImageCandidatesFromSrcsetAttribute(StringView(attribute.value()), imageCandidates);
>> 
>> Why do we have to specify StringView() explicitly?
> 
> Because value() returns an AtomicString, not a String. I tried adding another constructor to StringView that takes an AtomicString, but lots of places in WebCore were getting compiler errors about ambiguous function calls (since other functions had overrides which took StringViews and AtomicStrings, it didn't know which override to choose). I didn't want to fix each call site since that would be outside the scope of this patch.

At Anders’s suggestion, in one of my future patches, I add view() functions to both String and AtomicString that return a StringView. Once those are added, this code should be attribute.value().view(). I think it’s better than the construction syntax, since that can mean many different things.

[Myles C. Maxfield]

(In reply to comment #15)
> (From update of attachment 235796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235796&action=review
> 
> >>> Source/WebCore/html/HTMLImageElement.cpp:362
> >>> +        parseImageCandidatesFromSrcsetAttribute(StringView(attribute.value()), imageCandidates);
> >> 
> >> Why do we have to specify StringView() explicitly?
> > 
> > Because value() returns an AtomicString, not a String. I tried adding another constructor to StringView that takes an AtomicString, but lots of places in WebCore were getting compiler errors about ambiguous function calls (since other functions had overrides which took StringViews and AtomicStrings, it didn't know which override to choose). I didn't want to fix each call site since that would be outside the scope of this patch.
> 
> At Anders’s suggestion, in one of my future patches, I add view() functions to both String and AtomicString that return a StringView. Once those are added, this code should be attribute.value().view(). I think it’s better than the construction syntax, since that can mean many different things.

That is going to be tricky because StringView already has a toString() function which returns a String. Giving String a function which returns a StringView means the two classes will depend on each other.

[Darin Adler]

(In reply to comment #16)
> That is going to be tricky because StringView already has a toString() function which returns a String. Giving String a function which returns a StringView means the two classes will depend on each other.

Yes. It’s not all that tricky, though. As I said, I have a patch where I’ve already done this. It’s not significantly different than the relationship that AtomicString and String have with each other.