1139560 - `srcset` parser doesn’t adhere to the spec

Status: RESOLVED FIXED

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

Description

[Mat Marquis]

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 )

Comment 1

[John Schoenick [:johns]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, per spec currentSrc can't ever return null: it's not a nullable type.  Why did we implement it as nullable?

Comment 3

[John Schoenick [:johns]]

(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

Comment 4

[John Schoenick [:johns]]

Attachment: 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

Comment 5

[John Schoenick [:johns]]

Attachment: 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

Comment 6

[John Schoenick [:johns]]

Attachment: <img>.currentSrc should be not be nullable

The spec was updated to make this non-nullable, updated. WPT failures catch this as well.

Comment 7

[John Schoenick [:johns]]

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

Comment 8

[Andrew Overholt [:overholt]]

Thanks for the patches here, John!

Comment 9

[Mike Taylor [:miketaylr]]

Attachment: Update WPT expectations.patch

Here's a patch fix web-platform test expectations based on John's fixes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Potential bugs in new feature.

Comment 11

[Mike Taylor [:miketaylr]]

Thanks Johnny. John, can you check in my patch along with yours?

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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).

Comment 13

[Mike Taylor [:miketaylr]]

Unfortunately I don't have commit access to check these patches in myself.

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Johnny, what should happen in this situation? This and bug 1139554 both seem to need check-in.

Comment 15

[Josh Matthews [:jdm]]

I'll deal with this.

Comment 16

[John Schoenick [:johns]]

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.

Comment 17

[Mike Taylor [:miketaylr]]

Attachment: update-wpt-2.patch

Fixing the bit-rot that will be caused by bug 1139554 landing before this. Carrying forward r+.

Comment 18

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8b305f26b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2cd9c336c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ef86da9b15

Comment 19

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcb5e699ef5

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for w-p-t failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc49f2ed016

https://treeherder.mozilla.org/logviewer.html#?job_id=8158212&repo=mozilla-inbound

Comment 21

[Mike Taylor [:miketaylr]]

Attachment: 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?

Comment 22

[Mike Taylor [:miketaylr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa26b2b9c66 is green, let's try again.

Comment 23

[Carsten Book [:Tomcat]]

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!

Comment 24

[Mike Taylor [:miketaylr]]

Attachment: 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.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70e0f2d8fb1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ed34bd6ae1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb871246ca63
https://hg.mozilla.org/integration/mozilla-inbound/rev/80c32af73390

Comment 26

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8334126&repo=mozilla-inbound

Comment 27

[Josh Matthews [:jdm]]

We are having such a coordination breakdown. This is ridiculous :(

Comment 28

[Mike Taylor [:miketaylr]]

Agreed, I don't think I'm helping here. :/

Comment 29

[Jean-Yves Perrier [:teoli]]

Adding dev-doc-needed: not sure if this will need specific doc, but it is a good opportunity to check the accuracy of ours.

Comment 30

[Mat Marquis]

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?

Comment 31

[Josh Matthews [:jdm]]

I just need to finish the merge process here, but thanks for the offer.

Comment 32

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/341484805367
https://hg.mozilla.org/integration/mozilla-inbound/rev/6452590ddb77
https://hg.mozilla.org/integration/mozilla-inbound/rev/61445e93cd38

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/341484805367
https://hg.mozilla.org/mozilla-central/rev/6452590ddb77
https://hg.mozilla.org/mozilla-central/rev/61445e93cd38

Comment 34

[Josh Matthews [:jdm]]

Attachment: Reject non-standard parses of integers in srcset descriptors

Comment 35

[Josh Matthews [:jdm]]

Attachment: Fix srcset parser 'After descriptor' state mishandling spaces

Comment 36

[Josh Matthews [:jdm]]

Attachment: <img>.currentSrc should be not be nullable

Comment 37

[Josh Matthews [:jdm]]

Attachment: Update srcset web-platform expectations

Comment 38

[Josh Matthews [:jdm]]

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.

Comment 39

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d83a84142769
https://hg.mozilla.org/releases/mozilla-aurora/rev/3057b13be498
https://hg.mozilla.org/releases/mozilla-aurora/rev/c7d9cd5c2f8b
https://hg.mozilla.org/releases/mozilla-aurora/rev/422331ee9895

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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 42

[Ryan VanderMeulen [:RyanVM]]

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 43

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590816 [details] [diff] [review]
Fix srcset parser 'After descriptor' state mishandling spaces

[Triage Comment]

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dffb5c867f47
https://hg.mozilla.org/releases/mozilla-beta/rev/07666fc071be
https://hg.mozilla.org/releases/mozilla-beta/rev/7285a02cd883
https://hg.mozilla.org/releases/mozilla-beta/rev/cd5b5709b2e4