1177627 - Child with null parent during unlinking

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.)

Comment 1

[Andrew McCreight [:mccr8]]

FWIW, I don't see anything with UnbindFromTree in the signature in the top 300 crashes on Nightly.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Julien Wajsberg [:julienw]]

Wondering if you had the time to look at this in this post-Whistler week :)
Bug 1165469 has more than 1000 comments now :D

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Back out most of 7fcf6bf43eda for causing Gaia unit test crashes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c452e64e1d4

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And that telemetry was reporting rather small numbers.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/036eff95f08b

Comment 8

[Andrew McCreight [:mccr8]]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/036eff95f08b

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdfc95cad121