Closed Bug 1139554 Opened 5 years ago Closed 5 years ago

`srcset` attribute contents are dropped due to parsing error


(Core :: DOM: Core & HTML, defect)

39 Branch
Not set



Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox-esr31 --- wontfix
firefox-esr38 --- affected


(Reporter: mat, Assigned: johns)




(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36

Steps to reproduce:

Use an `srcset` attribute with the `1x` descriptor omitted (an omitted descriptor should default to `1x`, which appears to be working correctly per, despite the parser issue)

So, to reproduce, use the following markup: <img srcset="image, image 2x, image 3x" alt="…">

Actual results:

The parser drops "image, image 2x," and selects the only parsed candidate "image 3x." If the attribute _only_ contains "image, image 2x," the entire attribute is dropped. It seems like the current parser goes on to (attempt to) parse descriptors when the source url ends in a comma, instead of going back to the “splitting loop” step and defaulting that image candidate to 1x.

Expected results:

The image candidate with the omitted descriptor would be used on a "1x" display, while the other candidates would be parsed and applied as expected.
For context, this was reported as a Picturefill issue in FF Nightly (39.0a1):
The issue seems to come from the fact that the srcset parser at ResponsiveImageSelector::SetCandidatesFromSourceSet is not spec compliant and assumes a URL ends with a whitespace, rather than handling candidates of the form "image,".

Also - since picturefill recognizes native support and backs off, this issue is a significant compatibility issue, unless fixed before hitting stable.
[Tracking Requested - why for this release]: Significant compat issue for <picture> enabled in 38
Ever confirmed: true
I think this is just due to passing the wrong iteration point to ConsumeDescriptors
Blocks: 1139560
Comment on attachment 8572827 [details] [diff] [review]
Fix srcset parser mishandling bare URLs followed by a comma

[ Spec for srcset parsing at: ]

When we backtrack on the URL to strip trailing commas we also want to use that updated iterator for the start of descriptors, or it may try to consume the start of the next candidate as a descriptor.

We could skip ConsumeDescriptors if urlEnd != iter, since we know there are none, but special casing the create-a-default-candidate here doesn't seem worth it (ConsumeDescriptors will immediately see the comma, and break to FillCandidate, where the no-descriptors case is handled)
Attachment #8572827 - Flags: review?(jst)
Assignee: nobody → john
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8572827 [details] [diff] [review]
Fix srcset parser mishandling bare URLs followed by a comma

Could we add a test that ensures this doesn't regress down the road as well?

Thanks for the fix! r=jst
Attachment #8572827 - Flags: review?(jst) → review+
Argh, this isn't right because urlEnd can't be passed to ConsumeDescriptors in the common case because it's advancing the iterators. Just remove the whole urlEnd pseudo-optimization and rewind iter -- we advance over commas on the next iteration of the loop anyway. I blame try taking five hours today.

Regarding tests:
The web platform tests discovered this and so should handle it once the expected-fail is updated. I'll push an update to their expected-fail values when I land this + bug 1139560

Is at 34 pass / 201 fail, with this + the 1139560 patches is at 229 Pass / 6 Fail
Attachment #8573061 - Flags: review?(jst)
Attachment #8572827 - Attachment is obsolete: true
These tests in parse-a-srcset-attribute.html cover this issue:

"data:,a, data:,b ("	
"data:,a, data:,b ( "	
"data:,a, data:,b (,"	
"data:,a, data:,b (x"	
"data:,a, data:,b ()"
Comment on attachment 8573061 [details] [diff] [review]
Fix srcset parser mishandling bare URLs followed by a comma

Excellent, r=jst
Attachment #8573061 - Flags: review?(jst) → review+
Blocks: 1064083
Blocks: 1140769
Tracking for 38 and 39 since this sounds significant for compatibility.
This still hasn't landed. John, can you check it in, or is there someone else I should ask to do that? Thanks!
Flags: needinfo?(john)
I'll deal with this.
Flags: needinfo?(john)
I'll prep a patch for that tonight/tomorrow morning.
Here's a patch that updates WPT expectations (which bit-rots the one in 1139560, but I'll update that).
Attachment #8584316 - Flags: review?(jst) → review+
Josh, can you check-in the WPT patch when you get a chance (with John's patch)? I think you might get to it faster than a [checkin-needed] keyword. Thanks!
Flags: needinfo?(josh)
Flags: needinfo?(john)
Blocks: 1148535
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
John, could you fill an uplift request to beta? thanks
Flags: needinfo?(john)
Comment on attachment 8573061 [details] [diff] [review]
Fix srcset parser mishandling bare URLs followed by a comma

Approval Request Comment
[Feature/regressing bug #]: 1018389
[User impact if declined]: New feature isn't feature-complete; some pages will break.
[Describe test coverage new/current, TreeHerder]: There are web-platform tests covering these changes.
[Risks and why]: Low risk, self-contained changes.
[String/UUID change made/needed]: None
Flags: needinfo?(john)
Attachment #8573061 - Flags: approval-mozilla-beta?
Comment on attachment 8584316 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: 1018389
[User impact if declined]: Test breakage on beta.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: NPTOB.
[String/UUID change made/needed]: None
Attachment #8584316 - Flags: approval-mozilla-beta?
Attachment #8573061 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8584316 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should be in 38 beta 3
You need to log in before you can comment on or make changes to this bug.