Closed
Bug 1462746
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::ShouldClearTargets
Categories
(Core :: DOM: Core & HTML, defect, P1)
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)
9.93 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Recent crasher regression, needs an owner.
Flags: needinfo?(overholt)
Priority: -- → P1
Updated•7 years ago
|
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
tracking-firefox62:
--- → +
Assignee | ||
Comment 4•7 years ago
|
||
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•7 years 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
Reporter | ||
Comment 6•7 years ago
|
||
Added another signature.
Crash Signature: [@ mozilla::ShouldClearTargets] → [@ mozilla::ShouldClearTargets]
[@ static bool mozilla::ShouldClearTargets]
Comment 7•7 years ago
|
||
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•7 years ago
|
||
it is not exactly the same below, but ok, let me do something like that.
Assignee | ||
Comment 9•7 years ago
|
||
haahaa, the NS_ASSERTION -> MOZ_ASSERT change revealed some other existing issue.
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years 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 12•7 years ago
|
||
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•7 years 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 15•7 years ago
|
||
Timea, want to re-test https://bugzilla.mozilla.org/show_bug.cgi?id=1465357#c2 ?
Flags: needinfo?(timea.babos)
Assignee | ||
Comment 16•7 years ago
|
||
I guess one may need to wait a bit to get new Nightly build.
Comment 17•7 years 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•7 years 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•7 years ago
|
||
Based on the stack trace, they are about the same issue.
Thanks for testing!
Flags: needinfo?(bugs)
Comment 20•7 years ago
|
||
Based on Comment 17 and Comment 19 I will close this issue as Verified - Fixed.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11948 for changes under testing/web-platform/tests
Upstream PR merged
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•