385845 - "ASSERTION: mDocElement not in doc?" if documentElement is removed during onload

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Jesse Ruderman]

Attachment: testcase that removes documentElement during onload

###!!! ASSERTION: mDocElement not in doc?: 'mDocument->IndexOf(mDocElement) != -1', file mozilla/content/xml/document/src/nsXMLContentSink.cpp, line 343

Comment 1

[Jesse Ruderman]

Attachment: testcase that removes documentElement from an inline script

This triggers the assertion, too.

Comment 2

[Eli Friedman]

It seems to me that the assertion is bogus; that guarantee simply doesn't exist because of scripts like this one.  (XML parsing is still consistent even if the document tree is being modified as it is parsed.)

Comment 3

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

Attachment: another testcase

Here's another testcase (XHTML this time).

Comment 4

[Phil Ringnalda (:philor)]

And http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1285965566.1285966438.9435.gz says that reftest/tests/layout/svg/crashtests/385840-1.svg is another testcase.

Comment 5

[Phil Ringnalda (:philor)]

Attachment: rm

I'm way out of my depth, but I think that the state of the code in 1.8,

317     if (mDocElement) {
318       // Notify document observers that all the content has been stuck
319       // into the document.
320       // XXX do we need to notify for things like PIs?  Or just the
321       // documentElement?
322       NS_ASSERTION(mDocument->IndexOf(mDocElement) != -1,
323                    "mDocElement not in doc?");
324 
325       mozAutoDocUpdate docUpdate(mDocument, UPDATE_CONTENT_MODEL, PR_TRUE);
326       mDocument->ContentInserted(nsnull, mDocElement,
327                                  // XXXbz is this last arg relevant if
328                                  // the container is null?
329                                  mDocument->IndexOf(mDocElement));
330     }

means that the assertion was related to notifying mDocument's observers about mDocElement, and not wanting to tell them about something that wasn't in their document, and so bogus or not it's no longer related to anything that's actually happening where it's being asserted.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 480355 [details] [diff] [review]
rm

r=me

Comment 7

[Phil Ringnalda (:philor)]

Comment on attachment 480355 [details] [diff] [review]
rm

Tempting to claim I can just push this, since it's an orange fix and practically speaking NPOToptB.

Comment 8

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/cc874004357b

Comment 9

[Phil Ringnalda (:philor)]

And http://hg.mozilla.org/mozilla-central/rev/891a83ede325 since I somehow failed to notice the annotation about how layout/xul/base/src/crashtests/321056-1.xhtml was yet another testcase for this.