I think we all know how much trouble mutation events are causing. Lets try to do something about it while waiting for their replacement. The main problem is that we're firing them synchronously while having scriptblockers and updates going. While we try to fix this new problems keep emerging leading to ever more complex code. Instead we should fire them from scriptrunners. This should simplify things greatly as well as provide a simple fix for any places where they are still an issue (simply add a scriptblocker with a bigger scope to delay the event). Unfortunately DOMNodeRemoved can't be fixed this way. The spec says that it needs to fire before the mutation. But even more importantly, if we fire it after the mutation, then we'll fire it at a disconnected node, and so the event won't bubble up to eventhandlers attached higher up in the parent chain. So instead we should fire them before adding any scriptblockers or starting any updates. And before accumulating any state. This means adding code to fire them in more places, but it still seems like a better solution. In some cases in XUL/XBL it would be way too complex to fire DOMNodeRemoved before starting the mutation. I think it's fine to simply not fire them in these cases. Similarly, UserDataHandlers for node adoption should fire from a scriptrunner. Patches coming up!

The only outstanding issue is how to properly fix bug 643786. So landing this is blocked by a better fix for that bug. This patch disables the test for that bug so that it can pass try.

This removes unused classes and arguments as well as some code branches that only existed to deal with mutations happening while firing mutation events. I'm not asking for review on this yet since we might want to wait with landing this until the first two patches have survived a release as to make backout easier. But attaching for safe keeping as well as to let you see which cleanups I left for later (rather than forgot).

Comment on attachment 526486 [details] [diff] [review] Part 1: Fix mutation events Removing request for now. There's still an outstanding issue with AdoptNode calls somewhere. Tracking it down.

Turns out that it was just a wrong assert. This has that fixed.

With the patches here we don't need the workaround from bug 645572 any more. This backs out and adds an assertion instead.

Have you run the patches on tryserver? How often does + NS_ASSERTION((aChild->IsNodeOfType(nsINode::eCONTENT) && + static_cast<nsIContent*>(aChild)-> + IsInNativeAnonymousSubtree()) || + nsContentUtils::IsSafeToRunScript(), + "Want to fire DOMNodeRemoved event, but it's not safe"); fire? Also, I wonder how much more often that happens if you remove that NativeAnonymousSubtree check? if there are many cases when the event isn't dispatched in XBL, should we disable mutation events altogether in anonymous content? Having inconsistent APIs is *bad*. Either we should fire the event or not.

I'm fairly sure it doesn't fire at all, nor would if I remove the anon check. However keep in mind that we don't even get here if nsINode::RemoveChildAt is used, rather than for example nsIDOMNode::RemoveChild is used. The places that I know about that won't fire the event is XUL templates as well as when removing the node creates using xbl:attr in combination with xbl:text. I.e. when XBL attribute forwarding creates a text child. I don't think it would make things more consistent to never fire DOMNodeRemoved for native anon content. As things stand it's fairly consistent in that certain specially managed nodes doesn't cause it to fire. Rather than when the nodes appear in certain places (in native anon content). I should remove the native anon check though to ensure that things remain that way.

