Closed Bug 1462746 Opened 7 years ago Closed 7 years ago

Crash in mozilla::ShouldClearTargets

Categories

(Core :: DOM: Core & HTML, defect, P1)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified

People

(Reporter: marcia, Assigned: smaug)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
clearly my stuff.
Assignee: nobody → bugs
Flags: needinfo?(overholt)
Attached 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)
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+
it is not exactly the same below, but ok, let me do something like that.
haahaa, the NS_ASSERTION -> MOZ_ASSERT change revealed some other existing issue.
Attached 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)
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+
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa94106d7aef ensure SubtreeRoot always returns non-null value, r=mrbkap
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I guess one may need to wait a bit to get new Nightly build.
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)
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)
Based on the stack trace, they are about the same issue. Thanks for testing!
Flags: needinfo?(bugs)
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: