251025 - document.importNode does not set the right owner document.

Status: RESOLVED FIXED

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

Description

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

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

document.importNode does not set the right owner document.
It just clones the nodes.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Ed Anuff]

Attachment: Testcase xul file

Comment 2

[Ed Anuff]

Attachment: Testcase script file

Comment 3

[Ed Anuff]

I've just uploaded a test case to demonstrate the problem.  When it's run as 
chrome, clicking on the second listitem (the one added by the javascript 
function), causes the following javascript errors to be reported:

Error: uncaught exception: Permission denied to get property Event.ctrlKey
Error: uncaught exception: Permission denied to get property Event.button

Interestingly, if not run as chrome, it works.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Testcase

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

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

Are you adding nsIContent::CloneContent just so that you are able to specify a
new document? Or does it have other advantages too, like stuff that userdata needs?

The reason i'm asking is that we need to figure out some way to implement
adoptNode which means that we have to be able to change ownerdoc for a node. So
maybe we could make importNode do a clone and then call whatever function
adoptNode will use?

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> Are you adding nsIContent::CloneContent just so that you are able to specify a
> new document? Or does it have other advantages too, like stuff that userdata
needs?

I'm adding CloneContent because we need a way to clone a node that isn't the DOM
cloneNode method. We don't want importNode to call the userDataHandler with
NODE_CLONED, which cloneNode will do.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181126 [details] [diff] [review]
v1

>Index: content/base/public/nsIAttribute.h

>+  nsIDocument *GetOwnerDoc()

Should this be a const method?

> Index: content/base/public/nsIContent.h

