delza Reporter
	Description • 21 years ago

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030312

When I create a new XML document in Javascript, which has no associated window
for display, Document.createEvent("MutationEvents") returns null,
Node.dispatchEvent(evt) (with an event created from HTML page document) does not
trigger handlers, and actually mutating the page via DOM methods does not
trigger handlers.

I can understand not firing UI events for "headless" (no UI) XML documents, but
Mutation events should still be fired.  It's a model/view thing.

Reproducible: Always

Steps to Reproduce:
1.  Create a new document: document.implementation.createDocument('','',null)
2.  Subscribe to any mutation event
3.  Call DOM methods to modify document

Actual Results:  
Event handlers are never fired.

Expected Results:  
I should get events in the handlers.  I've tested the exact same code by
subscribing to mutation events on the (visible) HTML DOM and they fire just fine.

	delza Reporter
	Comment 1 • 21 years ago

Just some sample code to demonstrate what I'm trying to do.

	Greg K.
	Comment 2 • 21 years ago

delza@alliances.org, what should I see on loading that testcase?

	delza Reporter
	Comment 3 • 21 years ago

The test case creates a new XML document, subscribes to all mutation events on
that document (the handler simply writes events as new itesm of the UL list or
as alerts, depending on configuration), then modifies the document by creating a
new node and inserting it.  Finally, the XML of the resulting document is
printed in the UL to confirm the mutations.  So what you *should* see is events
being printed out.  What you'll actually see is nothing, which makes it kind of
a anticlimatic test case.

If you have suggestions for a better test case, I'll try to do that.

	Martin Honnen
	Comment 5 • 21 years ago

This test case loads an example XML file
  http://bugzilla.mozilla.org/attachment.cgi?id=121269&action=view
into an iframe inside the test page and into memory by using XMLHttpRequest.
A mutation event listener is added and then a node is inserted. For the XML
document loaded into the iframe the mutation event listener fires while for the
XML document loaded into memory the event listener is not fired.
The debug output shows that for both documents the node is inserted thus the
mutation event listener should fire in both cases.

	Martin Honnen
	Comment 6 • 21 years ago

As I find the same problem like the reporter I confirm the bug. Also changed
summary to describe the problem more precisely.
I have tested with Mozilla 1.4a (Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a)
Gecko/20030401) but the problem also occurs with Netscape 7.02 (Mozilla/5.0
(Windows; U; Win98; en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02) thus this is
not a regression.

	delza Reporter
	Comment 7 • 21 years ago

So there are two basic ways to get a new XML document (that I'm aware of, modulo
some variation) in Mozilla: DOMImplementation.createDocument() and creating a
new iFrame element and loading the document into it.

In the case of createDocument, I do not receive mutateEvents when the document
is changed, or when I create a mutateEvent and call dispatchEvent (note that I
can subscribe to the events and call dispatchEvent with no errors, but it
silently fails).  On the other hand, I can add new properties using
Node.prototype, set the properties, and view the properties when I retrieve the
node from the DOM tree.

In the case of the iFrame document, I can receive mutateEvents whether they
originate from actual changes or dispatchEvent, and I can set properties on an
individual node and retrieve them as long as I keep a pointer to the node, but
when I retrieve the node from the DOM tree the property no longer available. 
Properties added via Node.prototype are not visible on nodes retrieved from the
tree (including if I add the property to the iFrame's
contentWindow.Node.prototype rather than just Node.prototype).

I have some theories, thanks to Martin, but it would be nice to hear from
someone on the Mozilla team about the real reason behind this inconsistent
behaviour and any workaround, or fixability and timeframe.

For what it's worth, here are my theories.  I think that the event dispatching
behaviour is dependent on being tied to a window to manage the events, but that
XML documents created with createDocument are not (and cannot be) associated
with a window.

I think that the modifications to Node.prototype are also local to the window
and are not carried through to the iFrame.contentWindow.  I don't have any
explanation for why adding a property to iFrame.contentWindow.Node.prototype
doesn't work, or why setting a property on an individual node works, but
disappears when the node is retrieved again from the DOM.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 21 years ago

I'm seeing several problems here:

1)  The document node does not fire mutation events when a child is appended to
    it.  Doesn't matter whether the document is shown.
2)  Mutation events are not fired for normal DOM modifications for any document
    without a window (since HasMutationListeners returns false).
3)  nsDocument::DispatchEvent will just bail early if it has no presshells.  So
    will nsEventListenerManager::DispatchEvent, if the document has no
    presshells.

We should probably fix all three....

	Johnny Stenback (:jst)
	Comment 9 • 21 years ago

Yeah, we absolutely should...

	Alex Vincent [:WeirdAl] Assignee
	Comment 10 • 19 years ago

Oh, man.  This bug has just absolutely stopped work on my Verbosio project.  I
am dead without this.  This gives me one hell of an incentive to fix.

	Alex Vincent [:WeirdAl] Assignee
	Comment 11 • 19 years ago

Accepting bug.  Patch coming as soon as I have something.

	Alex Vincent [:WeirdAl] Assignee
	Comment 12 • 19 years ago

This patch fixes the issue with regard to document nodes appending child nodes,
but with a window still existing for the document.  I'm still working on
this...

	Alex Vincent [:WeirdAl] Assignee
	Comment 13 • 19 years ago

:( I need an event stage manager.  It does lots of good checks (including
checking security state, whether the event's been initialized, etc.).

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 19 years ago

> :( I need an event stage manager. 

Why?  Whatever functionality is in the ESM that's not presentation-dependent
should probably move out of the ESM...

	Alex Vincent [:WeirdAl] Assignee
	Comment 15 • 19 years ago

(In reply to comment #14)
> > :( I need an event stage manager. 
> 
> Why?  Whatever functionality is in the ESM that's not presentation-dependent
> should probably move out of the ESM...

I made a wrong guess; nsGenericElement doesn't use it for mutation events:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2717

	Alex Vincent [:WeirdAl] Assignee
	Comment 16 • 19 years ago

* Adds a check function for mutation listeners to nsDocument
* Adds dispatching of mutation events to nsDocument, targeted at inserted or
removed nodes
* Changes nsGenericElement's HasMutationListeners to allow for a document
missing a global script object and window
* Adds bubbling of DOM events in general to nsGenericDOMDataNode.

	Alex Vincent [:WeirdAl] Assignee
	Comment 17 • 19 years ago

Requesting blocking1.8b5 to try to get this in for the 1.8 milestone.

	Scott MacGregor
	Comment 19 • 19 years ago

It's really too late in the release cycle for this change. Furthermore I don't
see any justification or risk analysis as to why we should consider this change
for this release. 

Not a regression over 1.0. Minusing this for now.

	Alex Vincent [:WeirdAl] Assignee
	Comment 20 • 19 years ago

cc'ing caillon; should I start work on a new patch based on your comments over
IRC, or wait for the full review?

	Alex Vincent [:WeirdAl] Assignee
	Comment 21 • 19 years ago

Comment on attachment 194700 [details] [diff] [review]
patch, v1 for 1.8 branch

Dropping support for 1.8, per comment 19's first sentence.

	Alex Vincent [:WeirdAl] Assignee
	Comment 22 • 19 years ago

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

back to peterv for the review request.

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 23 • 19 years ago

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

Your static method should be placed on nsContentUtils, and all callers (from
nsDocument and nsGenericElement) should be updated.


>-  if (NS_EVENT_FLAG_BUBBLE & aFlags && parent) {
>-    ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>-                                 aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+  if (NS_EVENT_FLAG_BUBBLE & aFlags) {
>+    //Initiate bubbling phase.  Special case first call to document
>+    if (parent) {
>+      ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                   aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+    } else {
>+      if (document) {
>+        document->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                 aFlags & NS_EVENT_BUBBLE_MASK,
>+                                 aEventStatus);

Don't you want to assign the return of HandleDOMEvent() to |ret| here?

	Alex Vincent [:WeirdAl] Assignee
	Comment 24 • 19 years ago

> Don't you want to assign the return of HandleDOMEvent() to |ret| here?

Interesting question, in that nsGenericElement and nsGenericDOMDataNode do
different things in the capturing and bubbling phases.  

nsGenericElement capturing:  ret assigned for document, not parent
nsGenericElement bubbling:   ret assigned for document and parent
nsGenericDOMDataNode capturing: ret not assigned for document or parent
nsGenericDOMDataNode bubbling: ret not assigned for parent.  My patch changed
that (probably incorrectly).

For nsDocument, there was never a ret value checked.  In none of the cases where
ret was assigned did anyone call NS_FAILED(ret) or NS_ENSURE_SUCCESS, so I don't
know what the intent is.

This is pretty inconsistent; I'd like to have someone clarify for me what the
code should be doing in all these cases.

	Alex Vincent [:WeirdAl] Assignee
	Comment 25 • 19 years ago

sicking says always assign, always check with NS_ENSURE_SUCCESS.

	Alex Vincent [:WeirdAl] Assignee
	Comment 26 • 19 years ago

fixing caillon's review nits in comment 23, adding NS_ENSURE_SUCCESS per
comments 24, 25

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 19 years ago

Don't put the NS_ENSURE_SUCCESS after the |if|. Always use the following pattern:

...
    rv = foo->bar()
    NS_ENSURE_SUCCESS(rv, rv);
...

return NS_OK;


Or possibly end with a |return baz->bin();|

Never end with |return rv;|

	Alex Vincent [:WeirdAl] Assignee
	Comment 28 • 19 years ago

Fixing sicking's nit, though I do have a question.  This change means
NS_ENSURE_SUCCESS comes before aEvent->flags is reset to its original value for
the local handling stage of each event.  Could that be troublesome?

	Alex Vincent [:WeirdAl] Assignee
	Comment 29 • 19 years ago

Comment on attachment 197557 [details] [diff] [review]
patch, v2.1

Whoops.  Missed something.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 31 • 19 years ago

That sounds like it could be a problem, though I'd have to look more in detail
at the code.

In this instance:

+    ret = mListenerManager->HandleEvent(aPresContext, aEvent, aDOMEvent, this,
+                                        aFlags, aEventStatus);
+    NS_ENSURE_SUCCESS(ret, ret);
     aEvent->flags &= ~aFlags;

you could just move the ENSURE one row down.

	Alex Vincent [:WeirdAl] Assignee
	Comment 32 • 19 years ago

I'll not submit a new patch yet, pending the actual review.

	Alex Vincent [:WeirdAl] Assignee
	Comment 33 • 19 years ago

Comment on attachment 197559 [details] [diff] [review]
patch, v2.2

bz's checkin for bug 278472 causes this patch to bitrot.  I'll re-examine and
submit a new patch.

	Alex Vincent [:WeirdAl] Assignee
	Comment 34 • 19 years ago

I'll be using attachment 198110 [details] as a testcase for this bug.

	Alex Vincent [:WeirdAl] Assignee
	Comment 35 • 19 years ago

This patch moves HasMutationListeners into nsContentUtils, and rewrites it to
work with documents.

	Alex Vincent [:WeirdAl] Assignee
	Comment 36 • 19 years ago

Please consider this an addition to attachment 198166 [details] [diff] [review].

	Alex Vincent [:WeirdAl] Assignee
	Comment 37 • 19 years ago

Comment on attachment 198236 [details] [diff] [review]
patch for SVG (missed in previous patch)

curses, bitrotted again.

	Alex Vincent [:WeirdAl] Assignee
	Comment 38 • 19 years ago

fixing conflict, combining with SVG patch.

	Peter Van der Beken [:peterv]
	Comment 39 • 19 years ago

Comment on attachment 198779 [details] [diff] [review]
patch, v3.1

>Index: mozilla/content/base/public/nsContentUtils.h
>===================================================================

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to the node or window passed in.

Given that this takes windows, I think it shouldn't be name *Node*Has...

>+   * @param aType the type of listener (NS_EVENT_BITS_MUTATION_*)

There's only an aNode argument.

>+   * @return whether there are mutation listeners of the specified type for

The type isn't specified.

>+  static PRBool NodeHasMutationListeners(nsISupports* aNode);

This function doesn't seem to be used outside nsContentUtils, it should be file
static in nsContentUtils.cpp, don't put it in the header.

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).
>+   * @param aContent the content of the node to search for listeners

s/the content of//

>Index: mozilla/content/base/src/nsContentUtils.cpp
>===================================================================

>+/* static */
>+PRBool
>+nsContentUtils::NodeHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(getter_AddRefs(manager));
>+    if (manager) {
>+      PRBool hasMutationListeners = PR_FALSE;
>+      manager->HasMutationListeners(&hasMutationListeners);
>+      if (hasMutationListeners)
>+        return PR_TRUE;

Just do |return hasMutationListeners;|

>+/* static */
>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  if (aContent) {
>+    if (aContent->GetCurrentDoc() == nsnull)
>+      return PR_FALSE;
>+    NS_PRECONDITION(aContent->GetCurrentDoc() == aDocument, 
>+                    "Document must be owner document of content!");
>+  } else 
>+  if (!aDocument)
>+    return PR_FALSE;

  NS_PRECONDITION(aContent || aDocument, "Must have document if no content!");
  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
		  aContent->GetCurrentDoc() == aDocument,
		  "Incorrect aDocument");

  if ((aContent && !aContent->IsInDoc()) || !aDocument)
    return PR_FALSE;

>+    if (window && (!window->HasMutationListeners(aType)))

Drop the braces around !window->HasMutationListeners(aType)

>+  for (nsIContent* curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;

  nsIContent* curr;
  for (curr = aContent; curr; curr = curr->GetParent()) {
    if (NodeHasMutationListeners(curr))
      return PR_TRUE;
  }

>+  if (NodeHasMutationListeners(aDocument))
>+    return PR_TRUE;
>+
>+  return (window && NodeHasMutationListeners(window));

You could keep the old code:

  return NodeHasMutationListeners(aDocument) ||
	 NodeHasMutationListeners(window);

>Index: mozilla/content/base/src/nsDocument.cpp
>===================================================================

>@@ -3579,16 +3582,28 @@ nsDocument::RemoveChild(nsIDOMNode* aOld
>     return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>   }
> 
>   PRInt32 indx = mChildren.IndexOfChild(content);
>   if (indx == -1) {
>     return NS_ERROR_DOM_NOT_FOUND_ERR;
>   }
> 
>+  if (nsContentUtils::HasMutationListeners(nsnull,
>+        this,
>+        NS_EVENT_BITS_MUTATION_NODEREMOVED)) {
>+    nsCOMPtr<nsIDOMEventTarget> aMutationOldTarget(do_QueryInterface(aOldChild));
>+    nsMutationEvent mutationOld(PR_TRUE, NS_MUTATION_NODEREMOVED, aMutationOldTarget);
>+    nsIDOMNode* relNode = this;
>+    mutationOld.mRelatedNode = relNode;
>+
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    content->HandleDOMEvent(nsnull, &mutationOld, nsnull, NS_EVENT_FLAG_INIT, &status);
>+  }
>+
>   ContentRemoved(nsnull, content, indx);
> 
>   mChildren.RemoveChildAt(indx);
>   if (content == mRootContent) {
>     DestroyLinkMap();
>     mRootContent = nsnull;
>   }
> 

Instead of adding that code, please add nsGenericElement::doRemoveChild,
something like:

/* static */
nsresult
nsGenericElement::doRemoveChild(nsIDOMNode *aOldChild, nsIContent* aParent,
				nsIDocument* aDocument,
				nsAttrAndChildArray& aChildArray,
				nsIDOMNode** aReturn)
{
  NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!");
  NS_PRECONDITION(!aParent || aParent->GetCurrentDoc() == aDocument,
		  "Incorrect aDocument");

  *aReturn = nsnull;

  if (!aOldChild) {
    return NS_ERROR_NULL_POINTER;
  }

  nsCOMPtr<nsIContent> content(do_QueryInterface(aOldChild));
  if (!content) {
    /*
     * If we're asked to remove something that doesn't support nsIContent
     * it can not be one of our children, i.e. we return NOT_FOUND_ERR.
     */
    return NS_ERROR_DOM_NOT_FOUND_ERR;
  }

  nsContentOrDocument container(aParent, aDocument);
  PRInt32 pos = container.IndexOf(content);

  if (pos < 0) {
    /*
     * aOldChild isn't one of our children.
     */
    return NS_ERROR_DOM_NOT_FOUND_ERR;
  }

  nsresult rv = container.RemoveChildAt(pos, PR_TRUE, aChildArray);

  *aReturn = aOldChild;
  NS_ADDREF(aOldChild);

  return rv;
}

Call nsGenericElement::doRemoveChild from nsDocument::RemoveChild and
nsGenericElement::RemoveChild and I think that gives the same result with less
code.

>Index: mozilla/content/base/src/nsGenericDOMDataNode.cpp
>===================================================================

>@@ -793,54 +793,67 @@ nsGenericDOMDataNode::HandleDOMEvent(nsP

>   //Bubbling stage
>-  if (NS_EVENT_FLAG_BUBBLE & aFlags && parent) {
>-    ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>-                                 aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+  if (NS_EVENT_FLAG_BUBBLE & aFlags) {
>+    //Initiate bubbling phase.  Special case first call to document

What does |Special case first call to document| mean?

>+    if (parent) {
>+      ret = parent->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                   aFlags & NS_EVENT_BUBBLE_MASK, aEventStatus);
>+      NS_ENSURE_SUCCESS(ret, ret);
>+    } else {
>+      if (document) {

    }
    else if (document) {

>Index: mozilla/content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>@@ -1674,17 +1674,19 @@ nsGenericHTMLElement::SetAttr(PRInt32 aN

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Make that

  nsIDocument* document = GetCurrentDoc();
  if (document) {
    hasListeners = nsContentUtils::HasMutationListeners(this,
      document,

>@@ -1984,17 +1986,19 @@ nsresult

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>Index: mozilla/content/svg/content/src/nsSVGElement.cpp
>===================================================================

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>@@ -286,17 +288,19 @@ nsSVGElement::WalkContentStyleRules(nsRu

>   if (IsInDoc()) {
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

>@@ -573,17 +577,19 @@ nsSVGElement::DidModifySVGObservable(nsI

>   if (IsInDoc()) {
>     modification = !!mAttrsAndChildren.GetAttr(attrName->LocalName(),
>                                                attrName->NamespaceID());
>-    hasListeners = nsGenericElement::HasMutationListeners(this,
>+    nsIDocument* document = GetCurrentDoc();
>+    hasListeners = nsContentUtils::HasMutationListeners(this,
>+      document,

Same here.

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

>@@ -1050,17 +1050,19 @@ nsXULElement::RemoveChildAt(PRUint32 aIn

>+    if (nsContentUtils::HasMutationListeners(this,
>+                                             doc,
>+                                             NS_EVENT_BITS_MUTATION_NODEREMOVED)) {

Long line.

>@@ -1211,17 +1213,19 @@ nsXULElement::SetAttr(PRInt32 aNamespace

>     if (IsInDoc()) {
>         PRBool isAccessKey = aName == nsXULAtoms::accesskey &&
>                              aNamespaceID == kNameSpaceID_None;
>-        hasListeners = nsGenericElement::HasMutationListeners(this,
>+        nsIDocument* document = GetCurrentDoc();
>+        hasListeners = nsContentUtils::HasMutationListeners(this,
>+            document,

Again, replace |if (IsInDoc()) {| with |if (document) {|

>@@ -1510,17 +1514,19 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa

>     PRBool hasMutationListeners =
>-        HasMutationListeners(this, NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
>+        nsContentUtils::HasMutationListeners(this,
>+                                             doc,
>+                                             NS_EVENT_BITS_MUTATION_ATTRMODIFIED);

Long line.

>@@ -2320,17 +2326,19 @@ nsXULElement::GetInlineStyleRule()

>     if (IsInDoc()) {
>-        hasListeners = nsGenericElement::HasMutationListeners(this,
>+        nsIDocument* document = GetCurrentDoc();
>+        hasListeners = nsContentUtils::HasMutationListeners(this,
>+            document,

Again, replace |if (IsInDoc()) {| with |if (document) {|

	Alex Vincent [:WeirdAl] Assignee
	Comment 40 • 19 years ago

Thanks, peterv.  I'll get a new patch ready tonight or tomorrow for re-review.

	Alex Vincent [:WeirdAl] Assignee
	Comment 41 • 19 years ago

(In reply to comment #39)
> >@@ -3579,16 +3582,28 @@ nsDocument::RemoveChild(nsIDOMNode* aOld
> Instead of adding that code, please add nsGenericElement::doRemoveChild,
...
> Call nsGenericElement::doRemoveChild from nsDocument::RemoveChild and
> nsGenericElement::RemoveChild and I think that gives the same result with less
> code.

Actually, I did think of that, but bz didn't do that in the patch for bug
278472.  Given that he did do that for ReplaceChild and InsertBefore, I figured
there must have been a good reason he didn't do so.  bz?

That said, I can still do it.

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 19 years ago

> I figured there must have been a good reason he didn't do so.  bz?

Yeah, the reason was that I wanted as small a patch as I could make, since I
needed to land it on branch too.  Nothing was a priori broken about RemoveChild
on documents, so I left it alone.

	Alex Vincent [:WeirdAl] Assignee
	Comment 43 • 19 years ago

I'm filing this patch as a response to peterv's r- commentary in comment 39. 
However, this patch does not cover the doRemoveChild changes peterv requested,
which is why I am not requesting review at this time.

I filed bug 312522 for the doRemoveChild changes, which I no longer feel
comfortable doing on my own.

	Alex Vincent [:WeirdAl] Assignee
	Comment 44 • 19 years ago

In writing this latest version of the patch, I noticed that the rv checking is a bit interesting at the end points.

At the side of nsEventListenerManager->HandleEvent(), it appears to always return NS_OK.  So any failures that happen are absorbed.

For capturing/bubbling at nsGlobalWindow, we just aren't checking any return values on mChromeEventHandler.  Similarly (though I suppose it doesn't matter for this patch), local event handling on nsGlobalWindow doesn't check rv values.  So again, we're returning NS_OK.

This patch passes the returned values through NS_ENSURE_SUCCESS() calls, which happily let things be.  After we get through with HandleDOMEvent() at each level, the level returns NS_OK.

On the receiving end (the functions which call HandleDOMEvent in the first place), a lot of HTML elements do use NS_ENSURE_SUCCESS calls to double-check that things were done correctly.
http://lxr.mozilla.org/seamonkey/search?string=%3A%3AHandleDOMEvent

Add all of the above items up, and I'm a bit concerned.  The extra NS_ENSURE_SUCCESS calls are good, but we could still have failures we're supposed to acknowledge, and we don't.

What do you guys think?  Should these concerns affect the patch?  At the very least, I think we might need some HTML testcases.

	Alex Vincent [:WeirdAl] Assignee
	Comment 45 • 19 years ago

(In reply to comment #44)
> Created an attachment (id=208460) [edit]
> patch, v4

Note to self:  anywhere we have IsInDoc(), we also have nsIDocument *document = GetCurrentDoc().  So a few if (document) or if (IsInDoc()) calls are probably redundant.

	Alex Vincent [:WeirdAl] Assignee
	Comment 46 • 19 years ago

Comment on attachment 208460 [details] [diff] [review]
patch, v4

bitrotted for the third time, thanks to bug 323311

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 47 • 19 years ago

Comment on attachment 208460 [details] [diff] [review]
patch, v4

Just some comments.

>+PRBool
>+ObjectHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(getter_AddRefs(manager));
>+    if (manager) {
>+      PRBool hasMutationListeners = PR_FALSE;
>+      manager->HasMutationListeners(&hasMutationListeners);
>+      return hasMutationListeners;
>+    }
>+  }

I know this code was there already before, but EventListenerManager really shouldn't be
created just to check whether there are mutation listeners. And if aNode
is an nsGenericElement, things are even worse. nsDOMEventRTTearoff is created first to
get the nsIDOMEventReceiver and then nsIEventListenerManager is created to
get the result PR_FALSE from the HadMutationListeners() - that is the normal case I think.
If/when bug 234455 gets fixed, one can ask to get the event listener manager only
if one already exists, and that can be got from nsINode, no need for nsIDOMEventReceiver.


>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  // If we don't have a document, we can't dispatch any mutation events anyway.

Why?

>+  if (!aDocument)
>+    return PR_FALSE;

	Alex Vincent [:WeirdAl] Assignee
	Comment 48 • 19 years ago

(In reply to comment #47)

> >+  // If we don't have a document, we can't dispatch any mutation events anyway.
> 
> Why?

I remember there being a specific reason for this that I discussed outside this bug (irc with one of the content folks?).  Something along the lines of mutation events inside document fragment nodes (which are the only other possibility I'm aware of).

That said, you might have this fixed with bug 234455.

	Alex Vincent [:WeirdAl] Assignee
	Comment 49 • 18 years ago

Martin, could you update the testcase in comment 5 to use https instead of http?  I think this is causing this error:

Error: uncaught exception: Permission denied to call method XMLHttpRequest.open

	Alex Vincent [:WeirdAl] Assignee
	Comment 51 • 18 years ago

(In reply to comment #47)
> if one already exists, and that can be got from nsINode, no need for
> nsIDOMEventReceiver.

smaug: Correct me if I'm wrong, but windows don't implement nsINode.  If that's the case, what should I do?

Also, for the record, since event dispatch is now highly centralized, I'm going to stop caring (at least for this bug) as to whether the handle event routines check rv.  :)

	Alex Vincent [:WeirdAl] Assignee
	Comment 52 • 18 years ago

Comment on attachment 119875 [details]
Test code for mutation events

I'm looking at the testcase for comment 1, and I don't think it works properly.  It checks for the node name of evt.relatedNode, which according to DOM 2 Events "holds the parent node".  Since the inserted node has no parent, evt.relatedNode is null, and errors get thrown in my browser suite with the testcase.

	Alex Vincent [:WeirdAl] Assignee
	Comment 53 • 18 years ago

*sigh* The preceding patch crashed on Martin Honnen's testcase.  Crash fixed.

	Martin Honnen
	Comment 54 • 18 years ago

Alex, I have updated the test case to access all files by HTTPS.

	Alex Vincent [:WeirdAl] Assignee
	Comment 55 • 18 years ago

My Firefox 1.5 build shows the iframe receiving the mutation event, but not the XMLHttpRequest version.

SeaMonkey debug trunk build with patch v5.1 shows the iframe not receiving the mutation event, and the XMLHttpRequest version receiving it.

SeaMonkey debug trunk build without patch v5.1 shows the iframe receiving the mutation event, but not the XMLHttpRequest version.

So somewhere I caused a regression.  I'll need to look into what I did that caused that.

	Alex Vincent [:WeirdAl] Assignee
	Comment 56 • 18 years ago

Stupid copy & paste typo. :) It would have been caught in reviews, anyway.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 57 • 18 years ago

Comment on attachment 214727 [details] [diff] [review]
patch, v5.2


>+PRBool
>+ObjectHasMutationListeners(nsISupports* aNode)
>+{
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(aNode));

Could you QI here to nsINode and use its GetEventListenerManager.
nsIDOMEventReceiver is often a tearoff, and although it is cached,
I think it should be better to avoid using it in this case.


>+
>+/* static */
>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
....
>+  return ObjectHasMutationListeners(aDocument) ||
>+         ObjectHasMutationListeners(window);
>+}

If you change ObjectHasMutationListeners to handle only nsINodes
(perhaps its name should be then NodeHasMutationListeners), you could have
here a special case for the window, something like
if (!NodeHasMutationListeners(aDocument)) {
  nsCOMPtr<nsIDOMEventReceiver> receiver = do_QueryInterface(window);
  ....
}


>+    PRBool hasListeners = PR_FALSE;
>+    if (doc) {
>+      hasListeners = nsContentUtils::HasMutationListeners(this,
>+        doc,
>+        NS_EVENT_BITS_MUTATION_NODEREMOVED);
>+    }
>+
>+    if (hasListeners) {

Why do you need hasListeners variable?
if (doc && nsContentUtils::HasMutationListeners...)
should be enough

	Alex Vincent [:WeirdAl] Assignee
	Comment 58 • 18 years ago

Answering smaug's r- comments for previous patch.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 59 • 18 years ago

Note that the HasMutationListeners is a very critical performance path. Right now I think we spend about 4% of Tp time in that function so you must not do anything to slow it down.

Of course, speedups are always welcome :)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 60 • 18 years ago

(In reply to comment #59)
> Note that the HasMutationListeners is a very critical performance path. Right
> now I think we spend about 4% of Tp time in that function so you must not do
> anything to slow it down.
> 
> Of course, speedups are always welcome :)
>

Exactly :) That is why I proposed using nsINode, not nsIDOMEventReceiver.
Is that 4% really true? We must do something to that.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 61 • 18 years ago

I don't remember where I got that number, but yeah, iirc it's somewhere around there.

The solution is actually pretty simple, we should only check for mutation events when aNotify is true.

	Alex Vincent [:WeirdAl] Assignee
	Comment 62 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

That's for some other bug, where I might want to be cc'd, but not actually do the work... :)

	Alex Vincent [:WeirdAl] Assignee
	Comment 63 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Oops, I promised sicking a chance to look the patch over.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 64 • 18 years ago

(In reply to comment #63)
> (From update of attachment 215071 [details] [diff] [review] [edit])
> Oops, I promised sicking a chance to look the patch over.
> 

that's a good idea. I'm not in anyway 'official' reviewer for anything else than xforms ;)

	timeless
	Comment 65 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).

I know you're just moving the comment, but the parenthetical bothers me :).

>+   * @param aContent  The node to search for listeners
>+   * @param aDocument The owner document of the node, or the node to search
>+   * @param aType     The type of listener (NS_EVENT_BITS_MUTATION_*)

>+PRBool
>+NodeHasMutationListeners(nsINode* aNode)
>+{
>+  if (!aNode)
>+    return PR_FALSE;
>+  nsCOMPtr<nsIEventListenerManager> manager;
>+  aNode->GetEventListenerManager(PR_FALSE, getter_AddRefs(manager));

i wonder if you should return early here instead of overindenting. but this is copied code.

>+  if (manager) {
>+    PRBool hasMutationListeners = PR_FALSE;
>+    manager->HasMutationListeners(&hasMutationListeners);
>+    return hasMutationListeners;
>+  }
>+  return PR_FALSE;
>+}

>+PRBool
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");
>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.

this wasn't a comptr before so it probably shouldn't be one now, as it would incurr an addref and release as well as given your placement the general construction of the comptr. the code should probably be:

if (aDocument) {
  nsIScriptGlobalObject* global = ...
  if (global) ...

>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }
>+
>+  // If we have a window, we know a mutation listener is registered, but it
>+  // might not be in our chain.  If we don't have a window, we might have a
>+  // mutation listener.  Check quickly to see.

The original code declared curr in the for loop init, since you're not using curr outside, i don't think you should change that

>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 66 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>Index: content/base/src/nsContentUtils.cpp
>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");
>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.
>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }
>+
>+  // If we have a window, we know a mutation listener is registered, but it
>+  // might not be in our chain.  If we don't have a window, we might have a
>+  // mutation listener.  Check quickly to see.
>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;
>+
>+  if (NodeHasMutationListeners(aDocument))
>+    return PR_TRUE;
>+
>+  // Last chance:  check the window.
>+  PRBool hasMutationListeners = PR_FALSE;
>+  nsCOMPtr<nsIDOMEventReceiver> rec(do_QueryInterface(window));
>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>+    if (manager) {
>+      manager->HasMutationListeners(&hasMutationListeners);
>+    }
>+  }
>+  return hasMutationListeners;
>+}

You can move this window check to inside the initial if statements. I think that'd be cleaner.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 67 • 18 years ago

Other then that a quick glance looks ok.

	Alex Vincent [:WeirdAl] Assignee
	Comment 68 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Now I'm requesting sr from peterv.

peterv:  if you want a new patch that incorporates the aforementioned changes, just say so.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 69 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

Another comment I forgot to make:

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
...
>+  nsIContent* curr;
>+  for (curr = aContent; curr; curr = curr->GetParent())
>+    if (NodeHasMutationListeners(curr))
>+      return PR_TRUE;

Please follow convention and use {} around |if| and |for| statements. That goes for everywhere in this function. It makes future patches easier to read.

	Peter Van der Beken [:peterv]
	Comment 70 • 18 years ago

Comment on attachment 215071 [details] [diff] [review]
patch, v5.3

>Index: content/base/public/nsContentUtils.h
>===================================================================

>+   * Quick helper to determine whether there are any mutation listeners
>+   * of a given type that apply to this content (are at or above it).
>+   *
>+   * @param aContent  The node to search for listeners
>+   * @param aDocument The owner document of the node, or the node to search

You need to specify this more. Must aDocument be non-null? What about aContent? This talks about the owner document, but in the code you use GetCurrentDoc, which is it?

>+   * @param aType     The type of listener (NS_EVENT_BITS_MUTATION_*)
>+   *
>+   * @return boolean true if there are mutation listeners of the specified type

w/boolean //

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+/**
>+ * Quick helper to determine whether there are any mutation listeners
>+ * of a given type that apply to the node or window passed in.

This talks about window but it takes nsINode?

>+ * @param aNode nsINode to check for listeners.

s/nsINode/node/

>+ * @return PRBool true if there are mutation listeners or not.

s/PRBool true //

>+NodeHasMutationListeners(nsINode* aNode)
>+{
>+  if (!aNode)
>+    return PR_FALSE;

It seems like it would be cheaper to put this null check around the one caller where it can be null (NodeHasMutationListeners(aDocument) below).

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || !aContent->IsInDoc() ||
>+     aContent->GetCurrentDoc() == aDocument,
>+     "Incorrect aDocument");

Line this up on the opening parens.

>+
>+  nsCOMPtr<nsPIDOMWindow> window;
>+
>+  // global will be null for documents that don't have windows.
>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (aDocument)
>+    global = aDocument->GetScriptGlobalObject();
>+  if (global) {
>+    window = do_QueryInterface(global);
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;
>+  }

Why not just

  nsCOMPtr<nsPIDOMWindow> window;
  if (aDocument) {
    window = do_QueryInterface(aDocument->GetScriptGlobalObject());
    if (window && !window->HasMutationListeners(aType))
      return PR_FALSE;
  }

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

>@@ -1439,31 +1445,31 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa

>-    if (doc) {
>-        if (hasMutationListeners) {
>-            nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
>+    if (hasMutationListeners) {
>+        nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
> 
>-            mutation.mRelatedNode = attrNode;
>-            mutation.mAttrName = aName;
>+        mutation.mRelatedNode = attrNode;
>+        mutation.mAttrName = aName;
> 
>-            if (!oldValue.IsEmpty())
>-              mutation.mPrevAttrValue = do_GetAtom(oldValue);
>-            mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
>+        if (!oldValue.IsEmpty())
>+            mutation.mPrevAttrValue = do_GetAtom(oldValue);
>+        mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
> 
>-            nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this),
>-                                        nsnull, &mutation);
>-        }
>+        nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this),
>+                                    nsnull, &mutation);
>+    }
> 
>+    if (doc) {

This change doesn't really add anything? It just messes up cvs blame.

	Alex Vincent [:WeirdAl] Assignee
	Comment 71 • 18 years ago

fixing nits from peterv's sr- and other comments since previous patch was posted.

	Peter Van der Beken [:peterv]
	Comment 72 • 18 years ago

Comment on attachment 216183 [details] [diff] [review]
patch, v5.4

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(aDocument,
>+                  "nsContentUtils::HasMutationListeners requires a document!");

I don't understand this, you require aDocument to be non-null, but then you made the function handle a null aDocument below? This really needs to be cleared up.

>+  NS_PRECONDITION(!aContent || aContent->GetCurrentDoc() == aDocument,
>+                  "Incorrect aDocument");
>+  if (aDocument)
>+  {

The style in these files is to put the brace on the same line, so do that everywhere.

>+    // global object will be null for documents that don't have windows.
>+    nsCOMPtr<nsPIDOMWindow> window;
>+    window = do_QueryInterface(aDocument->GetScriptGlobalObject());
>+    if (window && !window->HasMutationListeners(aType))
>+      return PR_FALSE;

You didn't add the braces around this if.

>+      PRBool hasListeners = PR_FALSE;
>+      if (manager)
>+      {
>+        manager->HasMutationListeners(&hasListeners);
>+      }
>+      if (hasListeners)
>+      {
>+        return PR_TRUE;
>+      }

      if (manager) {
        PRBool hasListeners = PR_FALSE;
        manager->HasMutationListeners(&hasListeners);
        if (hasListeners) {
          return PR_TRUE;
        }
      }

>+  if (aDocument && NodeHasMutationListeners(aDocument))
>+  {
>+    return PR_TRUE;
>+  }
>+
>+  return PR_FALSE;

  return aDocument && NodeHasMutationListeners(aDocument);

	Alex Vincent [:WeirdAl] Assignee
	Comment 73 • 18 years ago

Marking dependent on bug 90983 so I can track that bug before posting another patch.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 74 • 18 years ago

No need to wait for that bug before checking this in. This patch will still make us fire mutation events for the vast majority of nodes in the document. Take one step at a time, it doesn't need to be perfect right away.

	Alex Vincent [:WeirdAl] Assignee
	Comment 75 • 18 years ago

I'm going to keep trying until I get this right.  :)

In current trunk builds, we would hit my assertion for NS_PRECONDITION(aDocument) while constructing the browser window, so I just took it out.  I'm now returning PR_FALSE in nsContentUtils::HasMutationListeners if (!aDocument), because this patch isn't about supporting mutation events in content that doesn't have a document.

If someone wants that, they can file their own bug for it.  :)

	Peter Van der Beken [:peterv]
	Comment 76 • 18 years ago

Comment on attachment 216776 [details] [diff] [review]
patch, v5.5

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>+nsContentUtils::HasMutationListeners(nsIContent* aContent,
>+                                     nsIDocument* aDocument,
>+                                     PRUint32 aType)
>+{
>+  NS_PRECONDITION(!aContent || aContent->GetCurrentDoc() == aDocument,
>+                  "Incorrect aDocument");
>+  if (!aDocument) {
>+    // We do not support event listeners on content not attached to documents.
>+    return PR_FALSE;
>+  }

Since you check here for aDocument, you should remove all the checks for null doc at the callsites for this function and remove the mention in the documentation that aDocument must be non-null.

>+  if (rec) {
>+    nsCOMPtr<nsIEventListenerManager> manager;
>+    rec->GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>+    PRBool hasListeners = PR_FALSE;

Drop this var. didn't you get a warning about the next declaration shadowing this one?

>+    if (manager) {
>+      PRBool hasListeners = PR_FALSE;


>+  if (NodeHasMutationListeners(aDocument)) {
>+    return PR_TRUE;
>+  }
>+
>+  return PR_FALSE;

Just replace this with:

  return NodeHasMutationListeners(aDocument);

	Alex Vincent [:WeirdAl] Assignee
	Comment 78 • 18 years ago

Fix checked in by bz.  Thanks, everyone!