769207 - (removemutationevents) Get rid of mutation events

Status: RESOLVED FIXED

(Core :: DOM: Events, task, P3)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Mutation events are synchronous, which causes a ludicrous number of bugs and performance issues.  We now have mutation observers, and we'd really like to kill support for mutation events at some point in the future.  Probably not for a while yet.

In bug 766426 I'm going to have to add some kind of hack to account for mutation events, maybe removing an assert.  So I'd like to file a followup to get rid of the hack once we get rid of mutation events, and have it depend on this so we hopefully remember to fix it when we do actually kill mutation events.

Comment 1

[Simon Pieters [:zcorpan]]

Per https://github.com/whatwg/dom/issues/305#issuecomment-2302407051 the rollout of removed mutation events is now 99% of Chrome users, though there's an origin trial to keep mutation events supported.

I think we can turn off mutation events in Nightly and also offer an origin trial to keep them supported. When the number of sites in Chrome's origin trial is very low or they remove the origin trial, we should be able to ship to release.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think it's safe to delete the mutation events completely from our tree. Should I do that? Or do you have a patch for that?

Comment 3

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

I don't have a patch.

Looks like bug 1963043 landed to 140, which is the current ESR, so yes, I think we could remove the code.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, then, I'll write a patch.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, the patches are much much bigger than I've expected. So, I need more time to post the patches to get reviewed.

Unfortunately, my patches still have some mysterious timeout bugs in some tests.
https://treeherder.mozilla.org/jobs?repo=try&revision=31dd97e2c7b3b9427d97f1c3bd0d3a01d49af67f

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 1: Stop collecting number of web apps which use DOM mutation events without `beforeinput` r=smaug!,#dom-core

We won't keep supporting the legacy DOM mutation event listeners.
Therefore, it's impossible to keep collecting the data without making
new path to count the event listeners.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 2: Make `nsIFrame` use new `enum class` to receive the type of attribute change r=smaug!,#dom-core,#layout-reviewers

MutationEvent_Bindings::MODIFICATION,
MutationEvent_Bindings::ADDITION and MutationEvent_Bindings::REMOVAL
will be removed. Therefore, we need alternative enum class for that.
This patch makes nsIFrame and subclasses use the new one.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 3: Make `nsIMutationObserver` use `AttrModType` r=smaug!,#dom-core

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 4: Make `SVGElement::WillChangeValue` return empty value when there is no mutation event listeners r=smaug!,#dom-core,#firefox-svg-reviewers

It returns empty value when there is no mutation event listeners for
the performance. Therefore, after removing the mutation events, it
always return empty value. Therefore, we can make it return nothing.

Note that DevTools uses devtoolsattrmodified event [1], but it does
not have old attribute value, so, removing this code should not break
the DevTools' function.

1. https://searchfox.org/firefox-main/rev/856a307913c2b73765b4e88d32cf15ed05549cae/devtools/server/actors/inspector/walker.js#2049-2054

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 5: Make `Element::GetAttributeChangeHit()` use `AttrModType` r=smaug!,#dom-core

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 6: Make `nsMathMLContainerFrame::ChildListChanged()` stop taking the attribute modification type r=#layout-reviewers!

It's unused.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 7: Get rid of `DOMCharacterDataModified` event r=smaug!,#dom-core

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 8: Get rid of `DOMAttrModified` event r=smaug!,#dom-core,#firefox-svg-reviewers

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 9: Get rid of `DOMNodeInserted` event r=smaug!,#dom-core

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 10: Get rid of `DOMNodeInsertedIntoDocument` and `DOMNodeRemovedFromDocument` events r=smaug!,#dom-core

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 11: Get rid of `DOMSubtreeModified` event r=smaug!,#dom-core

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 12: Get rid of `DOMNodeRemoved` event r=smaug!,#dom-core