> +  virtual nsresult CloneContent(nsIDocument *aOwnerDocument,

Should this be const?

>Index: content/base/src/nsDocument.cpp
>+      nsINodeInfo *nodeInfo;
>+      nsCOMPtr<nsINodeInfo> newNodeInfo;
>+      if (document == this) {
>+        nodeInfo = attr->NodeInfo();
>+      }
>+      else {
>+        rv = mNodeInfoManager->GetNodeInfo(nodeInfo->NameAtom(),

You never set nodeInfo in the else clause, but you're calling methods on it.  I
think you wanted to set nodeInfo to attr->NodeInfo() no matter what, then
create a newNodeInfo if document != this.

>+      nsCOMPtr<nsIContent> imported = do_QueryInterface(aImportedNode);

Are you really guaranteed that this will be non-null?  Can't random JSObjects
be passed in for aImportedNode?  If we're guaranteed this is ok, why did we
null-check after QI to nsIAttribute?

Do we want to add an explicit case for the node types we don't support
importing and then have an assert in a DEFAULT block?  Or maybe just a warning
(and make the explicit case #ifdef DEBUG or something).

>Index: content/base/src/nsDocumentFragment.cpp
>+  NS_IMETHOD    GetOwnerDocument(nsIDOMDocument** aOwnerDocument)
>+  { return nsGenericElement::GetOwnerDocument(aOwnerDocument); }

Put that body on separate lines from the braces?  Would be a lot easier to
read...

>@@ -163,9 +164,8 @@ public:
>+  nsresult Clone(nsINodeInfo *aNodeInfo,  PRBool aDeep, nsIContent 

Extra space after first comma?

Also, this method doesn't seem to be implemented anywhere in this class... am I
just missing something?

>Index: content/base/src/nsGenericDOMDataNode.h

>+  virtual nsGenericDOMDataNode *Clone(nsIDocument *aOwnerDocument,
>+                                      PRBool aCloneText)

Should that be a const method?

The new methods in nsGenericElement.h could use some documenting as to what
they do...

More later.. have to run now.

Comment 9

[Peter Van der Beken [:peterv]]

> >Index: content/base/src/nsDocumentFragment.cpp
> >+  NS_IMETHOD    GetOwnerDocument(nsIDOMDocument** aOwnerDocument)
> >+  { return nsGenericElement::GetOwnerDocument(aOwnerDocument); }
> 
> Put that body on separate lines from the braces?  Would be a lot easier to
> read...

I'm trying to follow the style in the rest of the file.

> >@@ -163,9 +164,8 @@ public:
> >+  nsresult Clone(nsINodeInfo *aNodeInfo,  PRBool aDeep, nsIContent 

> Also, this method doesn't seem to be implemented anywhere in this class... am I
> just missing something?

It's implemented through NS_IMPL_DOM_CLONENODE.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181126 [details] [diff] [review]
v1

You need to change the nsIContent IID.

>Index: content/base/src/nsGenericDOMDataNode.h

Document the interaction of CloneNode/Clone/CloneContent here?

>Index: content/base/src/nsGenericElement.cpp

>+      rv = CloneChildrenTo(aDst);

>+      rv = ImportChildrenTo(aDst, domDoc);

I guess the only reason for CloneChildrenTo is to avoid getting the document
and having to make a method call on the document.  Because it seems to me that
importing into the current document (when cloning) should be pretty identical
to just cloning, right?

I guess I'm not sure why you're using importNode() and cloneNode() inside
ImportChildrenTo/CloneChildrenTo instead of just using nsIContent::CloneContent
and not having to worry about having two separate methods, having to QI, etc...

>+nsGenericElement::CloneContent(nsIDocument *aOwnerDocument, PRBool aDeep,
>+                               nsIContent **aResult)

>+  nsNodeInfoManager* nodeInfoManager = aOwnerDocument->NodeInfoManager();

What if aOwnerDocument is null?  After all, content nodes only hold weak refs
to the document through the nodeinfo manager, no?  So they could in fact
outlive the document, and then their GetOwnerDoc() would be null...  I guess
the only way we can get into that case is if we're cloning such a node, and
then GetOwnerDoc() == aOwnerDocument will test true, eh?

Nevertheless, it may be worth documenting on nsIContent that the document
passed to cloneContent must be non-null.  Not sure what we can do to enforce
that in this code, though, and we can't even assert it.  :(

>Index: content/base/src/nsGenericElement.h

>+  nsresult CloneContent(nsIDocument *aOwnerDocument, PRBool aDeep,
>+                        nsIContent **aResult);
>+  nsresult ImportChildrenTo(nsGenericElement *aDst,
>+                            nsIDOMDocument *aImportDocument);
>+  nsresult CloneChildrenTo(nsGenericElement *aDst);
>+  virtual nsresult Clone(nsINodeInfo *aNodeInfo,  PRBool aDeep,
>+                         nsIContent **aResult)
>+  nsresult CloneNode(nsIDOMNode *aSource, PRBool aDeep, nsIDOMNode **aResult);

Document the relationships between all these methods?  And document what
Clone() is expected to do in subclasses?

>Index: content/base/src/nsTextNode.cpp
>@@ -311,8 +305,9 @@ NS_NewAttributeContent(PRInt32 aNameSpac

>+  // XXX Need ownerDocument.

We could pass one in.  There's only one caller of this method....  Either here
or followup bug, either way (though I guess this needs to wait on your
ownerDocument changes).

>Index: content/html/content/src/nsHTMLSpanElement.cpp

>-NS_IMPL_DOM_CLONENODE(nsHTMLUnknownElement)

I guess doing this means we don't need to implement CloneNode(), but it seems
like the brainprint savings of using the macro here anyway are worth it...
maybe it's just me.

>Index: content/xul/content/src/nsXULElement.cpp

I'm assuming the only difference between the old and new Create() is what it
does with the nodeinfo?  If so, is it really correct to use a prototype for
document A when importing into document B?  That seems quite odd to me.... 
It'd make more sense to create a prototype-less element in that case and copy
all the stuff we need off the prototype...

>Index: content/xul/content/src/nsXULElement.h

>-    nsXULPrototypeElement*              mPrototype;
>+    nsRefPtr<nsXULPrototypeElement> mPrototype;

Leave that lined up like it was, please.

I'd like to take another look once you fix these comments and merge this to
tip....

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> >Index: content/base/src/nsGenericElement.cpp
> 
> >+      rv = CloneChildrenTo(aDst);
> 
> >+      rv = ImportChildrenTo(aDst, domDoc);
> 
> I guess the only reason for CloneChildrenTo is to avoid getting the document
> and having to make a method call on the document.  Because it seems to me that
> importing into the current document (when cloning) should be pretty identical
> to just cloning, right?

Actually, the reason is that once we implement DOM Level 3 UserData, cloning
will be different from importing (we need to send a different notification).

Comment 12

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

Having a prototype from a 'different' document is IMHO just fine since
prototypes aren't really connected to a document anyway (it's impossible to get
from protoelement to document). And moving nodes between documents has kept the
prototype for a long time.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1.1

> >+nsGenericElement::CloneContent(nsIDocument *aOwnerDocument, PRBool aDeep,
> >+				   nsIContent **aResult)
> 
> >+  nsNodeInfoManager* nodeInfoManager = aOwnerDocument->NodeInfoManager();
> 
> What if aOwnerDocument is null?  After all, content nodes only hold weak refs

> to the document through the nodeinfo manager, no?  So they could in fact
> outlive the document, and then their GetOwnerDoc() would be null...  I guess
> the only way we can get into that case is if we're cloning such a node, and
> then GetOwnerDoc() == aOwnerDocument will test true, eh?

Yes.

> Nevertheless, it may be worth documenting on nsIContent that the document
> passed to cloneContent must be non-null.  Not sure what we can do to enforce
> that in this code, though, and we can't even assert it.  :(

I added a comment. We could assert on (GetOwnerDoc() == aOwnerDocument ||
aOwnerDocument) but I'm not sure how helpful that is.

> I'm assuming the only difference between the old and new Create() is what it
> does with the nodeinfo?  If so, is it really correct to use a prototype for
> document A when importing into document B?  That seems quite odd to me.... 
> It'd make more sense to create a prototype-less element in that case and copy

> all the stuff we need off the prototype...

What sicking said, I'm not sure that this is a problem. We could file a
follow-up bug for this if you want.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I might well not be able to get to this before the 19th, which means not until I
get back in July... I'll see what I can do, though.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'm going to try to get to this in the next few days.

Comment 16

[Ed Anuff]

Attachment: JavaScript based workaround

I'm not sure if this helps, since the actual fix would be in the C-code, but
this was the workaround that I used to acheived the desired results or
importning parsed xul nodes into the current xul document.  It's just a deep
clone implement in JS that uses the create methods of the target document to
create the new nodes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 186099 [details] [diff] [review]
v1.1

>Index: content/base/public/nsIAttribute.h
>+  nsIDocument *GetOwnerDoc()

Const method, please.

sr=bzbarsky.  Looks great!

Comment 18

[Alexey Gaidukov]

I tried to compile seamonkey (13.07.2005) with the patch
https://bugzilla.mozilla.org/attachment.cgi?id=186099. I get error

 nsHTMLInputElement.cpp:407: error: no matching function for call to
‘nsHTMLInputElement::GetValue(nsAutoString&) const’
nsHTMLInputElement.cpp:158: note: candidates are: virtual nsresult
nsHTMLInputElement::GetValue(nsAString_internal&) <near match>
nsHTMLInputElement.cpp:418: error: no matching function for call to
‘nsHTMLInputElement::GetChecked(PRBool*) const’
nsHTMLInputElement.cpp:158: note: candidates are: virtual nsresult
nsHTMLInputElement::GetChecked(PRBool*) <near match>
gmake[1]: *** [nsHTMLInputElement.o] Error 1

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 186099 [details] [diff] [review]
v1.1

Sicking's MIA.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 186099 [details] [diff] [review]
v1.1

r=jst

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Unfortunately I had to make Clone and CloneContent non-const because some
implementations call non-const methods. nsHTMLInputElement::Clone calls
nsHTMLInputElement::GetValue and nsHTMLInputElement::GetChecked.
nsXMLProcessingInstruction::Clone and nsXMLStylesheetPI::Clone call
nsXMLProcessingInstruction::GetData. nsXTFElementWrapper::Clone passes |this|
to nsXTFElementWrapper::CloneState(nsIDOMElement*). I tried making the called
methods const, but it is not trivial.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

you could use NS_CONST_CAST in the interim, maybe...

shouldn't nsIAttribute::GetOwnerDoc be const?

+  virtual nsGenericDOMDataNode *Clone(nsIDocument *aOwnerDocument,
+                                      PRBool aCloneText)
+  {
+    NS_ERROR("This shouldn't be called!");

couldn't you make it pure virtual, then?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, if we're calling methods that really should be const but just happen not
to be, I'd NS_CONST_CAST(type, this) as needed.

Comment 24

[Peter Van der Beken [:peterv]]

(In reply to comment #22)
> shouldn't nsIAttribute::GetOwnerDoc be const?

Actually, there was already a const one, checked in as part of bug 285597.
Removed the non-const one.

> couldn't you make it pure virtual, then?

Yeah, done.

(In reply to comment #23)
> Yeah, if we're calling methods that really should be const but just happen not
> to be, I'd NS_CONST_CAST(type, this) as needed.

Ok, I'll check in a patch that switches back to const and NS_CONST_CASTs for the
calls to nsHTMLInputElement::GetValue, nsHTMLInputElement::GetChecked and
nsXTFElementWrapper::CloneState. I'll make nsXMLProcessingInstruction::Clone and
nsXMLStylesheetPI::Clone call nsGenericDOMDataNode::GetData which I just made
const instead of nsXMLProcessingInstruction::GetData which is the DOM method but
just forwards to nsGenericDOMDataNode::GetData.

Comment 25

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

It looks like this may have turned balsa (on Firefox tinderbox) orange
(assertions are fatal on that build, so they turn the build orange).

Comment 26

[Boris Zbarsky [:bzbarsky]]

So the assert happens because NS_NewAttributeTextNode actually sets the
mDocument.  It should probably be doing what nsGenericDOMDataNode::CloneContent
does, which is more like this:

+  // XXX We really want to pass the document to the constructor, but can't
+  //     yet. See https://bugzilla.mozilla.org/show_bug.cgi?id=27382

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 195709 [details] [diff] [review]
Something like this

I'll check this in, thanks.

Comment 29

[Peter Van der Beken [:peterv]]

Bz's patch checked in, made Clone/CloneNode const again.
balsa's still orange, but that seems to come from bug 307939.