Closed Bug 382636 Opened 13 years ago Closed 13 years ago

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


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

Not set





(Reporter: jruderman, Assigned: smaug)


(Blocks 1 open bug)


(Keywords: assertion, testcase)


(1 file, 1 obsolete file)

Attached file 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.
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
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...
Flags: blocking1.9?
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.
Assignee: nobody → jonas
Flags: blocking1.9? → blocking1.9+
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.
Assignee: jonas → Olli.Pettay
Attached patch possible patch (obsolete) — Splinter Review
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.
Attachment #278032 - Flags: superreview?(bzbarsky)
Attachment #278032 - Flags: review?(bzbarsky)
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?
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?
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.
Sounds like Smaug's copying the container is the right thing to do, if we can't just fix bug 325818.  Can we?
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 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.
Attachment #278032 - Attachment is obsolete: true
Attachment #278032 - Flags: superreview?(bzbarsky)
Attachment #278032 - Flags: review?(bzbarsky)
Or should change nsDocument::GetScriptGlobalObject().
Removing nsDocument::Destroy will do that.
Or we can remove the container thing and see whether the problem that prompted adding it is there... I would vote for that.
Fixed in Bug 393762.
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 368573
Flags: in-testsuite?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Depends on: 393762
Depends on: 414856
This was fixed in 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.
Flags: wanted1.8.1.x-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.