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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 266776 [details]
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.
(Reporter)

Comment 1

11 years ago
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)

Updated

11 years ago
Assignee: jonas → Olli.Pettay
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Created attachment 278032 [details] [diff] [review]
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 368573
Flags: in-testsuite?
(Reporter)

Comment 15

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Depends on: 393762
Depends on: 414856
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.
Flags: wanted1.8.1.x-
You need to log in before you can comment on or make changes to this bug.