`srcset` parser doesn’t adhere to the spec

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mat, Assigned: jdm)

Tracking

({regression})

39 Branch
mozilla40
x86
macOS
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38+ fixed, firefox39+ fixed, firefox40 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

1.20 KB, patch
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
3.00 KB, patch
Details | Diff | Splinter Review
15.73 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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:

Open http://w3c-test.org/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html in FF Nightly (39.0a1).


Actual results:

Of 235 parser tests, only 34 pass.


Expected results:

Ideally, 100% of these tests would pass. Failing on edge cases is to be expected, but the differences between the parser as implemented and as specified have led to issues with extremely common use cases (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1139554 )
Blocks: 1017875
Component: Untriaged → DOM
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
The vast majority of these failures are just currentSrc returning null instead of "" and most of the others are bug 1139554. I'll take a look.
Depends on: 1139554
Er, per spec currentSrc can't ever return null: it's not a nullable type.  Why did we implement it as nullable?
(In reply to Not doing reviews right now from comment #2)
> Er, per spec currentSrc can't ever return null: it's not a nullable type. 
> Why did we implement it as nullable?

It was nullable in the spec at the time: https://github.com/zcorpan/picture-element/commit/03aa705fac83
Created attachment 8573063 [details] [diff] [review]
Reject non-standard parses of integers in srcset descriptors

We already have a NonStandard flag from the parser, just add it to the list of flags that make the parse fail

Fixes failures in web-platform-tests suite
Attachment #8573063 - Flags: review?(jst)
Created attachment 8573064 [details] [diff] [review]
Fix srcset parser 'After descriptor' state mishandling spaces

Missed "...  Set position to the previous character in input." step in https://html.spec.whatwg.org/#parse-a-srcset-attribute

That is due to the After descriptor state consuming characters, but then returning the last one to the next loop iteration

Fixes failures in web-platform-tests
Attachment #8573064 - Flags: review?(jst)
Created attachment 8573065 [details] [diff] [review]
<img>.currentSrc should be not be nullable

The spec was updated to make this non-nullable, updated. WPT failures catch this as well.
Attachment #8573065 - Flags: review?(jst)
These + bug 1139554 try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8486fcfa11f6

Down to 229 Pass / 6 Fail, remaining failures are much less concerning, will circle back
Attachment #8573063 - Flags: review?(jst) → review+
Attachment #8573064 - Flags: review?(jst) → review+
Attachment #8573065 - Flags: review?(jst) → review+
Blocks: 1064083
Thanks for the patches here, John!
Assignee: nobody → john
Created attachment 8575426 [details] [diff] [review]
Update WPT expectations.patch

Here's a patch fix web-platform test expectations based on John's fixes.
[Tracking Requested - why for this release]: Potential bugs in new feature.
tracking-firefox38: --- → ?
Attachment #8575426 - Flags: review?(jst) → review+
Thanks Johnny. John, can you check in my patch along with yours?
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox38: ? → +
tracking-firefox39: --- → +
Looks like this still needs check-in. 
It would be good if this can land and we have a chance to uplift it to 38 before next Monday (merge day).
Flags: needinfo?(miket)
Unfortunately I don't have commit access to check these patches in myself.
Johnny, what should happen in this situation? This and bug 1139554 both seem to need check-in.
Flags: needinfo?(jst)
(Assignee)

Comment 15

4 years ago
I'll deal with this.
Flags: needinfo?(jst)
Flags: needinfo?(john)
Sorry for the delay here, I talked with jdm on irc and he's going to check these in. Bug 1139554 can be landed and closed, the patches here can be landed, but a follow-up needs to be opened or this bug left open for the remaining for WPT failures.
Created attachment 8584329 [details] [diff] [review]
update-wpt-2.patch

Fixing the bit-rot that will be caused by bug 1139554 landing before this. Carrying forward r+.
Attachment #8575426 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 1148535
Created attachment 8584776 [details] [diff] [review]
update-wpt-expectations-3.patch

Disabling the test that caused backout. It's passing on my machine but failing others. Will invesigate further in Bug 1064083.

3rd time's a charm?
Attachment #8584329 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa26b2b9c66 is green, let's try again.
Keywords: checkin-needed
hi, that failed to apply:

renamed 1139560 -> update-wpt-expectations-3.patch
applying update-wpt-expectations-3.patch
patching file testing/web-platform/meta/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh update-wpt-expectations-3.patch

could you take a look, thanks!
Flags: needinfo?(miket)
Keywords: checkin-needed
Created attachment 8586252 [details] [diff] [review]
update-wpt-expectations-rebased.patch

OK, that was confusing. Some changes landed (yesterday) on how unicode escapes are handled in the test ini files for web-platform-tests. 

From jgraham, "Basically now most characters in prefs files are stored as unescaped UTF8 except ones below 0x20 (iirc) and ones that have special meanings to the ini parser"

That explains the collision. Rebased and wpt 1-4 are passing @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ab480ebfc8.
Attachment #8584776 - Attachment is obsolete: true
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8334126&repo=mozilla-inbound
Flags: needinfo?(john)
(Assignee)

Comment 27

4 years ago
We are having such a coordination breakdown. This is ridiculous :(
Flags: needinfo?(john)
Agreed, I don't think I'm helping here. :/
Adding dev-doc-needed: not sure if this will need specific doc, but it is a good opportunity to check the accuracy of ours.
Keywords: dev-doc-needed
(Reporter)

Comment 30

4 years ago
Sorry if I’m swooping in mid-process, but how are we looking on landing these fixes? Is there anything we’re waiting on that the RICG could help out with?
(Reporter)

Updated

4 years ago
Flags: needinfo?(josh)
(Assignee)

Comment 31

4 years ago
I just need to finish the merge process here, but thanks for the offer.
Flags: needinfo?(josh)
https://hg.mozilla.org/mozilla-central/rev/341484805367
https://hg.mozilla.org/mozilla-central/rev/6452590ddb77
https://hg.mozilla.org/mozilla-central/rev/61445e93cd38
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 34

4 years ago
Created attachment 8590815 [details] [diff] [review]
Reject non-standard parses of integers in srcset descriptors
(Assignee)

Updated

4 years ago
Attachment #8573063 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: john → josh
(Assignee)

Comment 35

4 years ago
Created attachment 8590816 [details] [diff] [review]
Fix srcset parser 'After descriptor' state mishandling spaces
(Assignee)

Updated

4 years ago
Attachment #8573064 - Attachment is obsolete: true
(Assignee)

Comment 36

4 years ago
Created attachment 8590817 [details] [diff] [review]
<img>.currentSrc should be not be nullable
(Assignee)

Updated

4 years ago
Attachment #8573065 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Created attachment 8590819 [details] [diff] [review]
Update srcset web-platform expectations
(Assignee)

Updated

4 years ago
Attachment #8586252 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
Comment on attachment 8590815 [details] [diff] [review]
Reject non-standard parses of integers in srcset descriptors

Approval Request Comment
[Feature/regressing bug #]: 1018389
[User impact if declined]: Easily-encountered feature brokenness (see originating bug report)
[Describe test coverage new/current, TreeHerder]: Thorough suite of WPT tests.
[Risks and why]: Low. This feature is isolated and these patches merely allow more web content to display as expected; if it breaks, the content just doesn't appear.
[String/UUID change made/needed]: None

Please consider this an approval request for all patches in this bug.
Attachment #8590815 - Flags: approval-mozilla-beta?
Attachment #8590815 - Flags: approval-mozilla-aurora?
Keywords: regression
Comment on attachment 8590815 [details] [diff] [review]
Reject non-standard parses of integers in srcset descriptors

Approving for uplift to aurora. RyanVM, this should apply to all patches in this bug.
Attachment #8590815 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Unsurprisingly, the trunk patch hit bustage on Aurora. I'm making one attempt to fix before backing out:
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f39280c5746
Comment 41 was a bit over-aggressive (stolen from one of the last green Try pushes, but whatever). One more follow-up, already confirmed green on Try.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0eda02ebe76
Comment on attachment 8590815 [details] [diff] [review]
Reject non-standard parses of integers in srcset descriptors

Taking as it is a new feature of 38. Should be in 38 beta 4.
Attachment #8590815 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8590816 [details] [diff] [review]
Fix srcset parser 'After descriptor' state mishandling spaces

[Triage Comment]
Attachment #8590816 - Flags: approval-mozilla-beta+
Attachment #8590816 - Flags: approval-mozilla-aurora+
Attachment #8590817 - Flags: approval-mozilla-beta+
Attachment #8590817 - Flags: approval-mozilla-aurora+
Attachment #8590819 - Flags: approval-mozilla-beta+
Attachment #8590819 - Flags: approval-mozilla-aurora+
Keywords: dev-doc-needed
Duplicate of this bug: 1135800
You need to log in before you can comment on or make changes to this bug.