548463 - Disallow adopting node into a different document from adoptNode

Status: RESOLVED FIXED

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

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Alex Vincent [:WeirdAl]]

Pardon my ignorance, but why would we do this?  I was under the impression adoptNode was the correct - in fact, the only correct - way to move a node from one document to another.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Alex, this bug is about disallowing adoptNode calls from inside userdata handlers called during the process of adopting a node.  So doing:

  doc1.adoptNode(n);

and then during the callbacks adoptNode is specified to make doing:

  doc2.adoptNode(n);

There's no sane way to support this (and especially, no sane way to return success from the initial adoptNode call; a successful return would be a lie, since the node is not in fact in doc1).

Comment 4

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

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

It would have been nice with a bit more context in the patch, given how complicated this code is.

>@@ -3858,15 +3863,19 @@ nsGenericElement::doReplaceOrInsertBefor
>       newContent->RemoveChildAt(--i, PR_TRUE);
>       mutated = mutated || guard.Mutated(1);
>     }
> 
>-    // If we've had any unexpeted mutations so far we need to recheck that
>-    // the child can still be inserted.
>-    if (mutated) {
>-      for (i = 0; i < count; ++i) {
>-        // Get the n:th child from the array.
>-        nsIContent* childContent = fragChildren[i];
>-
>+    for (i = 0; i < count; ++i) {
>+      // Get the n:th child from the array.
>+      nsIContent* childContent = fragChildren[i];
>+      if (!container->HasSameOwnerDoc(childContent) ||
>+          doc != container->GetOwnerDoc()) {
>+        return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
>+      }
>+
>+      // If we've had any unexpected mutations so far we need to recheck that
>+      // the child can still be inserted.
>+      if (mutated) {

Would it work to make AdoptNode call nsMutationGuard::DidMutate and keep the for-loop inside the |if (mutated)|. That should keep this fast in the common case, and might be useful elsewhere too.

It looks like you need to add the same test to the case when we're not adding a fragment. I.e. inside here:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#3972

Also, the fact that doctype nodes are adopted inside InsertChildAt scares me. First of all it seems like that will result in userdata handlers being called while there are script blockers. I think just removing the special cases for doctypes from here
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#3770
and here
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#3265
will do the trick.

Looks good otherwise, but would like to see another patch.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> Would it work to make AdoptNode call nsMutationGuard::DidMutate and keep the
> for-loop inside the |if (mutated)|. That should keep this fast in the common
> case, and might be useful elsewhere too.

I'll try that, yes.

> It looks like you need to add the same test to the case when we're not adding a
> fragment. I.e. inside here:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#3972

I don't see why? You can't use the DOMNodeRemoved event to adopt the node being removed, because the actual removal will fail and throw (trying to remove a child from the childlist that's already been removed by adoptNode). The point with fragments is that you can use the DOMNodeRemoved event for one of the fragment's children to adopt one of its other children.

> Also, the fact that doctype nodes are adopted inside InsertChildAt scares me.

I'm not really sure why doctype nodes worry you but other types not? Especially since adopting doctype nodes will actually fail. Also, I wonder who reviewed that :-).

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

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

(In reply to comment #5)
> > It looks like you need to add the same test to the case when we're not
> > adding a fragment. I.e. inside here:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#3972
> 
> I don't see why? You can't use the DOMNodeRemoved event to adopt the node
> being removed, because the actual removal will fail and throw (trying to
> remove a child from the childlist that's already been removed by adoptNode).

What if you adopt an ancestor of the node that is being removed?

> > Also, the fact that doctype nodes are adopted inside InsertChildAt scares
> > me.
> I'm not really sure why doctype nodes worry you but other types not?

Hmm.. I think I was worried because all other nodes should already be adopted and so we'll do nothing.

> Especially
> since adopting doctype nodes will actually fail. Also, I wonder who reviewed
> that :-).

Yeah, I think I wasn't considering the fact that adopting could run script when I reviewed this. Maybe.

I wonder if we should run the adoption notifications asynchronously?

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Comment 9

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

Comment on attachment 432846 [details] [diff] [review]
v1.2

Looks great!

Only thing I'm somewhat wondering is if we should always call nsMutationGuard::DidMutate, even on failure. 

Just in case someone down the line forgets to check the return value from Adopt.

Calling DidMutate too much should just be a perf issue. Calling it too little might be a security issue.

r=me in any event.

Comment 10

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/d4c024a0cfc1
http://hg.mozilla.org/mozilla-central/rev/5bbaec69228a