Child with null parent during unlinking

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment)

Apparently we're crashing regularly in some B2G test that adds and removes a bunch of DOM nodes. See bug 1165469 comment 754. From the stack, we unlink a document, which calls UnbindFromTree on a child which is an HTMLSharedElement, which calls UnbindFromTree on a child which is an HTMLSharedElement, which calls UnbindFromTree on a child that is an nsGenericDOMDataNode, and that child has a null mParent, causing us to crash on this check:
  if (aNullParent || !mParent->IsInShadowTree()) {

This is a regression from my patch that changes how the content unbinder is implemented (bug 866681). I'm assuming that a node that is a child should never have a null mParent? I'm not sure how this could be happening. I'll have to read over the content unbinder code again, and also figure out what my patch could have done to break this besides change when the ContentUnbinder runs. It is also strange that only this one B2G test is triggering it.

(I'm filing separate from bug 1165469 because that one is so spammy.)
FWIW, I don't see anything with UnbindFromTree in the signature in the top 300 crashes on Nightly.
I was looking at the code too, and mParent really shouldn't be null there.
And given that it is b2g my guess is that it might be something shadow DOM related.
(but even then, I don't understand how mParent could be null.)

I'll re-read the code tomorrow.
Wondering if you had the time to look at this in this post-Whistler week :)
Bug 1165469 has more than 1000 comments now :D
Assignee: nobody → continuation
Comment on attachment 8629019 [details] [diff] [review]
Back out most of 7fcf6bf43eda for causing Gaia unit test crashes.

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

I'm not sure what the failure rate is, but the try push looks quite green.

The one change here is that I didn't add back the content unbinder telemetry, because I delete that in another bug. I don't think it is a big deal as we didn't look at it too much.
Attachment #8629019 - Flags: review?(bugs)
And that telemetry was reporting rather small numbers.
Attachment #8629019 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Comment on attachment 8629019 [details] [diff] [review]
Back out most of 7fcf6bf43eda for causing Gaia unit test crashes.

Approval Request Comment
[Feature/regressing bug #]: bug 866681
[User impact if declined]: A gaia unit test is crashing very frequently due tos this (bug 1165469). It is also possible this could affect regular Firefox users, too, as there's nothing Gaia specific here.
[Describe test coverage new/current, TreeHerder]: this code is heavily tested
[Risks and why]: low, this just backs out a patch to restore existing behavior.
[String/UUID change made/needed]: none
Attachment #8629019 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/036eff95f08b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8629019 [details] [diff] [review]
Back out most of 7fcf6bf43eda for causing Gaia unit test crashes.

Approving for uplift as this is a patch backout.
Attachment #8629019 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
blocking-b2g: 2.2? → ---
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.