1783021 - Tab crashes caused by extension "ContextSearch web-ext"'s Quick Menu after Inspector's Animations View has been activated once

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:105.0) Gecko/20100101 Firefox/105.0
Build ID: 20220803094413

Steps to reproduce

1. In a new profile, install ContextSearch web-ext 1.41.1. Keep extension's settings as default.
2. Open any webpage, e.g. https://www.example.com/.
3. Open Firefox's Inspector. Switch the right-pane to Animations.
4. Invoke ContextSearch's Quick Menu by:
   4a. highlighting a word; and then
   4b. holding a right-click.
5. Dismiss Quick Menu by left-clicking anywhere outside Quick Menu.

Actual result

The tab crashes with signature [@ mozilla::dom::MutationObservers::NotifyAnimationMutated ].

Expected result

No crashes.

Additional notes

1. A crash can be triggered even if Animation View is not active, as long as it has been activated at least once after a page is loaded. I.E. (Step 1 is implied):
   a: step 2 > 3 > switch to Layout View / close Inspector > step 4 > 5 => Crash;
   b: step 2 > 3 > keep Animations View active > reload page > 4 > 5 => Crash;
   c: step 2 > 3 > switch to Layout View / close Inspector > reload page > 4 > 5 => NO Crashes;
   d: open a new tab (about:newtab) > step 3 > 2 > 4 > 5 => Crash.

2. This crash is a longstanding bug. I can reproduce it since 2020-05-13 build but only because ContextSearch's Quick Menu stopped working/cannot be invoked prior to that build.

3. ContextSearch's Quick Menu works most of the time besides this scenario.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:Fanolian+BMO, if you think that's a regression, could you try to find a regression range using for example mozregression?

Comment 2

[Fanolian]

ContextSearch web-ext's author is notified via https://github.com/ssborbis/ContextSearch-web-ext/issues/524.

Comment 3

[Fanolian]

Crash report: https://crash-stats.mozilla.org/report/index/a3c1db3e-819a-4ca0-bb4c-caf5d0220803

Comment 4

[Fanolian]

Attachment: ContextSearch crash and Animation View.mp4

The crash happens since ContextSearch v1.40rc3 which can be found in its github page.

The attached video shows that with v1.40rc3+, Animation View cannot show the animation details and results a crash.

Although the crash is caused by a change of an extension, Firefox should not crash anyway. I am leaving this bug report open.

Comment 5

[ssborbis]

I've narrowed down the cause to removing an element from the shadowDOM before a CSS transition completes. After opening the Inspector window to the Animations tab, the following code should crash a tab relatively consistently.

(() => {
  let s = document.createElement('shadow-test');
  document.documentElement.appendChild(s);
  s.attachShadow({mode:"open"});
  
  let property = 'opacity';
  let initial = '1';
  let final = '0';
  
  let div = document.createElement('div');
  div.style = `${property}:${initial};transition:${property} 2s;`

  s.shadowRoot.appendChild(div);
  div.offsetWidth;

  div.style[property] = final;
  setTimeout(() => div.parentNode.removeChild(div), 1000);
})();

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug has a crash signature, thus the bug will be considered confirmed.

Comment 7

[Fanolian]

With the STR in comment 5, the crash started since 2018-05-30 build.
pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=89d79c2258be1c420153e1adc54338b0165fc88e&tochange=5866d6685849311f057e7e229b9ace63a2641c29

Comment 8

[Julian Descottes [:jdescottes]]

The crash seems to be triggered as soon as we start observing mutations with animations set to true, which is what the animations actor does at https://searchfox.org/mozilla-central/rev/14781effaa15c12b1652beb75f021489567bad8f/devtools/server/actors/animation.js#665-669

Meaning this can be reproduced without the Animation Inspector, by executing the following snippet in chrome scope of a content process:

  let observer = new tabs[0].content.MutationObserver(() => {});
  observer.observe(tabs[0].content.document.documentElement , {
    animations: true,
    subtree: true,
  }); 

And then run the snippet from comment 5 in the tab where you started the observer.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: bug1783021_STRs.html

Comment 10

[Julian Descottes [:jdescottes]]

The crash points to https://searchfox.org/mozilla-central/rev/14781effaa15c12b1652beb75f021489567bad8f/dom/base/MutationObservers.cpp#224, maybe someone from Core can investigate?

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This looks similar (or duplicate of) to bug 1669165.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:boris, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[Boris Chiou [:boris]]

Thanks for the initial investigation. I will take a look later.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I looked this a bit since a relevant bug (bug 1669165) is in our S2 list.

All the crashes here were triggered one of these RemoveProperty calls in Element::UnbindFromTree. An important fact here is that these RemoveProperly calls are invoked after we unset mParent in the same function. And in the MutationObservers::NotifyAnimationMutated function, we end up calling ShadowRoot::FromNode which invokes nsINode::IsShadowRoot and in the function we have;

const bool isShadowRoot = IsInShadowTree() && !GetParentNode();

The GetParentNode() is now totally wrong since mParent has been already nullified.

Just moving up those RemoveProperty calls before nullifying mParent would resolve this issue.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1783021 - Remove animation related properties before nulling out nsINode::mParent. r?smaug!,jdescottes!

Without this change we can't tell properly whether the element in question is the
shadow root or not in animation mutation observer, specifically at a
ShadowRoot::FromNote call in ForEachAncestorObserver [1].

The test case in this change is based on a test case created by Julian Descottes [2].

[1] https://searchfox.org/mozilla-central/rev/b1e5f2c7c96be36974262551978d54f457db2cae/dom/base/MutationObservers.cpp#67
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1783021#c9

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Thanks you Julian for the test case in comment 9! It was quite easy to write a mochitest based on the test case.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1214807cfb19
Remove animation related properties before nulling out nsINode::mParent. r=smaug,jdescottes

Comment 19

[Noemi Erli[:noemi_erli]]

Backed out changeset 1214807cfb19 (Bug 1783021) for causing failures in test_animation_observers_sync.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=1214807cfb191663dca1a9fd5d089806d371525a&selectedTaskRun=fu2hkvEKTV61bOcJdgOiFw.0

Log: https://treeherder.mozilla.org/logviewer?job_id=391251531&repo=autoland&lineNumber=2854

Backout: https://hg.mozilla.org/integration/autoland/rev/c71974b43478f31f39cf6f8ed0c73463e012d9c5

Comment 20

[Hiroyuki Ikezoe (:hiro)]

The test, test_animation_observers_sync.html caused a bunch of assertion "ASSERTION: shouldn't have observed an animation being removed", this is a variant of bug 1189015.

With the change for this bug, in ForEachAncestorObserver we properly walk up DOM tree node here so that if a MutationObserver observe the parent element of the animated element, thus AnimationAdded gets called twice, one is for the animated element, the other one is the parent element.

I am not sure what the proper fix for bug 1189015, I am just going to allow the assertions in the test. For that, we need bug 1792514.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3bd80aa22b
Remove animation related properties before nulling out nsINode::mParent. r=smaug,jdescottes

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7b3bd80aa22b

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox106 to wontfix.

For more information, please visit auto_nag documentation.