Closed Bug 1460295 Opened 6 years ago Closed 6 years ago

unresponsive visiting login page on eqbank.ca

Categories

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

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- ?
firefox62 + fixed

People

(Reporter: mixedpuppy, Assigned: xidorn)

References

(
URL
)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat])

Attachments

(2 files)

Testcase for DOMSubtreeModified on attribute mutation
310 bytes, text/html
Details
Bug 1460295 - Don't dispatch mutation event for style attribute change from CSSOM.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
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.
Can verify the site crashes firefox in nightly

Sending to Layout to give it a first pass to see where this bug belongs
Component: General → Layout
Additional note: no apparent problem on Safari/Chrome.
Priority: -- → P2
Seems to be nested DOMSubtreeModified event from changing style.left.

IIRC there were something similar found recently...
Component: Layout → DOM: CSS Object Model
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.
See Also: → 1449584
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
Whiteboard: [webcompat]
(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.)
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.
Flags: needinfo?(emilio)
I got that in a landable state and asked Rune Lillesveen about whether they'd be interested in taking the patch.
Flags: needinfo?(emilio)
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?
Attachment #8975780 - Attachment mime type: text/plain → text/html
For comment 11.
Flags: needinfo?(bugs)
[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.
tracking-firefox62: --- → ?
(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
Flags: needinfo?(bugs)
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)?
Flags: needinfo?(bugs)
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
Would need to then reverse engineer exactly what blink does. That feels a tad risky.

Event's field's don't return nonsense.
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
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.
Flags: needinfo?(bugs)
Well, they are kinda nonsense when it's triggered for attribute change, because you don't know what's happening from the event information.
According to caniuse Blink / WebKit just don't support DOMAttrModified event.
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)...
Tracking for 62 so that we will be sure to follow up (whether for 62 or a later release)
status-firefox61: --- → ?
status-firefox62: --- → affected
tracking-firefox62: ? → +
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.
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.
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.)
I can send a intend to change / unship stuff if it's necessary...
> 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 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.
Attachment #8979429 - Flags: review?(bzbarsky) → review+
Keywords: site-compat
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.
Attachment #8979429 - Flags: review+ → review?(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!
Attachment #8979429 - Flags: review?(bzbarsky) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/430e2dc50a2a
Don't dispatch mutation event for style attribute change from CSSOM. r=bz
Adding dev-doc-needed for the unship of DOMAttrModified and DOMSubtreeModified events being triggered by CSSOM changes.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/430e2dc50a2a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1463920
Blocks: 1461285
Assignee: nobody → xidorn+moz
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: