331668 - nsGenericElement::doInsertChildAt doesn't check aParent when setting mutation.mRelatedNode

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Alex Vincent [:WeirdAl]]

After a fix for bug 201236 lands, we run into another problem.  Apparently, nsGenericElement::doInsertChildAt sets mRelatedNode to aParent.  In the case of nsDocument::InsertBefore() calls, aParent is null.  So DOMNodeInserted event listeners get evt.target as the inserted node, and evt.relatedNode == null.

nsGenericElement::doRemoveChildAt has the same bug.

The simple fix would be to replace aParent with container, as it is defined in doRemoveChildAt.

However, I wonder about a better solution.  Suppose we change aParent's type from nsIContent to nsINode.  We could NS_ENSURE_ARG_POINTER(aParent), then, and not change the latter code.  This however would require a more complex patch across the board.

bz, sicking, smaug, what do you think?  If you prefer the simple fix, I can whip up a patch for quick r+sr.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Suppose we change aParent's type from nsIContent to nsINode.

I considered that, but then you need some other way to flag whether you're working with a kid of the document and I suspect the code will actually end up more complicated than what we have now.  Feel free to prove me wrong, of course -- if we can not complicate the code and have aParent be the parent node, so much the better.

If not, then just using container for the related node looks correct to me.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

I'm not that adventurous.  :)  I think this patch is good enough, though I'd accept a follow-up bug to investigate the nsINode switch.

peterv:  I'm requesting r+sr from you because I've requested sr from you on the latest patch for bug 201236.  The two patches touch similar code, so it's simpler this way.  The drawback is that the two patches conflict:  checking in one patch will bitrot the other.  We'll cross that bridge when we come to it, I figure, and I want to give you the simplest path to review.

Comment 4

[Alex Vincent [:WeirdAl]]

Also, this patch is less than ten lines of changes.  :)

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 216193 [details] [diff] [review]
patch, v1

>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.458
>diff -p -u -8 -r3.458 nsGenericElement.cpp
>--- content/base/src/nsGenericElement.cpp	24 Mar 2006 22:42:54 -0000	3.458
>+++ content/base/src/nsGenericElement.cpp	25 Mar 2006 07:12:31 -0000
>@@ -2344,17 +2344,21 @@ nsGenericElement::doInsertChildAt(nsICon
>       }
>     }
> 
>     // XXXbz how come we're not firing mutation listeners for adding to
>     // documents?
>     if (aParent &&
>         HasMutationListeners(aParent, NS_EVENT_BITS_MUTATION_NODEINSERTED)) {
>       nsMutationEvent mutation(PR_TRUE, NS_MUTATION_NODEINSERTED);
>-      mutation.mRelatedNode = do_QueryInterface(aParent);
>+      nsINode* container = aParent;
>+      if (!container) {

This can't happen since the if above checks for aParent, so I doubt this fixes the bug.

Comment 6

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

I would suggest waiting with fixing this until I've fixed bug 90983, otherwise you're running the risk of regressing Tp. When I fix that bug I could fix this one with it. I'll do that pretty soon.

Comment 7

[Alex Vincent [:WeirdAl]]

(In reply to comment #5)
> >+      nsINode* container = aParent;
> >+      if (!container) {
> 
> This can't happen since the if above checks for aParent, so I doubt this fixes
> the bug.

*sigh* As I said, this patch and the work I've done for bug 201236 were fairly closely related.  The context of the other bug's latest patch said:

     }
-
-    // XXXbz how come we're not firing mutation listeners for adding to
-    // documents?
-    if (aParent &&
-        HasMutationListeners(aParent, NS_EVENT_BITS_MUTATION_NODEINSERTED)) {
+    PRBool hasListeners =
+      aDocument &&
+      nsContentUtils::HasMutationListeners(aParent,
+                                           aDocument,
+                                           NS_EVENT_BITS_MUTATION_NODEINSERTED);
+    if (hasListeners) {
       nsMutationEvent mutation(PR_TRUE, NS_MUTATION_NODEINSERTED); 

So your objection is not terribly useful.  There's a reason I marked this bug as dependent on bug 201236.  :)

Comment 8

[Alex Vincent [:WeirdAl]]

sicking: peterv is correct in his claim that the change I proposed does nothing... until my standalone document complex is fixed.  As peterv has r-'d this patch, the ghost in this shell suggests waiting.

Comment 9

[Alex Vincent [:WeirdAl]]

Reading over sicking's patch for bug 90983, I'm fairly satisfied his solution will fix this.  Reassigning to him.

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: Set related node to container.

*sigh* Reference bug 90983 comment 22.

sicking: This is a one-line patch we previously discussed.  If you grant r/sr, please check this in for me.  :)

Comment 11

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

Sorry, i forgot to push submit on my review.

Comment 12

[Robert Sayre]

Checked in to trunk.

/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.cpp
new revision: 3.483; previous revision: 3.482
done

Comment 13

[Alex Vincent [:WeirdAl]]

Boo-yah.