Unfortunately, we need to keep dispatching devtoolschildremoved event
to the DevTools. Therefore, this patch renames the DOMNodeRemoved
event dispatcher and related methods.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 13: Get rid of `MutationEvent` r=smaug!,#dom-core

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 769207 - part 14: Get rid of the pref to enable the legacy DOM mutation events and delete tests checking or depending on mutation events r=smaug!,#dom-core,#layout-reviewers,#firefox-svg-reviewers

Comment 20

[Julian Descottes [:jdescottes]]

Hi Masayuki,

I had a quick look at the stack, and this patch removes most of the mutation events available in DevTools as event breakpoints. But I realize they're already broken in Nightly, at least I can't seem to trigger them. I tried to force dom.mutation_events.enabled back to true, I also checked back against ESR 128, but I still can't get the feature to work.

So it feels like this stack is only removing an already broken / missing feature from DevTools, but before we review the patch, do you know why / for how long this has been completely broken in DevTools?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I couldn't reach how to notify DevTools of an event dispatching. Do you know where does it? So, I don't know how to implement the feature and I have no idea how to break it.

Comment 22

[Alexandre Poirot [:ochameau]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #21)

Hmm, I couldn't reach how to notify DevTools of an event dispatching. Do you know where does it? So, I don't know how to implement the feature and I have no idea how to break it.

The main way all DOM Events are trigerring breakpoints in DevTools is via this line:
https://searchfox.org/firefox-main/rev/e02959386f6f89c1476edba10b3902f4e4f3ed4c/dom/events/EventListenerManager.cpp#1377

Maybe<EventCallbackDebuggerNotificationGuard> dbgGuard;

This will ultimately inform DevTools codebase that a DOM event is being processed and pause accordingly.

But may be the Mutation events were going through a special codepath ?
Or DevTools codebase was poorly identifying them.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, but as far as I've tested, at least, DOMNodeInserted, DOMNodeRemvoed and DOMCharacterDataModified break points work for me.
I tested within my test suite, enable the pref and choose these events to log them from "Chose event types to log...".

I guess you took mistake to listen to the DOM mutation events when you tested?

Comment 24

[Julian Descottes [:jdescottes]]

Oh yes for some reason I thought this would break on whatever would trigger the mutation, but you are right. For the breakpoints to trigger we have to actually listen to the events in the content page. So if we're removing the events on the platform side it's completely fine to remove the DevTools counterpart.

And thanks for sharing your test page, looks really useful!

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Robert Longson: Could you review attachment 9510242 [details]? Thanks.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fab82267c957
https://hg.mozilla.org/integration/autoland/rev/f2f10ce158d7
part 1: Stop collecting number of web apps which use DOM mutation events without `beforeinput` r=smaug,dom-core,toolkit-telemetry-reviewers
https://github.com/mozilla-firefox/firefox/commit/784537b41e2f
https://hg.mozilla.org/integration/autoland/rev/a915e9f37b7a
part 2: Make `nsIFrame` use new `enum class` to receive the type of attribute change r=smaug,dom-core,dshin
https://github.com/mozilla-firefox/firefox/commit/f8236195a0f4
https://hg.mozilla.org/integration/autoland/rev/f890e764bf2c
part 3: Make `nsIMutationObserver` use `AttrModType` r=smaug,dom-core,firefox-style-system-reviewers,layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/e7b5c546b985
https://hg.mozilla.org/integration/autoland/rev/fbdefc1feb4a
part 4: Make `SVGElement::WillChangeValue` return empty value when there is no mutation event listeners r=smaug,dom-core,firefox-svg-reviewers,longsonr
https://github.com/mozilla-firefox/firefox/commit/b2e52582540c
https://hg.mozilla.org/integration/autoland/rev/3660b1202a99
part 5: Make `Element::GetAttributeChangeHit()` use `AttrModType` r=smaug,dom-core
https://github.com/mozilla-firefox/firefox/commit/b38a095a5e77
https://hg.mozilla.org/integration/autoland/rev/73507f256e68
part 6: Make `nsMathMLContainerFrame::ChildListChanged()` stop taking the attribute modification type r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/8df231886082
https://hg.mozilla.org/integration/autoland/rev/47b14ec37605
part 7: Get rid of `DOMCharacterDataModified` event r=smaug,dom-core,devtools-reviewers,bomsy
https://github.com/mozilla-firefox/firefox/commit/95faf8027ac9
https://hg.mozilla.org/integration/autoland/rev/c5c00572442e
part 8: Get rid of `DOMAttrModified` event r=smaug,dom-core,firefox-svg-reviewers,devtools-reviewers,longsonr,bomsy
https://github.com/mozilla-firefox/firefox/commit/f5b0b68f6046
https://hg.mozilla.org/integration/autoland/rev/c4d54190394d
part 9: Get rid of `DOMNodeInserted` event r=smaug,dom-core,devtools-reviewers,bomsy
https://github.com/mozilla-firefox/firefox/commit/98cdc0abc092
https://hg.mozilla.org/integration/autoland/rev/2d3ab07233d8
part 10: Get rid of `DOMNodeInsertedIntoDocument` and `DOMNodeRemovedFromDocument` events r=smaug,dom-core,devtools-reviewers,bomsy
https://github.com/mozilla-firefox/firefox/commit/a170eeb9ec63
https://hg.mozilla.org/integration/autoland/rev/c428e8523089
part 11: Get rid of `DOMSubtreeModified` event r=smaug,dom-core,devtools-reviewers,bomsy
https://github.com/mozilla-firefox/firefox/commit/2e2cb7cb9a4c
https://hg.mozilla.org/integration/autoland/rev/6e2db8483078
part 12: Get rid of `DOMNodeRemoved` event r=smaug,dom-core,devtools-reviewers,toolkit-telemetry-reviewers,janerik,bomsy
https://github.com/mozilla-firefox/firefox/commit/a27e993972db
https://hg.mozilla.org/integration/autoland/rev/81e10ab76e1d
part 13: Get rid of `MutationEvent` r=smaug,dom-core,webidl
https://github.com/mozilla-firefox/firefox/commit/ac0474c864c0
https://hg.mozilla.org/integration/autoland/rev/18540c9751b2
part 14: Get rid of the pref to enable the legacy DOM mutation events and delete tests checking or depending on mutation events r=smaug,dom-core

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Waiting for a day to file follow up bugs to link to the relevant inline comment in searchfox.

Comment 28

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f2f10ce158d7
https://hg.mozilla.org/mozilla-central/rev/a915e9f37b7a
https://hg.mozilla.org/mozilla-central/rev/f890e764bf2c
https://hg.mozilla.org/mozilla-central/rev/fbdefc1feb4a
https://hg.mozilla.org/mozilla-central/rev/3660b1202a99
https://hg.mozilla.org/mozilla-central/rev/73507f256e68
https://hg.mozilla.org/mozilla-central/rev/47b14ec37605
https://hg.mozilla.org/mozilla-central/rev/c5c00572442e
https://hg.mozilla.org/mozilla-central/rev/c4d54190394d
https://hg.mozilla.org/mozilla-central/rev/2d3ab07233d8
https://hg.mozilla.org/mozilla-central/rev/c428e8523089
https://hg.mozilla.org/mozilla-central/rev/6e2db8483078
https://hg.mozilla.org/mozilla-central/rev/81e10ab76e1d
https://hg.mozilla.org/mozilla-central/rev/18540c9751b2

Comment 29

[Hamish Willee]

FF144 MDN docs work for this can be tracked in https://github.com/mdn/content/issues/41137

Looking at https://developer.mozilla.org/en-US/docs/Web/API/MutationEvent the events are marked as removed in FF140. My assumption is that the events were disabled behind dom.mutation_events.enabled in that version, and this version actually removes all the code. Is that correct?

If so, there is nothing to do for this from the MDN perspective.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Right, and yes, you don't need to update MDN for this.