563980 - random "ASSERTION: Please remove this from the document properly" during crashtest

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The most common random assertion remaining during crashtest seems to be occurrences of "ASSERTION: Please remove this from the document properly".  There's on existing bug on this, bug 475132, but I'm filing this one separately for investigation of these random assertions.

I plan to land some debugging code to print information about the elements with this problem in the hopes that it will help us figure out the problem.

I suspect it could be due to a cycle collection unlink method that fails to call UnbindFromTree.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I landed some debugging code for this:
http://hg.mozilla.org/mozilla-central/rev/76e0ed39f6ad

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: output from logs so far

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

My guess is that we're hitting one of the early returns (other than the first) in nsHTMLEditor::HideResizers / nsHTMLEditor::HideGrabber / nsHTMLEditor::HideInlineTableEditingUI ; likely either (a) the null-check on the pres shell or (b) the parent content / root (because nsEditor's Unlink clears mRootElement without calling the anonymous content clearing in ~nsHTMLEditor).

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Additional debugging code:
http://hg.mozilla.org/mozilla-central/rev/4498b6800a49

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It seems to be the no pres shell case.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

And it looks like the code is already able to handle a null pres shell so we should just be able to stop null-checking it.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

I think this should be fine; I just pushed it to try to make sure it doesn't do anything bad.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Some of the other early returns are also potentially scary, but they don't seem to be the present problem...)

Comment 9

[(no longer active)]

Comment on attachment 443763 [details] [diff] [review]
patch

Makes sense to me.

Have you tried attachment 358544 [details] with this patch?

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I couldn't reproduce this assertion on that attachment without the patch... so I hadn't.  But I checked that I still see the other three assertions even with this patch (though not all the time).

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Debugging code backed out:
http://hg.mozilla.org/mozilla-central/rev/166e9ae1bb5d
and patch landed:
http://hg.mozilla.org/mozilla-central/rev/41ffa62580c4