Comment on attachment 526532 [details] [diff] [review] Part 2: Fix UserDataHandler for adoptNode >+class nsUserDataCaller : public nsRunnable { { should be in the next line. >+ nsCOMPtr<nsINode> parent = adoptedNode->GetNodeParent(); >+ if (parent) { >+ nsContentUtils::MaybeFireNodeRemoved(adoptedNode, parent, >+ adoptedNode->GetOwnerDoc()); >+ } Why nsCOMPtr<nsINode> parent? Why not just nsINode* parent? > >- rv = nsNodeUtils::CallUserDataHandlers(nodesWithProperties, this, >- nsIDOMUserDataHandler::NODE_ADOPTED, >- PR_FALSE); >- NS_ENSURE_SUCCESS(rv, rv); >+ nsContentUtils::AddScriptRunner(new nsUserDataCaller(nodesWithProperties, >+ this)); Since nsUserDataCaller has nsCOMPtr and nsCOMArray members, I assume this change does show up in the profiles. addref/release especially on cycle collected objects is not very fast. Before creating nsUserDataCaller, could we check if there are any user data handlers. Though, would be better to profile first. r- until nsUserDataCaller usage is profiled and either fixed or explained why it doesn't slow down adoptNode.

Comment on attachment 526531 [details] [diff] [review] Part 1: Fix mutation events v1.1 > nsIContent* > nsGenericTextNode::ReplaceWholeText(const nsAFlatString& aContent, > nsresult* aResult) > { > *aResult = NS_OK; > >- // Batch possible DOMSubtreeModified events. >- mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull); >- mozAutoDocUpdate updateBatch(GetCurrentDoc(), UPDATE_CONTENT_MODEL, PR_TRUE); >- >+ // Handle parent-less nodes > nsCOMPtr<nsIContent> parent = GetParent(); >- >- // Handle parent-less nodes > if (!parent) { > if (aContent.IsEmpty()) { > return nsnull; > } > >- SetText(aContent.get(), aContent.Length(), PR_TRUE); >+ SetNodeValue(aContent); > return this; > } > >+ nsCOMPtr<nsIDocument> doc = GetOwnerDoc(); Why nsCOMPtr? >+ // Fire mutation events. Optimize the common case of there being no >+ // listeners >+ nsPIDOMWindow* window = nsnull; >+ if (doc) { >+ window = doc->GetInnerWindow(); >+ } >+ if (!window || >+ window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED)) { >+ for (PRInt32 i = first; i <= last; ++i) { >+ nsCOMPtr<nsIContent> child = parent->GetChildAt((PRUint32)i); >+ if (child && >+ (i != index || aContent.IsEmpty())) { >+ nsContentUtils::MaybeFireNodeRemoved(child, parent, doc); >+ } >+ } >+ } Is there a reason to not use nsContentUtils::HasMutationListeners? >@@ -3642,51 +3684,26 @@ nsGenericElement::RemoveChildAt(PRUint32 > return NS_OK; > } > > nsresult > nsINode::doRemoveChildAt(PRUint32 aIndex, PRBool aNotify, > nsIContent* aKid, nsAttrAndChildArray& aChildArray, > PRBool aMutationEvent) > { >- nsIDocument* doc = GetCurrentDoc(); >- >- nsMutationGuard::DidMutate(); >- > NS_PRECONDITION(aKid && aKid->GetNodeParent() == this && > aKid == GetChildAt(aIndex) && > IndexOf(aKid) == (PRInt32)aIndex, "Bogus aKid"); > >+ nsMutationGuard::DidMutate(); >+ >+ nsIDocument* doc = GetCurrentDoc(); >+ > mozAutoDocUpdate updateBatch(doc, UPDATE_CONTENT_MODEL, aNotify); > >- nsMutationGuard guard; >- >- mozAutoSubtreeModified subtree(nsnull, nsnull); >- if (aNotify && >- aMutationEvent && >- nsContentUtils::HasMutationListeners(aKid, >- NS_EVENT_BITS_MUTATION_NODEREMOVED, this)) { >- mozAutoRemovableBlockerRemover blockerRemover(GetOwnerDoc()); >- >- nsMutationEvent mutation(PR_TRUE, NS_MUTATION_NODEREMOVED); >- mutation.mRelatedNode = do_QueryInterface(this); >- >- subtree.UpdateTarget(GetOwnerDoc(), this); >- nsEventDispatcher::Dispatch(aKid, nsnull, &mutation); >- } So this breaks DOMNodeRemoved for example when innerHTML is used? ... apparently no since you add new code there to dispatch events in that case. DOMNodeRemoved handling looks quite fragile to me. And if I read the code correctly, this breaks DOMNodeRemoved for example when anchor_element.text is used. >+nsPLDOMEvent::nsPLDOMEvent(nsINode *aEventNode, nsEvent &aEvent) >+ : mEventNode(aEventNode), mDispatchChromeOnly(PR_FALSE) >+{ >+ nsEventDispatcher::CreateEvent(nsnull, &aEvent, EmptyString(), >+ getter_AddRefs(mEvent)); >+ NS_ASSERTION(mEvent, "Should never fail to create an event"); >+ nsCOMPtr<nsIPrivateDOMEvent> priv = do_QueryInterface(mEvent); >+ NS_ASSERTION(priv, "Should also not fail to QI to nsIDOMEventPrivate"); >+ priv->DuplicatePrivateData(); >+} Unusual to call DuplicatePrivateData() here, but should work ok. >+++ b/content/events/test/test_bug288392.html I don't understand the need for changes in this file. DOMSubtreeModified should just work the same way. (The changes may make tests easier to read, but please fix the test in a different bug so that we can ensure that the patch for this bug doesn't break any tests) >-load 643786-1.html Why this change?

(In reply to comment #11) > >+ nsCOMPtr<nsINode> parent = adoptedNode->GetNodeParent(); > >+ if (parent) { > >+ nsContentUtils::MaybeFireNodeRemoved(adoptedNode, parent, > >+ adoptedNode->GetOwnerDoc()); > >+ } > Why nsCOMPtr<nsINode> parent? Why not just nsINode* parent? I'm generally trying to hold strong references whenever calling out into script. Otherwise there's a risk that nodes will go away while running the script and I don't want to rely on kungFuDeathGrips. > >- rv = nsNodeUtils::CallUserDataHandlers(nodesWithProperties, this, > >- nsIDOMUserDataHandler::NODE_ADOPTED, > >- PR_FALSE); > >- NS_ENSURE_SUCCESS(rv, rv); > >+ nsContentUtils::AddScriptRunner(new nsUserDataCaller(nodesWithProperties, > >+ this)); > Since nsUserDataCaller has nsCOMPtr and nsCOMArray members, I assume this > change does show > up in the profiles. addref/release especially on cycle collected objects is not > very fast. > Before creating nsUserDataCaller, could we check if there are any user data > handlers. > Though, would be better to profile first. Since nsUserDataCaller isn't cycle collected this never refcount cycle collected objects more then needed. But it's a good idea to short-cut this when there are no nodes with user data as that is by far the most common case. Ideally this code will go away very soon as I think we should drop support for userdata. Weakmaps solves most of the same use cases anyway.

(In reply to comment #13) > I'm generally trying to hold strong references whenever calling out into > script. Otherwise there's a risk that nodes will go away while running the > script and I don't want to rely on kungFuDeathGrips. But MaybeFireNodeRemoved should take care all all that, and addref/release only if needed. > Since nsUserDataCaller isn't cycle collected this never refcount cycle > collected objects more then needed. I don't understand that. What has nsUserDataCaller not-being-cycle-collectable to do with addrefing/releasing being called (and those being slow when called on cycle collectable objects, like nsINode). > Weakmaps solves most of the same use cases anyway. What is Weakmaps? Apparently something ECMA related...

(In reply to comment #12) > > > >+ nsCOMPtr<nsIDocument> doc = GetOwnerDoc(); > Why nsCOMPtr? See comment 13 about holding strong references while running script. > >+ // Fire mutation events. Optimize the common case of there being no > >+ // listeners > >+ nsPIDOMWindow* window = nsnull; > >+ if (doc) { > >+ window = doc->GetInnerWindow(); > >+ } > >+ if (!window || > >+ window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED)) { > >+ for (PRInt32 i = first; i <= last; ++i) { > >+ nsCOMPtr<nsIContent> child = parent->GetChildAt((PRUint32)i); > >+ if (child && > >+ (i != index || aContent.IsEmpty())) { > >+ nsContentUtils::MaybeFireNodeRemoved(child, parent, doc); > >+ } > >+ } > >+ } > Is there a reason to not use nsContentUtils::HasMutationListeners? nsContentUtils::MaybeFireNodeRemoved does that. Hence the 'Maybe'. > So this breaks DOMNodeRemoved for example when innerHTML is used? > ... apparently no since you add new code there to dispatch events in > that case. DOMNodeRemoved handling looks quite fragile to me. Yes, that is the downside with this approach. We can't rely on RemoveChildAt firing DOMNodeRemoved, so we have to manually fire it whereever needed. I don't see a way around that other than simply leaving things as they are which I think is a worse solution. Medium to long term we will hopefully get rid of mutation events entirely which will solve this in a good way. > And if I read the code correctly, this breaks DOMNodeRemoved for example > when anchor_element.text is used. Good catch. I need to audit callers of nsContentUtils::SetNodeTextContent. Somehow I missed that when auditing callers of RemoveChildAt. > >+nsPLDOMEvent::nsPLDOMEvent(nsINode *aEventNode, nsEvent &aEvent) > >+ : mEventNode(aEventNode), mDispatchChromeOnly(PR_FALSE) > >+{ > >+ nsEventDispatcher::CreateEvent(nsnull, &aEvent, EmptyString(), > >+ getter_AddRefs(mEvent)); > >+ NS_ASSERTION(mEvent, "Should never fail to create an event"); > >+ nsCOMPtr<nsIPrivateDOMEvent> priv = do_QueryInterface(mEvent); > >+ NS_ASSERTION(priv, "Should also not fail to QI to nsIDOMEventPrivate"); > >+ priv->DuplicatePrivateData(); > >+} > Unusual to call DuplicatePrivateData() here, but should work ok. If you have better ideas then I'm all ears. It took me a while to figure out how this stuff works. > >+++ b/content/events/test/test_bug288392.html > I don't understand the need for changes in this file. > DOMSubtreeModified should just work the same way. > (The changes may make tests easier to read, but please fix the > test in a different bug so that we can ensure that the patch for this > bug doesn't break any tests) Sure, the changes were just to make test results easier to read. The problem right now is that if one of the early tests fail, all subsequent tests fail too which I ran into while writing this patch. > >-load 643786-1.html > Why this change? This is why this bug depends on bug 643786. The fix there needs to be adjusted. I disabled the test here so that I could run things through tryserver without crashing. I definitely won't land without a better fix for bug 643786.

(In reply to comment #14) > (In reply to comment #13) > > I'm generally trying to hold strong references whenever calling out into > > script. Otherwise there's a risk that nodes will go away while running the > > script and I don't want to rely on kungFuDeathGrips. > But MaybeFireNodeRemoved should take care all all that, and addref/release > only if needed. But then the document or nodes could go away by the time MaybeFireNodeRemoved returns. > > Since nsUserDataCaller isn't cycle collected this never refcount cycle > > collected objects more then needed. > I don't understand that. What has nsUserDataCaller not-being-cycle-collectable > to do with addrefing/releasing being called (and those being slow when > called on cycle collectable objects, like nsINode). Yes, but we only addref/release nodes if the nodelist is non-empty, no? And in that case we need to addref/release them anyway (and that's an extremely rare case given how rarely userdata is used on the web. Not worth optimizing for IMHO). > > Weakmaps solves most of the same use cases anyway. > What is Weakmaps? Apparently something ECMA related... Yes. It's a hashtable which lets you use objects as keys without holding the object alive. Hence you can store arbitrary data about an object by storing it in a hash.

(In reply to comment #16) > Yes, but we only addref/release nodes if the nodelist is non-empty, no? And in > that case we need to addref/release them anyway (and that's an extremely rare > case given how rarely userdata is used on the web. Not worth optimizing for > IMHO). True, if you optimize nsUserDataCaller usage so that it is not created always - only if there are user data handler.

(In reply to comment #16) > > But then the document or nodes could go away by the time MaybeFireNodeRemoved > returns. Ah, right, there is a loop. In such case nsCOMPtr is indeed needed, but only if event will be dispatched.

So it turns out that removing the native-anon check from the assertion makes it trigger a whole lot. It's due to editor messing around with native anonymous nodes and doing so using DOM methods. IIRC it does so while we're doing layout stuff which is why we have script blockers. So keeping the patch as it was in v1.1 means that we won't fire mutation events for some editor stuff happening in native-anon trees, which is somewhat inconsistent. So I changed the code that short-cuts firing DOMNodeRemoved if the are script-blockers such that it always fires if we're in native anonymous content. Though I would actually prefer not to do that as I think the added, internal-only, inconsistency is worse than the risk that people will listen to the event and do bad things from the handler. It's extremely easy to shoot yourself in the foot by mistake while there are script blockers on the stack. Note that this is not a new problem with this patch. We already fire many mutation events for native-anon content while there are scriptblockers. This patch actually reduces the problem such that it only remains for DOMNodeRemoved. Let me know which way you prefer to go.

We must not fire any mutation events for native anon. nsContentUtils::HasMutationListeners should take care of that.

And my comment 9 was wrong. I was thinking anonymous content, not native anonymous. Sorry.

Comment on attachment 527437 [details] [diff] [review] Part 1: Fix mutation events v2 > nsINode::ReplaceOrInsertBefore(PRBool aReplace, nsINode* aNewChild, > nsINode* aRefChild) > { > if (!aNewChild || (aReplace && !aRefChild)) { > return NS_ERROR_NULL_POINTER; > } > >- if (!IsNodeOfType(eDOCUMENT) && >- !IsNodeOfType(eDOCUMENT_FRAGMENT) && >- !IsElement()) { >+ if ((!IsNodeOfType(eDOCUMENT) && >+ !IsNodeOfType(eDOCUMENT_FRAGMENT) && >+ !IsElement()) || >+ !aNewChild->IsNodeOfType(eCONTENT)){ > return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR; > } > >+ PRUint16 nodeType = 0; >+ nsresult res = aNewChild->GetNodeType(&nodeType); >+ NS_ENSURE_SUCCESS(res, res); >+ >+ // Before we do anything else, fire all DOMNodeRemoved mutation events >+ // We do this up front as to avoid having to deal with script running >+ // at random places further down. >+ // Scope firing mutation events so that we don't carry any state that >+ // might be stale >+ { >+ nsCOMPtr<nsIDocument> doc = GetOwnerDoc(); >+ nsPIDOMWindow* window = nsnull; >+ if (doc) { >+ window = doc->GetInnerWindow(); >+ } >+ if (!window || >+ window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED)) { I'd still like to avoid that nsCOMPtr<nsIDocument> in case there are no mutation event listeners. > void >+nsGenericElement::FireNodeRemovedForChildren() >+{ >+ nsIDocument* doc = GetOwnerDoc(); >+ nsPIDOMWindow* window = nsnull; >+ if (doc) { >+ window = doc->GetInnerWindow(); >+ } >+ >+ // Optimize the common case >+ if (window && >+ !window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED)) { >+ return; >+ } Why not just if (!nsContentUtils::HasMutationListeners(...)) ? I did ask that before, and actually didn't get any answer why you're explicitly using window->HasMutationListeners and not the helper method nsContentUtils::HasMutationListeners > virtual nsresult SetTextContent(const nsAString& aTextContent) > { >- // Batch possible DOMSubtreeModified events. >- mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull); >- return nsContentUtils::SetNodeTextContent(this, aTextContent, PR_FALSE); >+ return nsContentUtils::SetNodeTextContent(this, aTextContent, PR_FALSE, >+ PR_TRUE); missing one space before 'return' >+ // Batch possible DOMSubtreeModified events. >+ mozAutoSubtreeModified subtree(doc, nsnull); >+ >+ FireNodeRemovedForChildren(); >+ > // This BeginUpdate/EndUpdate pair is important to make us reenable the > // scriptloader before the last EndUpdate call. > mozAutoDocUpdate updateBatch(doc, UPDATE_CONTENT_MODEL, PR_TRUE); > >- // Batch possible DOMSubtreeModified events. >- mozAutoSubtreeModified subtree(doc, nsnull); >- > // Remove childnodes >- nsContentUtils::SetNodeTextContent(this, EmptyString(), PR_FALSE); >+ nsContentUtils::SetNodeTextContent(this, EmptyString(), PR_FALSE, PR_FALSE); So why can't ::SetInnerHTML reuse SetNodeTextContent's DOMNodeRemoved dispatching? Why does it need to have the separate FireNodeRemovedForChildren()? I assume because of mozAutoDocUpdate? Some comment would be good. nsresult nsContentUtils::SetNodeTextContent(nsIContent* aContent, const nsAString& aValue, - PRBool aTryReuse) -{ + PRBool aTryReuse, + PRBool aFireNodeRemoved) +{ + // Fire DOMNodeRemoved mutation events before we do anything else. + nsCOMPtr<nsIDocument> owningDoc; + nsCOMPtr<nsIContent> owningContent; + + nsIDocument* doc = aContent->GetOwnerDoc(); + + // Batch possible DOMSubtreeModified events. + mozAutoSubtreeModified subtree(doc, nsnull); This addrefs/releases doc, so there will be probably a measurable slow down in innerHTML and textContent Sorry for complaining nsCOMPtr usage, but that is what I've tried to optimize in innerHTML and event handling and elsewhere. mozAutoSubtreeModified has UpdateTarget, which can be used to optimize addrefing/releasing, so that those happen only when some mutation events are actually dispatched. Since RemoveChildAt is very fragile, I need to still go through all the cases it is used.

Comment on attachment 527439 [details] [diff] [review] Part 2: Fix UserDataHandler for adoptNode v2 >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp >--- a/content/base/src/nsDocument.cpp >+++ b/content/base/src/nsDocument.cpp >@@ -6007,63 +6007,94 @@ BlastSubtreeToPieces(nsINode *aNode) > #endif > aNode->RemoveChildAt(0, PR_FALSE); > > // XXX Should we abort here? > NS_ASSERTION(NS_SUCCEEDED(rv), "Uhoh, RemoveChildAt shouldn't fail!"); > } > } > >+ >+class nsUserDataCaller : public nsRunnable { { should be in the next line > nsDocument::AdoptNode(nsIDOMNode *aAdoptedNode, nsIDOMNode **aResult) > { > NS_ENSURE_ARG(aAdoptedNode); > > *aResult = nsnull; > > nsresult rv = nsContentUtils::CheckSameOrigin(this, aAdoptedNode); > NS_ENSURE_SUCCESS(rv, rv); > >- nsCOMPtr<nsINode> adoptedNode; >+ nsCOMPtr<nsINode> adoptedNode = do_QueryInterface(aAdoptedNode); >+ >+ // Scope firing mutation events so that we don't carry any state that >+ // might be stale >+ { >+ nsCOMPtr<nsINode> parent = adoptedNode->GetNodeParent(); >+ if (parent) { >+ nsContentUtils::MaybeFireNodeRemoved(adoptedNode, parent, >+ adoptedNode->GetOwnerDoc()); >+ } As far as I see, MaybeFireNodeRemoved guarantees that parent stays alive during the possible event dispatch. So no need for nsCOMPtr. You should probably document that MaybeFireNodeRemoved keeps all the parameters alive. >@@ -3572,61 +3572,59 @@ AdoptNodeIntoOwnerDoc(nsINode *aParent, > > nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aNode, &rv); > NS_ENSURE_SUCCESS(rv, rv); > > nsCOMPtr<nsIDOMNode> adoptedNode; > rv = domDoc->AdoptNode(node, getter_AddRefs(adoptedNode)); > NS_ENSURE_SUCCESS(rv, rv); > >- if (doc != aParent->GetOwnerDoc()) { >- return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; >- } Why can you remove this? Because the method is only called in such cases that doc == aParent->GetOwnerDoc() is guaranteed? Could you add an assertion that aNode doesn't have parent (so that AdoptNode doesn't fire DOMNodeRemoved)

Comment on attachment 527437 [details] [diff] [review] Part 1: Fix mutation events v2 >@@ -1106,20 +1109,23 @@ public: > * > * Will reuse the first text child if one is available. Will not reuse > * existing cdata children. > * > * @param aContent Node to set contents of. > * @param aValue Value to set contents to. > * @param aTryReuse When true, the function will try to reuse an existing > * textnodes rather than always creating a new one. >+ * @param aTryReuse When true, the function will fire DOMNodeRemoved for the >+ * nodes being removed. s/aTryReuse/aFireNodeRemoved/ >+nsContentUtils::MaybeFireNodeRemoved(nsINode* aChild, nsINode* aParent, >+ nsIDocument* aOwnerDoc) ... >+ if (!IsSafeToRunScript() && >+ !(aChild->IsNodeOfType(nsINode::eCONTENT) && >+ static_cast<nsIContent*>(aChild)-> >+ IsInNativeAnonymousSubtree())) { Because of HasMutationListeners check, no need to check IsInNativeAnonymousSubtree > nsGenericTextNode::ReplaceWholeText(const nsAFlatString& aContent, > nsresult* aResult) > { > *aResult = NS_OK; > >- // Batch possible DOMSubtreeModified events. >- mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull); >- mozAutoDocUpdate updateBatch(GetCurrentDoc(), UPDATE_CONTENT_MODEL, PR_TRUE); >- >+ // Handle parent-less nodes > nsCOMPtr<nsIContent> parent = GetParent(); >- >- // Handle parent-less nodes > if (!parent) { > if (aContent.IsEmpty()) { > return nsnull; > } > >- SetText(aContent.get(), aContent.Length(), PR_TRUE); >+ SetNodeValue(aContent); > return this; > } > >+ nsCOMPtr<nsIDocument> doc = GetOwnerDoc(); >+ >+ // Batch possible DOMSubtreeModified events. >+ mozAutoSubtreeModified subtree(doc, nsnull); mozAutoSubtreeModified keeps doc alive, so no need for 2 addref/release > nsGenericElement::Normalize() > { > // Batch possible DOMSubtreeModified events. > mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull); > >+ nsCOMPtr<nsIDocument> doc = GetOwnerDoc(); Same here >+ nsPIDOMWindow* window = nsnull; >+ if (doc) { >+ window = doc->GetInnerWindow(); >+ } >+ bool hasRemoveListeners = >+ !window || window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED); >+ > nsresult result = NS_OK; > PRUint32 index, count = GetChildCount(); > > for (index = 0; (index < count) && (NS_OK == result); index++) { > nsIContent *child = GetChildAt(index); > > nsCOMPtr<nsIDOMNode> node = do_QueryInterface(child); > if (node) { > PRUint16 nodeType; > node->GetNodeType(&nodeType); > > switch (nodeType) { > case nsIDOMNode::TEXT_NODE: > > // ensure that if the text node is empty, it is removed > if (0 == child->TextLength()) { >+ if (hasRemoveListeners) { >+ nsContentUtils::MaybeFireNodeRemoved(child, this, doc); >+ } > result = RemoveChildAt(index, PR_TRUE); So, if DOMNodeRemoved moved the child to elsewhere, wrong node is removed here. > if (siblingNodeType == nsIDOMNode::TEXT_NODE) { >+ if (hasRemoveListeners) { >+ nsContentUtils::MaybeFireNodeRemoved(sibling, this, doc); >+ } > result = RemoveChildAt(index+1, PR_TRUE); Same here. > static nsresult > AdoptNodeIntoOwnerDoc(nsINode *aParent, nsINode *aNode) > { >+ NS_ASSERTION(!aNode->GetNodeParent(), >+ "Should have removed from parent already"); >+ Ah, you did add the assertion I asked when reviewing the other patch :) > if (didSanitize) { > // Replace the style element content with its sanitized style text >- nsContentUtils::SetNodeTextContent(style, sanitizedStyleText, PR_TRUE); >+ // The node should really be empty since we haven't added anything >+ // to it, so the last argument shouldn't matter. But just in case, >+ // tell it not to fire any. >+ nsContentUtils::SetNodeTextContent(style, sanitizedStyleText, PR_TRUE, >+ PR_FALSE); So we don't dispatch mutation events in this case. Why? I'd like to see still one new version of the patch. And since this changes mutation event handling, which is both somewhat regression risky and security sensitive, I think someone else should review this too.

(In reply to comment #26) > > nsINode::ReplaceOrInsertBefore(PRBool aReplace, nsINode* aNewChild, ... > I'd still like to avoid that nsCOMPtr<nsIDocument> in case there are no > mutation event listeners. Doh! I just realized that this code was wrong. The short-cut check to check if the flag was set on the window was only correct for the first of the three sets of events fired here. The other nodes might be living in a different document, and so we'd need to check on a different window. So the short-cut to just check on one window doesn't work. Once fixed, the relevant code doesn't exist at all. I also switched to using FireNodeRemovedForChildren to fire the events for the fragment which has a correct optimization. > > void > >+nsGenericElement::FireNodeRemovedForChildren() > >+{ > >+ nsIDocument* doc = GetOwnerDoc(); > >+ nsPIDOMWindow* window = nsnull; > >+ if (doc) { > >+ window = doc->GetInnerWindow(); > >+ } > >+ > >+ // Optimize the common case > >+ if (window && > >+ !window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED)) { > >+ return; > >+ } > Why not just > if (!nsContentUtils::HasMutationListeners(...)) ? > I did ask that before, and actually didn't get any answer why you're > explicitly using window->HasMutationListeners and not the > helper method nsContentUtils::HasMutationListeners Ah, I misunderstood the question. I can't call nsContentUtils::HasMutationListeners and pass in the parent node, since even if the parent doesn't have a listener, one of the children might. So the only time we can short-cut is if there are no listeners in the window at all. > > virtual nsresult SetTextContent(const nsAString& aTextContent) > > { > >- // Batch possible DOMSubtreeModified events. > >- mozAutoSubtreeModified subtree(GetOwnerDoc(), nsnull); > >- return nsContentUtils::SetNodeTextContent(this, aTextContent, PR_FALSE); > >+ return nsContentUtils::SetNodeTextContent(this, aTextContent, PR_FALSE, > >+ PR_TRUE); > missing one space before 'return' > > > >+ // Batch possible DOMSubtreeModified events. > >+ mozAutoSubtreeModified subtree(doc, nsnull); > >+ > >+ FireNodeRemovedForChildren(); > >+ > > // This BeginUpdate/EndUpdate pair is important to make us reenable the > > // scriptloader before the last EndUpdate call. > > mozAutoDocUpdate updateBatch(doc, UPDATE_CONTENT_MODEL, PR_TRUE); > > > >- // Batch possible DOMSubtreeModified events. > >- mozAutoSubtreeModified subtree(doc, nsnull); > >- > > // Remove childnodes > >- nsContentUtils::SetNodeTextContent(this, EmptyString(), PR_FALSE); > >+ nsContentUtils::SetNodeTextContent(this, EmptyString(), PR_FALSE, PR_FALSE); > > So why can't ::SetInnerHTML reuse SetNodeTextContent's DOMNodeRemoved > dispatching? > Why does it need to have the separate FireNodeRemovedForChildren()? > I assume because of mozAutoDocUpdate? Some comment would be good. Since I want to reuse the mozAutoDocUpdate update-batch. So I have to start the batch before calling nsContentUtils::SetNodeTextContent, which means that by the time we get into nsContentUtils::SetNodeTextContent we already have scriptblockers and so can't fire the events. Will add a comment. > nsresult > nsContentUtils::SetNodeTextContent(nsIContent* aContent, > const nsAString& aValue, > - PRBool aTryReuse) > -{ > + PRBool aTryReuse, > + PRBool aFireNodeRemoved) > +{ > + // Fire DOMNodeRemoved mutation events before we do anything else. > + nsCOMPtr<nsIDocument> owningDoc; > + nsCOMPtr<nsIContent> owningContent; > + > + nsIDocument* doc = aContent->GetOwnerDoc(); > + > + // Batch possible DOMSubtreeModified events. > + mozAutoSubtreeModified subtree(doc, nsnull); > This addrefs/releases doc, so there will be probably a > measurable slow down in innerHTML and textContent > Sorry for complaining nsCOMPtr usage, but that is what I've tried to > optimize in innerHTML and event handling and elsewhere. > mozAutoSubtreeModified has UpdateTarget, which can be used to optimize > addrefing/releasing, so that > those happen only when some mutation events are actually dispatched. No it doesn't. It only addrefs/releases if window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED) returns true.

(In reply to comment #27) > Why can you remove this? Because the method is only called in such cases > that doc == aParent->GetOwnerDoc() is guaranteed? > Could you add an assertion that aNode doesn't have parent > (so that AdoptNode doesn't fire DOMNodeRemoved) Because no scripts should run during the adoption since the node should already have been removed from its parent and any needed DOMNodeRemoved events should already have fired. Such an assertion is already added in the "Part 1" patch.

(In reply to comment #29) > > nsresult > > nsContentUtils::SetNodeTextContent(nsIContent* aContent, > > const nsAString& aValue, > > - PRBool aTryReuse) > > -{ > > + PRBool aTryReuse, > > + PRBool aFireNodeRemoved) > > +{ > > + // Fire DOMNodeRemoved mutation events before we do anything else. > > + nsCOMPtr<nsIDocument> owningDoc; > > + nsCOMPtr<nsIContent> owningContent; > > + > > + nsIDocument* doc = aContent->GetOwnerDoc(); > > + > > + // Batch possible DOMSubtreeModified events. > > + mozAutoSubtreeModified subtree(doc, nsnull); > > This addrefs/releases doc, so there will be probably a > > measurable slow down in innerHTML and textContent > > Sorry for complaining nsCOMPtr usage, but that is what I've tried to > > optimize in innerHTML and event handling and elsewhere. > > mozAutoSubtreeModified has UpdateTarget, which can be used to optimize > > addrefing/releasing, so that > > those happen only when some mutation events are actually dispatched. > > No it doesn't. It only addrefs/releases if > window->HasMutationListeners(NS_EVENT_BITS_MUTATION_NODEREMOVED) returns true. I don't understand this. mozAutoSubtreeModified sure has nsCOMPtr and it addref/release doc.

Ah, I thought you were referring to the nsCOMPtrs above. Fixed in patch coming up shortly.

(In reply to comment #28) > > nsGenericElement::Normalize() ... > > // ensure that if the text node is empty, it is removed > > if (0 == child->TextLength()) { > >+ if (hasRemoveListeners) { > >+ nsContentUtils::MaybeFireNodeRemoved(child, this, doc); > >+ } > > result = RemoveChildAt(index, PR_TRUE); > So, if DOMNodeRemoved moved the child to elsewhere, wrong node is removed here. It's already the case that if DOMNodeRemoved handlers change around the DOM during normalize() calls we remove all sorts of wrong nodes since the whole thing is based on indexes. > > if (didSanitize) { > > // Replace the style element content with its sanitized style text > >- nsContentUtils::SetNodeTextContent(style, sanitizedStyleText, PR_TRUE); > >+ // The node should really be empty since we haven't added anything > >+ // to it, so the last argument shouldn't matter. But just in case, > >+ // tell it not to fire any. > >+ nsContentUtils::SetNodeTextContent(style, sanitizedStyleText, PR_TRUE, > >+ PR_FALSE); > So we don't dispatch mutation events in this case. Why? I can't think of a way to do it safely. Can you? Note that I don't think there is a way for this to happen. During fragment parsing I don't think we ever hand out a reference to the fragment to script, and so it won't get a chance to poke contents into the style element. Additionally, I don't think the sanitizing parser is ever exposed to script anyhow. An alternative solution is to pass PR_TRUE here and rely on MaybeFireNodeRemoved to stop any dangerous mutation-event-firing from happening. That has the advantage that it asserts if we somehow do get into the situation of this call actually removing nodes. That also has the additional advantage that I can revert and remove the aFireNodeRemoved argument completely and make SetInnerHTML simply remove the nodes manually. That's a very small amount of code anyhow, and likely somewhat faster.

Comment on attachment 527838 [details] [diff] [review] Part 2: Fix UserDataHandler for adoptNode v3 Peter, could you have a look at this? I'll ping you for part 1 once that has Smaugs review.

Comment on attachment 527837 [details] [diff] [review] Part 1: Fix mutation events v3 >+nsPLDOMEvent::nsPLDOMEvent(nsINode *aEventNode, nsEvent &aEvent) >+ : mEventNode(aEventNode), mDispatchChromeOnly(PR_FALSE) >+{ >+ nsEventDispatcher::CreateEvent(nsnull, &aEvent, EmptyString(), >+ getter_AddRefs(mEvent)); >+ NS_ASSERTION(mEvent, "Should never fail to create an event"); >+ nsCOMPtr<nsIPrivateDOMEvent> priv = do_QueryInterface(mEvent); >+ NS_ASSERTION(priv, "Should also not fail to QI to nsIDOMEventPrivate"); >+ priv->DuplicatePrivateData(); >+} DuplicatePrivateData makes the event untrusted. So, before creating the DOM Event, take the trust-ness bit PRBool trusted = NS_IS_TRUSTED_EVENT(&aEvent); and then after DuplicatePrivateDate() call priv->SetTrusted(trusted); And please add a test for mutation event trust-ness.

The tests caught a bug in the cleanup patch. Still not requesting review on this though.

Fix a missing #include in the review last fixes.

Comment on attachment 528200 [details] [diff] [review] Tests rs=me

Comment on attachment 527838 [details] [diff] [review] Part 2: Fix UserDataHandler for adoptNode v3 Review of attachment 527838 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsContentUtils.h @@ +932,5 @@ > + /** > + * Synchronously fire DOMNodeRemoved on aChild. Only fires the event if > + * there really are listeners by checking using the HasMutationListeners > + * function above. The function makes sure to hold the relevant objects alive > + * for the duration of the event firing. However there are no guarentees guarantees @@ +937,5 @@ > + * that any of the objects are alive by the time the function returns. > + * If you depend on that you need to hold references yourself. > + * > + * @param aChild The node to fire DOMNodeRemoved at. > + * @param aParent aChilds parent that aChild is about to be removed from. I'd just say |the parent of aChild| @@ +938,5 @@ > + * If you depend on that you need to hold references yourself. > + * > + * @param aChild The node to fire DOMNodeRemoved at. > + * @param aParent aChilds parent that aChild is about to be removed from. > + * @param aOwnerDoc aChilds owner document. the ownerDocument of aChild ::: content/base/src/nsDocument.cpp @@ +6198,4 @@ > > if (adoptedNode->GetOwnerDoc() != this) { > return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; > } Can't you change this to an assertion too?

> ::: content/base/src/nsDocument.cpp > @@ +6198,4 @@ > > > > if (adoptedNode->GetOwnerDoc() != this) { > > return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; > > } > > Can't you change this to an assertion too? Yes! Will do.

Comment on attachment 528202 [details] [diff] [review] Part 1: Fix mutation events v3.1 Review of attachment 528202 [details] [diff] [review]: ----------------------------------------------------------------- There really is no way to implement NodeRemoved sanely, it's trivial to make any method that removes nodes to remove the wrong ones :-(. ::: content/base/src/nsContentUtils.cpp @@ +3624,5 @@ > // This relies on nsEventListenerManager::AddEventListener, which sets > // all mutation bits when there is a listener for DOMSubtreeModified event. > if (window && !window->HasMutationListeners(aType)) { > return PR_FALSE; > } I think it'd make sense to add a nsContentUtils::MayHaveMutationListeners(nsIDocument *aDoc) that checks the inner window, instead of copying this code all over.

(In reply to comment #46) > There really is no way to implement NodeRemoved sanely, it's trivial to make > any method that removes nodes to remove the wrong ones :-(. Yup, that's basically it. We can do better, but I'm not sure it's worth it. We should push hard to deploy the replacement instead, which would fire after the node is removed which can be made much more sane.

FIXED!!! http://hg.mozilla.org/mozilla-central/rev/d2ebd2fa29a1 http://hg.mozilla.org/mozilla-central/rev/e9ded5c5e0b7 http://hg.mozilla.org/mozilla-central/rev/ad2f423c9237 http://hg.mozilla.org/mozilla-central/rev/1447c689162d I'll file a separate bug for the cleanup patch.