Closed Bug 1139560 Opened 9 years ago Closed 9 years ago

`srcset` parser doesn’t adhere to the spec

Categories

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

39 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 + fixed
firefox39 + fixed
firefox40 --- fixed

People

(Reporter: mat, Assigned: jdm)

References

Details

(Keywords: regression)

Attachments

(4 files, 7 obsolete files)

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
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 )
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
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)
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)
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
Attached patch Update WPT expectations.patch (obsolete) — Splinter Review
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.
Attachment #8575426 - Flags: review?(jst) → review+
Thanks Johnny. John, can you check in my patch along with yours?
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)
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.
Attached patch update-wpt-2.patch (obsolete) — Splinter Review
Fixing the bit-rot that will be caused by bug 1139554 landing before this. Carrying forward r+.
Attachment #8575426 - Attachment is obsolete: true
Blocks: 1148535
Attached patch update-wpt-expectations-3.patch (obsolete) — Splinter Review
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
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)
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
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)
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
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?
Flags: needinfo?(josh)
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
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8573063 - Attachment is obsolete: true
Assignee: john → josh
Attachment #8573064 - Attachment is obsolete: true
Attachment #8573065 - Attachment is obsolete: true
Attachment #8586252 - Attachment is obsolete: true
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.