Closed Bug 1451940 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 + wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: wisniewskit, Assigned: kmag)

References

()

Details

(Keywords: regression, Whiteboard: [domsecurity-active][wptsync upstream error])

Attachments

(1 file)

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.
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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 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...
Attachment #8966068 - Flags: review?(bzbarsky) → review+
Assignee: nobody → kmaglione+bmo
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95438fe6d5a87270b1ad713da14f123b17faaea2
Bug 1451940: Don't reparse stylesheets when assigning empty string to empty element. r=bz
https://hg.mozilla.org/mozilla-central/rev/95438fe6d5a8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.
Attachment #8966068 - Flags: approval-mozilla-release?
Attachment #8966068 - Flags: approval-mozilla-beta?
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10410 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-active] → [domsecurity-active][wptsync upstream]
Whiteboard: [domsecurity-active][wptsync upstream] → [domsecurity-active][wptsync upstream error]
Upstream PR merged
Whiteboard: [domsecurity-active][wptsync upstream error] → [domsecurity-active][wptsync upstream]
Whiteboard: [domsecurity-active][wptsync upstream] → [domsecurity-active][wptsync upstream error]
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
Attachment #8966068 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I'll leave this tracking for 59, in case we have a dot release. Let's see how the fix does in beta 12.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
60 is now on m-r, setting 59 to wontfix.
Attachment #8966068 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.