382636 - XMLDocument.cloneNode(true) triggers "ASSERTION: Could not get loadgroup; onload may fire too early" for each image in the document

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Could not get loadgroup; onload may fire too early: 'loadGroup', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 2127

The document has to be .xml (not .xhtml).  The number of times the assertion fires is equal to the number of images in the document.

Comment 1

[Jesse Ruderman]

If I use an image with a javascript: URL, I also get:

###!!! ASSERTION: Unable to get an nsIScriptGlobalObjectOwner from the channel!: 'globalOwner', file /Users/jruderman/trunk/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp, line 140

Comment 2

[Boris Zbarsky [:bzbarsky]]

So...  We need to either set the container (as we do for documents created via createDocument()) or set the document as loaded as data for the clone.  We're trying to do network loads off of it, which is a really bad idea...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking blocking for now to keep it on my list. Will punt it if I run out of time though as this doesn't seem very critical.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er...  don't we have the data document content policy for a reason?  With this bug, if someone clones a data document, we'll do the loads, which seems like a bad thing...  I think we should mark the clone as loaded as data, myself.

Comment 5

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

Attachment: possible patch

I think this is what we want; if we're cloning a "data" document, then
mark the clone to be "data" too, and always set the container to be
the same as in the original document.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do you really want to set the container to be the same? Isn't the container only supposed to be set when the document lives inside a docshell?

Comment 7

[Boris Zbarsky [:bzbarsky]]

In general, yes.  But not for documents created via createDocument off a DOMImplementation.  See bug 325818.  Do the same considerations about cx apply here?  Or can we just fix that bug instead of perpetuating the hack?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would say that the same considerations apply. IMHO documents created via createDocument and documents created via cloneNode are more or less equivalent as far as behavior goes.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sounds like Smaug's copying the container is the right thing to do, if we can't just fix bug 325818.  Can we?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278032 [details] [diff] [review]
possible patch

>Index: content/base/public/nsIDocument.h
>+  void SetLoadedAsData(PRBool aLoadedAsData)

I don't really like having this mutable.  Can we make this a (protected?) method on nsXMLDocument and add an arg to NS_NewDOMDocument?

And let's get the container thing sorted out.  I'd really rather not give non-docshell docs containers if we can at all avoid it...

Comment 11

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

Comment on attachment 278032 [details] [diff] [review]
possible patch

This is wrong. And createDocument does bad too. Or at least something
must be done to nsDocument::PreHandleEvent. We don't want to propagate events from data documents to |window|, I think.

Comment 12

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

Or should change nsDocument::GetScriptGlobalObject().
Removing nsDocument::Destroy will do that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Or we can remove the container thing and see whether the problem that prompted adding it is there... I would vote for that.

Comment 14

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

Fixed in Bug 393762.

Comment 15

[Jesse Ruderman]

Crashtest checked in.

Comment 16

[Daniel Veditz [:dveditz]]

This was fixed in 1.8.1.12 as part of bug 393762, then backed out because of regression bug 414856. Addons can work around it on the trunk, we don't need to back this out except on branch for compatibility reasons.