Crash in mozilla::ShouldClearTargets

VERIFIED FIXED in Firefox 62

Status

()

defect
P1
critical
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: marcia, Assigned: smaug)

Tracking

({crash, regression})

Trunk
mozilla62
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 unaffected, firefox62 verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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

=============================================================
Recent crasher regression, needs an owner.
Flags: needinfo?(overholt)
Priority: -- → P1
(Assignee)

Comment 2

a year ago
clearly my stuff.
Assignee: nobody → bugs
Flags: needinfo?(overholt)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1465357
(Assignee)

Comment 4

a year ago
Posted patch shadow_subtree_root.diff (obsolete) — Splinter Review
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.
Attachment #8981827 - Flags: review?(mrbkap)
(Assignee)

Comment 5

a year ago
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
Added another signature.
Crash Signature: [@ mozilla::ShouldClearTargets] → [@ mozilla::ShouldClearTargets] [@ static bool mozilla::ShouldClearTargets]
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.
Attachment #8981827 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

a year ago
it is not exactly the same below, but ok, let me do something like that.
(Assignee)

Comment 9

a year ago
haahaa, the NS_ASSERTION -> MOZ_ASSERT change revealed some other existing issue.
(Assignee)

Comment 10

a year ago
Posted patch v2Splinter Review
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.
Attachment #8981827 - Attachment is obsolete: true
Attachment #8982060 - Flags: review?(mrbkap)
(Assignee)

Comment 11

a year ago
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 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.
Attachment #8982060 - Flags: review?(mrbkap) → review+

Comment 13

a year ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa94106d7aef
ensure SubtreeRoot always returns non-null value, r=mrbkap

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aa94106d7aef
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Comment 15

a year ago
Timea, want to re-test https://bugzilla.mozilla.org/show_bug.cgi?id=1465357#c2 ?
Flags: needinfo?(timea.babos)
(Assignee)

Comment 16

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

Comment 17

a year ago
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.
Flags: needinfo?(timea.babos)

Comment 18

a year ago
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 19

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

Thanks for testing!
Flags: needinfo?(bugs)

Comment 20

a year ago
Based on Comment 17 and Comment 19 I will close this issue as Verified - Fixed.
Status: RESOLVED → VERIFIED
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11948 for changes under testing/web-platform/tests
Upstream PR merged
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.