1451940 - The fix in bug 1415352 may have affected web content like the NASA/JPL JWPlayer instance.

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Thomas Wisniewski]

While investigating webcompat.com issues #16245, mozregression pointed me toward bug 1415352 being the cause for the JWPlayer elements on that NASA/JPL video page to no longer be styled correctly. Indeed, it seems as though inline styles that were once applied are no longer being applied.

Kris, can you think of why that might be? Extensions aren't involved, as the problem happens in a fresh profile. I don't see any CSP warnings or overtly obvious messages in the consoles.

Comment 1

[Kris Maglione [:kmag]]

Hm. So, this is kind of a strange case.

Apparently what's happening is that the site is creating a bunch of empty <style> nodes, inserting rules via their .styleSheet attributes, and then eventually setting .textContent to a null string.

With the previous code, when that happened, as long as there were no previous text nodes to remove, we never triggered any mutation observers, so the stylesheet never got re-parsed.

With the new code, we treat assigning to textContent the same as we treat assigning to innerHTML, and assigning an empty string does cause the stylesheet to be re-parsed, which in tern throws away the old rules.

We could special case assigning the empty string to textContent in that function, and ignore it if the node is already empty, and the subject principal is the same... But that seems wrong. The new behavior seems correct. But if it isn't, I think we should probably treat assignments to innerHTML and assignments to textContent the same.

Boris, do you have any opinion?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So per spec at https://dom.spec.whatwg.org/#dom-node-textcontent, setting textContent to null or the empty string (defined to be equivalent) on an element should call https://dom.spec.whatwg.org/#concept-node-replace-all with "node" set to null, which will do the following:

1. Step 1 is a no-op because "node" is null.
2. Set removedNodes to the parent's children.  In this case, empty list.
3. Set addedNodes to the empty list (because "node" is null).
4. Remove all the parent's children.  There aren't any, so this actually does nothin.
5. "node" is null, so nothing happens in step 5.

In particular, none of <https://dom.spec.whatwg.org/#concept-node-insert>, <https://dom.spec.whatwg.org/#concept-node-remove>, and <https://dom.spec.whatwg.org/#concept-cd-replace> are reached.  Those are the only algorithms that invoke https://dom.spec.whatwg.org/#concept-node-text-change-ext which is the hook HTML uses in https://html.spec.whatwg.org/multipage/semantics.html#the-style-element:update-a-style-block to trigger re-parsing of the sheet.

So per spec assigning null or empty string to textContent should not re-parse if there are no existing kids.

Assigning "" to innerHTML if the node has no kids to start with... looks like that should also not re-parse per spec.  Worth testing what other browsers do.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element.

Per spec, assinging an empty string to textContent or innerHTML should not
trigger any mutation observers, and therefore should not trigger a stylesheet
reparse, when the element is already empty.

Since we need to apply some special handling to innerHTML and textContent
assignments to <style> nodes, and therefore re-parse directly in the setter
rather than in response to mutation observers, we need to special-case this
scenario.

Review commit: https://reviewboard.mozilla.org/r/234788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234788/

Comment 4

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> Assigning "" to innerHTML if the node has no kids to start with... looks
> like that should also not re-parse per spec.  Worth testing what other
> browsers do.

Chromium treats it the same as textContent. I don't have any other browsers handy to test with.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8966068 [details]
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element.

https://reviewboard.mozilla.org/r/234788/#review240666

Thank you for fixing this!  r=me with the one nit and some additions to the test.

::: dom/html/HTMLStyleElement.cpp:179
(Diff revision 1)
>                                           ErrorResult& aError)
>  {
> +  // Per spec, if we're setting text content to an empty string and don't
> +  // already have any children, we should not trigger any mutation observers, or
> +  // re-parse the stylesheet.
> +  if (aTextContent.IsEmpty() && !GetChildCount()) {

Probably faster to check !GetFirstChild().  There's no guarantee that GetChildCount() will stay O(1).

::: testing/web-platform/tests/css/cssom/css-style-reparse.html:34
(Diff revision 1)
> +      assert_equals(getComputedStyle(div).minWidth, "0px");
> +
> +      style.sheet.insertRule("#test-div { min-width: 42px; }");
> +      assert_equals(getComputedStyle(div).minWidth, "42px");
> +
> +      style[prop] = "";

Could you also add tests that:

1)  Going from no kids to a single length-0 textnode kid (via appendChild) reparses.

2)  Setting to "" when the only kid is a length-0 textnode reparses.

Basically those are cases in which style[prop] == "", but setting style[prop] = "" reparses...

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8966068 [details]
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element.

https://reviewboard.mozilla.org/r/234788/#review240666

> Probably faster to check !GetFirstChild().  There's no guarantee that GetChildCount() will stay O(1).

Fixed.

> Could you also add tests that:
> 
> 1)  Going from no kids to a single length-0 textnode kid (via appendChild) reparses.
> 
> 2)  Setting to "" when the only kid is a length-0 textnode reparses.
> 
> Basically those are cases in which style[prop] == "", but setting style[prop] = "" reparses...

Done.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95438fe6d5a87270b1ad713da14f123b17faaea2
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element. r=bz

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/95438fe6d5a8

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8966068 [details]
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1415352
[User impact if declined]: This bug causes breakage to some unknown percentage of web pages, notably any web page using the unprefixed.js library in combination with JW player (such as NASA/JPL).
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: The main change is to restore our previous behavior in the corner case of assigning an empty string to the textContent of a <style> element. It also slightly changes the behavior of the innerHTML setter to match the behavior of the textContent setter, which is different from our old behavior, but matches the behavior of other browsers.
[String changes made/needed]: None.

Comment 10

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

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

Comment 11

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

Upstream PR merged

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8966068 [details]
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element.

let's get this in beta asap.  approved for 60.0b12

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/84d5902b3a02

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

I'll leave this tracking for 59, in case we have a dot release. Let's see how the fix does in beta 12.

Comment 15

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using the steps and the link from comment 0 and issue #16245 (https://webcompat.com/issues/16245) to be more precise. I reproduced it using an older version of Nightly (2018-04-05) on Windows 10 x64.

I retested everything using latest Nightly 61.0a1 and beta 60.0b12 on Windows 10 x64, Ubuntu 16.04 x 64 and macOS 10.12. The bug is not reproducing anymore.

Comment 16

[Julien Cristau [:jcristau]]

60 is now on m-r, setting 59 to wontfix.