1460295 - unresponsive visiting login page on eqbank.ca

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Once I hit the login page Firefox becomes completely unresponsive.  CPU spikes and given a minute all memory is consumed.  I get the script unresponsive bar and can try to stop it, but beyond that nothing in the UI works.  I have to force quit.

Tested on a new profile with a nightly build OSX to ensure it wasn't an extension or any pref settings.

Just go to eqbank.ca and hit the login button.

Comment 1

[Marion Daly [:mdaly]]

Can verify the site crashes firefox in nightly

Sending to Layout to give it a first pass to see where this bug belongs

Comment 2

[Shane Caraveo (:mixedpuppy)]

Additional note: no apparent problem on Safari/Chrome.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Seems to be nested DOMSubtreeModified event from changing style.left.

IIRC there were something similar found recently...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

This is a profile I captured from the signin page: https://perfht.ml/2KaXpim

According to the stack in this profile, I bet the following code is the reason:
> b(s).bind("DOMSubtreeModified", function() {
>   b(s).find("img").css("left", "15px");
>   b(s).parent()
>     .css("padding-top", "0px")
>     .css("margin-top", "-1px")
>     .css("padding-bottom", "0px");
>   b(s).find("img")
>     .css("height", "24px")
>     .css("width", "24px");
> })

I just found the other related case, which is bug 1449584. In that case, mutating style attribute of descendants triggers mutation observer. Since mutation observer is asynchronous, it doesn't block the page, it just makes it consume lots of CPU. However in this case, the synchronous event is used, so it's even worse.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

If we want to fix this from our side, we should change the spec to make it not change the order when setting property with identical value, like what Blink is currently doing[1].

Probably worth raising in the CSSWG meeting and ask the working group to resolve on something...


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_property_value_set.cc?l=405-421&rcl=59f892fc9d3c442f4381ebd220658b19644812ba

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Raised w3c/csswg-drafts#2667.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(Also, it is easier to fix than bug 1449584 from our side, actually, because we can easily avoid triggering mutation event without much change, while stopping triggering mutation observer is more involving. See bug 1197705.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Xidorn and I talked about this. I'll send a Blink patch switching to the spec behavior tomorrow, that should make us know whether they're interested on taking it or not. If not, we can switch back.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

https://chromium-review.googlesource.com/c/chromium/src/+/1054867

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I got that in a landable state and asked Rune Lillesveen about whether they'd be interested in taking the patch.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase for DOMSubtreeModified on attribute mutation

So I just noticed that in my patched Blink build this page wasn't broken and that was because DOMSubtreeModified isn't fired for attribute mutations in Blink.

Olli, is that expected or a Blink bug? Should we avoid firing them for attribute mutations?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

For comment 11.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Web visible regression that renders a site unusable. Not sure if this single site is enough to justify tracking, but worth putting it in the relevant radars.

I contacted the site about this btw, not sure whether a reply is expected or not.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11) 
> Olli, is that expected or a Blink bug?
Blink bug


"This is a general event for notification of all changes to the document. It can be used instead of the more specific events listed below. It may be dispatched after a single modification to the document or, at the implementation's discretion, after multiple changes have occurred. The latter use should generally be used to accommodate multiple changes which occur either simultaneously or in rapid succession. The target of this event is the lowest common parent of the changes which have taken place. This event is dispatched after any other events caused by the mutation(s) have occurred. "


Of course mutation events in general are underspec'ed

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

It seems Blink doesn't trigger DOMSubtreeModified when the attribute's value changes, but it does trigger so when the attribute is added or removed.

Given that this is a depercated event, and many of its fields already return nonsense, do you think we can change our behavior to match what Blink has, so that we can trigger the event fewer times (and thus improve performance on this kind of broken pages)?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

https://groups.google.com/a/chromium.org/d/msgid/blink-dev/1cd31de0-ac1e-7234-38ba-38b979d1aa0a%40mozilla.com is the intent thread in blink-dev@ regarding the CSS change btw.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

The WHATWG DOM spec has explicitly removed mutation events[1] but browsers still have it, which doesn't seem to be a useful state. We probably should either have them unshipped or add them back to the spec, I guess.

[1] https://dom.spec.whatwg.org/#dom-events-changes

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Would need to then reverse engineer exactly what blink does. That feels a tad risky.

Event's field's don't return nonsense.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Oh wait, so UI Events is still a spec in maintance, so we can have it update to reflect what should be done.

(In reply to Olli Pettay [:smaug] from comment #18)
> Would need to then reverse engineer exactly what blink does. That feels a
> tad risky.

We can ask blink people to put their behavior into spec and probably add wpt for it.

> Event's field's don't return nonsense.

At least for DOMSubtreeModified event, none of the fields of MutationEvent returns anything useful given the current spec[1].

[1] https://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMSubtreeModified

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

null/0/"" != nonsense
the whole point is to use the same interface for all the mutation events, and then each of the event types use the field which are relevant to the particular event.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Well, they are kinda nonsense when it's triggered for attribute change, because you don't know what's happening from the event information.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

According to caniuse Blink / WebKit just don't support DOMAttrModified event.

Comment 23

[Boris Zbarsky [:bzbarsky]]

One option may be to stop firing attribute mutation events for the style attribute specifically, as an interim step toward stopping firing them altogether (which I also think we should do)...

Comment 24

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

Tracking for 62 so that we will be sure to follow up (whether for 62 or a later release)

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

In bug 1461696 I added a telemetry for DOMAttrModified event, and hopefully we can have the patch uplifted into beta. If that telemetry doesn't show scary usage, we can probably unship that event and have DOMSubtreeModified not fired for attribute changes.

Comment 26

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

The patch in bug 1461696 landed in beta on May 16, last Wednesday, so it would have shipped with 61 beta 6 on Friday. I'm guessing telemetry results may not be clear for a few days, but please let us know once you are able to take a look.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.

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

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

This is a conservative approach to stop dispatching mutation event for changes to style attribute from CSSOM.

I've verified locally that this fixes the issue on this specific website.

This change also make this behavior behind a pref so that we can revert it if necessary.

I'll add a test if there isn't any already. (I've triggered a try run for it.)

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

I can send a intend to change / unship stuff if it's necessary...

Comment 30

[Boris Zbarsky [:bzbarsky]]

> I can send a intend to change / unship stuff if it's necessary...

Yes, this definitely needs an intent describing compat risks, status in other engines, etc.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979429 [details]
Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.

https://reviewboard.mozilla.org/r/245616/#review251896

This probably needs a test.

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8979429 [details]
Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245616/diff/1-2/

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8979429 [details]
Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.

It seems there are already tests for checking this behavior. I just updated those tests to work in both ways. You may want to have another look at the test changes.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979429 [details]
Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.

https://reviewboard.mozilla.org/r/245616/#review252206

Test changes look great, thank you!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/430e2dc50a2a
Don't dispatch mutation event for style attribute change from CSSOM. r=bz

Comment 36

[Eric Shepherd [:sheppy]]

Adding dev-doc-needed for the unship of DOMAttrModified and DOMSubtreeModified events being triggered by CSSOM changes.

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/430e2dc50a2a

Comment 38

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/domattrmodified-and-domsubtreemodified-events-are-no-longer-fired-when-style-attribute-is-changed-via-cssom/

Comment 39

[Chris Mills [:cmills]]

I've documented this, adding a rel note here:
https://developer.mozilla.org/en-US/Firefox/Releases/62#DOM_2

And a note here:
https://developer.mozilla.org/en-US/docs/Web/Events/DOMSubtreeModified

We never documented DOMAttrModified