1827512 - script with only whitespace between opening and closing tags is not run if its src and type change, unlike in Blink and WebKit

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Thomas Wisniewski [:twisniewski]]

In the attached test-case, a script element has a non-JS type attribute, and whitespace in between its opening and closing tags. Its src and type are then dynamically set to JS and a console.log statement, but the new script is never run.

In WebKit and Blink, the script is run, seemingly as long as its type was non-JS, but changed to JS when the src changes.

This is causing a webcompat issue which breaks page load at www.legorafi.fr, where their rocket loader script loading relies on such a script change to fire either onload or onerror on such a change. This ultimately causes links to not work, as the site still has a preventDefault enabled on them which should not be there after the scripts have loaded.

Comment 1

[Thomas Wisniewski [:twisniewski]]

Attachment: testcase.html

Comment 2

[Thomas Wisniewski [:twisniewski]]

Oddly, mozregression is suggesting that this may have broken in this (hidden) revision: https://phabricator.services.mozilla.com/D166108 (bug 1802385)

Comment 3

[Kris Maglione [:kmag]]

I think this was probably regressed by bug 1784187. After that, scripts without src attributes are executed as inline scripts whether or not they contain any text content.

I'm not sure how big of an issue this is likely to be in practice. I think most people generally assume that <script> nodes to ignore src changes after they're inserted into the DOM, but given that we have an example where someone doesn't, and Blink and WebKit appear different, it could be a significant webcompat issue.

Tentatively setting the severity to S1, but I would accept arguments that it should be lower.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1784187

:avandolder, since you are the author of the regressor, bug 1784187, could you take a look?

For more information, please visit auto_nag documentation.

Comment 5

[Adam Vandolder [:avandolder]]

(In reply to Kris Maglione [:kmag] from comment #3)

I think this was probably regressed by bug 1784187. After that, scripts without src attributes are executed as inline scripts whether or not they contain any text content.

So this is not actually a regression at all, as far as I can tell. Bug 1784187 isn't at fault - it only adds a microtask checkpoint for "empty" scripts, which doesn't apply here since that definition of "empty" only includes 0-length text fragments, but in this case there's whitespace between the beginning and ending tags (which explains the "If I change it so that there isn't any whitespace between <script> and </script>, it works" comment from the linked web-compat issue). The problem is that we won't re-process script tags with any content in them whatsoever, unlike Blink or Webkit, who are happy to do a re-process when the src attribute is changed, as long as the script hasn't been executed already.

Comment 6

[Adam Vandolder [:avandolder]]

It looks like ScriptElement::MaybeProcessScript violates the spec for preparing an element, in that it performs step 14 (setting mAlreadyStarted to true) before steps 8-12 (checking the script type). Which would cause the difference in behaviour seen here.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I tried to find regression window and what I got so far seems more aligned with Adam's comment 5, i.e. this isn't a regression from bug 1784187. I could even reproduce this issue back to esr 102 and Fx 85. Neither the site nor the test case had ever worked. My finding is inconsistent with the webcompat report, so I added "regressionwindow-wanted" keyword to let this be on QA's radar.

Comment 8

[Adam Vandolder [:avandolder]]

Attachment: Bug 1827512 - Properly handle type and src attribute changes on script tags. r?smaug

Modifies the preparation of script tags in order to better follow the
specification. Now, scripts are only considered to have already started
if they have a valid type when being prepared, and only changes to the
src attribute trigger script preparation, instead of for all attributes.
This allows script tags to be re-run after their type and src are
modified, if they haven't run already.

Comment 9

[Thomas Wisniewski [:twisniewski]]

This also seems to be affecting www.billboardvinyls.com, as seen on webcompat.com

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Reviewed in the team meeting, moving to S3 per comment 6 and comment 7.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, this bug has a P1 WebCompat priority.
:avandolder, could you consider increasing the severity of this web compatibility bug?

For more information, please visit auto_nag documentation.

Comment 12

[Thomas Wisniewski [:twisniewski]]

For what it's worth, this is already known to be breaking links and galleries on two fairly popular websites, and I would be amazed if it isn't breaking more sites given the nature of the issue.

Comment 13

[Daniel Bodea [:danibodea]]

Assuming that only the test case is needed to investigate the regrassion range of this issue and that the expected result of it is to show a green square instead of a red one, then all tested builds are affected: Tested builds as old as Nightly v50.0a1 on both WIndows 10 and Ubuntu 22.

I may be failing to understand how this bug is to be reproduced (consistently). Please let me know if further investigation for a regression range is not necessary anymore or if it is, please explain how the issue should be tested.

Comment 14

[Adam Vandolder [:avandolder]]

(In reply to Daniel Bodea [:danibodea] from comment #13)

Assuming that only the test case is needed to investigate the regrassion range of this issue and that the expected result of it is to show a green square instead of a red one, then all tested builds are affected: Tested builds as old as Nightly v50.0a1 on both WIndows 10 and Ubuntu 22.

I may be failing to understand how this bug is to be reproduced (consistently). Please let me know if further investigation for a regression range is not necessary anymore or if it is, please explain how the issue should be tested.

No, you have it correct - this behaviour doesn't seem to be a regression, rather it's been the way Firefox has always handled it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/579cee2f1eda
Properly handle type and src attribute changes on script tags. r=smaug

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40538 for changes under testing/web-platform/tests

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/579cee2f1eda

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:avandolder, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Adam Vandolder [:avandolder]]

Attachment: Beta Uplift Approval Request

Approval Request Comment
[Feature/Bug causing the regression]: This isn't actually a regression, however the existing behaviour is not web compatible
[User impact if declined]: Multiple websites (see, for instance, https://webcompat.com/issues/113086) will continue to not function for 115 users.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: This change is of low risk.
[Why is the change risky/not risky?]: This change is not particularly risky since it fixes a fairly significant spec violation, but one which is obscure enough that it is unlikely any sites were relying on the current behaviour.
[String changes made/needed]: None

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9329114 [details]
Bug 1827512 - Properly handle type and src attribute changes on script tags. r?smaug

Moving beta uplift request to the patch

Comment 23

[Donal Meehan [:dmeehan]]

Comment on attachment 9329114 [details]
Bug 1827512 - Properly handle type and src attribute changes on script tags. r?smaug

Approved for 115.0b7.

Comment 24

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bffe91ed683