1462746 - Crash in mozilla::ShouldClearTargets

Status: VERIFIED FIXED

Description

[Marcia Knous [:marcia - needinfo? me]]

This bug was filed from the Socorro interface and is
report bp-952bf2b4-d13d-43c1-9071-e51e60180514.
=============================================================

Seen while looking at Mac crash stats: https://bit.ly/2KA0vwo. 11 crashes/9 installs on Mac in the last 7 days. Crashes seemed to have started with Build 20180512220039

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aabfe960ab59fea2e85896b1f8050786e16ab23b&tochange=a7461494a7a0bb3d80c1449c1d5d484d8b568d98

Top 10 frames of crashing thread:

0 XUL mozilla::ShouldClearTargets dom/base/nsWrapperCache.h:264
1 XUL mozilla::EventDispatcher::Dispatch dom/events/EventDispatcher.cpp:955
2 XUL mozilla::EventDispatcher::DispatchDOMEvent dom/events/EventDispatcher.cpp
3 XUL nsINode::DispatchEvent dom/base/nsINode.cpp:1079
4 XUL nsContentUtils::DispatchEvent dom/base/nsContentUtils.cpp:4471
5 XUL nsContentUtils::DispatchTrustedEvent dom/base/nsContentUtils.cpp:4439
6 XUL mozilla::dom::HTMLSlotElement::FireSlotChangeEvent dom/html/HTMLSlotElement.cpp:241
7 XUL nsDOMMutationObserver::HandleMutationsInternal dom/base/nsDOMMutationObserver.cpp:949
8 XUL mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint xpcom/base/CycleCollectedJSContext.cpp:543
9 XUL mozilla::CycleCollectedJSContext::AfterProcessTask xpcom/base/CycleCollectedJSContext.cpp:374

=============================================================

Comment 1

[:Ehsan Akhgari (Out of office 5/10-5/21)]

Recent crasher regression, needs an owner.

Comment 2

[Olli Pettay [:smaug]]

clearly my stuff.

Comment 4

[Olli Pettay [:smaug]]

Attachment: shadow_subtree_root.diff

I'm investigating whether shadow DOM could easily use mSubtreeRoot, but since QA is seeing this crash while testing shadow DOM, better to fix to asap.

Comment 5

[Olli Pettay [:smaug]]

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f94384469cda2ef9d807729fb47160bc50f4ab2a
remote: recorded changegroup in replication log in 0.104s

Comment 6

[Marcia Knous [:marcia - needinfo? me]]

Added another signature.

Comment 7

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

Comment on attachment 8981827 [details] [diff] [review]
shadow_subtree_root.diff

Review of attachment 8981827 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsINode.cpp
@@ +300,5 @@
> +      // method, which isn't supposed to return null.
> +      // So, fallback to slow path.
> +      node = const_cast<nsINode*>(this);
> +      nsINode* iter = node;
> +      while ((iter = iter->GetParentNode())) {

It's small, but would you mind taking this loop out into a common function? It's duplicated exactly a few lines below. Even a local:

auto RootOfNode = [](nsINode* iter) { ... };

would get rid of the duplication.

Comment 8

[Olli Pettay [:smaug]]

it is not exactly the same below, but ok, let me do something like that.

Comment 9

[Olli Pettay [:smaug]]

haahaa, the NS_ASSERTION -> MOZ_ASSERT change revealed some other existing issue.

Comment 10

[Olli Pettay [:smaug]]

Attachment: v2

You can blame me on the bad review for the LabelsList.
MaybeResetRoot really needs to be called after nsStyledElement::UnbindFromTree so that SubtreeRoot points to the right place again. Currently mSubtreeRoot points to somewhere higher up in the tree, and parentNode chain is possibly already cut.

And there was an invalid wpt test for that too (coming from the initial .labels commit). If the label and control stay in the same tree, .labels should still contain the label element.
From the spec
"If the for attribute is not specified, but the label element has a labelable element descendant, then the first such descendant in tree order is the label element's labeled control."
So, no need to be in document or anything like that.


Looks like we've got couple of NS_ASSERTIONs from the .labels stuff.

Comment 11

[Olli Pettay [:smaug]]

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/2a0ee9606592126fb17c82136367eefa062964e4
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0ee9606592126fb17c82136367eefa062964e4
remote: recorded changegroup in replication log in 0.110s

Comment 12

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

Comment on attachment 8982060 [details] [diff] [review]
v2

Review of attachment 8982060 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsGenericHTMLElement.cpp
@@ +478,2 @@
>    // We need to consider a labels element is removed from tree,
>    // it needs to update labels list and its root as well.

Per IRC, please fix the grammar in this comment.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa94106d7aef
ensure SubtreeRoot always returns non-null value, r=mrbkap

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/aa94106d7aef

Comment 15

[Olli Pettay [:smaug]]

Timea, want to re-test https://bugzilla.mozilla.org/show_bug.cgi?id=1465357#c2 ?

Comment 16

[Olli Pettay [:smaug]]

I guess one may need to wait a bit to get new Nightly build.

Comment 17

[Timea Babos]

Hey everyone,

I tried to reproduce the issue based on the steps provided in Bug 1465357 without any success in reproducing it. 
Same goes for the second site that uses shadowDOM (https://wiredjs.com/), the issue could not be reproduced anymore on the latest Nightly 62.0a1 (2018-06-03) on Windows 10 x64.

Comment 18

[Timea Babos]

Just to be sure, are the two crash signatures caused by the same issue?
I'm asking in order to know if the way I verified the fix (as mentioned above)is enough or further testing is needed.

Comment 19

[Olli Pettay [:smaug]]

Based on the stack trace, they are about the same issue.

Thanks for testing!

Comment 20

[Timea Babos]

Based on Comment 17 and Comment 19 I will close this issue as Verified - Fixed.

Comment 21

[Web Platform Test Sync Bot]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11948 for changes under testing/web-platform/tests

Comment 22

[Web Platform Test Sync Bot]

Upstream PR merged