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.

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

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

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.

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.

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.

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.

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....

Yeah, we absolutely should...

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.

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

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...

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

> :( 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...

(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

* 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.

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

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.

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

Comment on attachment 194700 [details] [diff] [review] patch, v1 for 1.8 branch Dropping support for 1.8, per comment 19's first sentence.

Comment on attachment 194692 [details] [diff] [review] patch, v1 back to peterv for the review request.

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?

> 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.

sicking says always assign, always check with NS_ENSURE_SUCCESS.

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

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;|

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?

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

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.

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

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.

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

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

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

Comment on attachment 198236 [details] [diff] [review] patch for SVG (missed in previous patch) curses, bitrotted again.

fixing conflict, combining with SVG patch.

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) {|

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

(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.

> 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.

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.

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.

(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.

Comment on attachment 208460 [details] [diff] [review] patch, v4 bitrotted for the third time, thanks to bug 323311

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;

(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.

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

(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. :)

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.

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

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

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.

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

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

Answering smaug's r- comments for previous patch.

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 :)

(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.

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.

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... :)

Comment on attachment 215071 [details] [diff] [review] patch, v5.3 Oops, I promised sicking a chance to look the patch over.

(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 ;)

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;

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.

Other then that a quick glance looks ok.

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.

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.

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.

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

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);

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

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.

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. :)

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);

Fix checked in by bz. Thanks, everyone!