Closed Bug 769207 (removemutationevents) Opened 14 years ago Closed 1 year ago

Get rid of mutation events

Categories

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

Product:
Core
Component:
DOM: Events
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
144 Branch
Tracking Flags:
Tracking Status
firefox144 --- fixed

People

(Reporter: ayg, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, webcompat:platform-bug)

Attachments

(14 external files)

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.
Depends on: 758493
Keywords: site-compat
Priority: -- → P3
Component: DOM → DOM: Core & HTML
Type: enhancement → task
Depends on: 1694637
Alias: killmutationevents → removemutationevents
Severity: normal → S3
Depends on: 1827132
Component: DOM: Core & HTML → DOM: Events

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.

Depends on: 1914513
Blocks: 1952620
No longer depends on: 1952620
Depends on: 941729
Blocks: 724128
No longer depends on: 1694637
Blocks: 1827132
No longer depends on: 1827132

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?

Flags: needinfo?(smaug)

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.

Flags: needinfo?(smaug)

Thanks, then, I'll write a patch.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

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

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.

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.

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

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

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?

Flags: needinfo?(masayuki)

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.

Flags: needinfo?(masayuki) → needinfo?(jdescottes)

(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.

Flags: needinfo?(jdescottes)

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?

Flags: needinfo?(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!

Flags: needinfo?(jdescottes)

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

Flags: needinfo?(longsonr)
Flags: needinfo?(longsonr)
Pushed by masayuki@d-toybox.com: 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

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

QA Whiteboard: [qa-triage-done-c145/b144]

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.

Flags: needinfo?(masayuki)

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

Flags: needinfo?(masayuki)
Regressions: 1992769
Regressions: 2089479
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: