641821 - Implement mutation events replacement (sync approach) (using moz prefix)

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

http://lists.w3.org/Archives/Public/public-webapps/2009AprJun/0745.html

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Patch coming for comments.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Jonas, what do you think about this.
The API is a bit simpler than what you proposed and what was documented in
http://www.w3.org/2008/webapps/wiki/MutationReplacement#MutationTarget_.28A_Mozilla_Proposal.29
In the patch the callback takes two parameters. One is the node to which the 
callback was registered, and the other is the changeTarget.
That way there is no need for those *Subtree* callbacks.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I uploaded the patch to tryserver.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a perf test

This is an artificial perftest.
Just to get notified that some attribute has changed is 4x faster with the
new setup than using mutation events.
But of course the new API lacks many of the features of the mutation events.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 540341 [details] [diff] [review]
patch


>+nsINode::RemoveMutationCallback(nsIAtom* aCallbackName, nsIDOMMozMutationCallback* aCallback)
>+{
>+  MutationObserverForCallbacks* observer =
>+    static_cast<MutationObserverForCallbacks*>(GetProperty(aCallbackName));
>+  if (observer) {
>+    observer->mCallbacks.RemoveObject(aCallback);
This should check if mCallbacks is empty and if so, remove the mutationobserver
and the delete the relevant property from node.

if (!observer->mCallbacks.Count()) {
  RemoveMutationObserver(observer);
  DeleteProperty(aCallbackName);
}

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

...that way removing all the callbacks removes also all the performance penalty.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Olli, could I see the actual numbers for that perftest, with the addition of a control timing the same operations with no observation of the attr change?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test case 2

Raw attribute setting 345ms
Mutation callbacks 1479ms
mutation events 6148ms

I profiled callbacks a bit.
Based on sysprof about 10% time is spent
in CX pushing. 
Native2JS conversion takes also about 10%.

nsCOMArray::RemoveObjectsAt takes 5%. This is coming from ScriptRunner handling.

JS_SetOptions takes 2%. That is surprising.
That is coming from nsXPCWrappedJSClass::CallMethod

I guess we leave the trace if setting attribute ends up calling
event listener or mutation callback.
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#1358

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> nsCOMArray::RemoveObjectsAt takes 5%. This is coming from ScriptRunner
> handling.
Er, things under nsCOMArray::RemoveObjectsAt take that 5%.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, the ordering when the callbacks are called needs tweaking.
I want them to happen in a queue always, but scriptrunners don't guarantee
that.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This has a queue of MutationCallbackRunners, so that mutations are
always handled in the order they happen.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Based on sysprof about 10% time is spent in CX pushing. 

Hmm.  I wonder how we can make that faster... it comes up other places too.

> Native2JS conversion takes also about 10%.

What things are being converted there?

> I guess we leave the trace if setting attribute ends up calling event listener
> or mutation callback.

Indeed.  What do those numbers look like if you turn off tjit to start with, so the whole thing is running under mjit?

And yes, that script running thing sucks.  Is that just the mutation callback scriptrunner?

But in general, those numbers look not too bad, given that we can work on improving them.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

The "nsCOMArray::RemoveObjectsAt takes 5%. This is coming from ScriptRunner handling." seems to be coming from mutation callback scriptrunner.

I'll try disabling tjit.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

dispatching tjit doesn't affect with the testcase.

Comment 15

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

Here's a few comments on the API:

I think almost anyone that registers for TextDataChanged is going to end up also registering for ChildlistChanged. It seems like the most common use case for TextDataChanged is wanting to track the textual contents of a specific node. This means that you'll also want to know when TEXT_NODEs are added and removed.

So I think we should change TextDataChanged to TextContentsChanged and make that get called whenever textnodes under the registered node are modified, added or removed.

(I'm not sure where that leaves changes to comment and PI nodes. I'd be happy to not support mutation notifications for them for now, that doesn't seem like a very common use case).


As for ChildlistChanged, we might end up having to provide more fine grained notifications. ChildlistChanged definitely deals a lot better with things like the innerHTML setter, however I'm worried that we'll end up simply forcing websites to do all the work we're saving. But it's a place to start. I'm fine with just having this for now and add more detailed notifications if people ask for it.


Same with AttributeChanged. I suspect that it'll be very common to only want to listen to attribute changes for a specific element. After all, that's what we do internally for the vast majority of cases. But it's probably a good idea to start with what you have and only if it turns out that people use it a lot in such a way that they only want notifications for a single element, then we can add API for that.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> Here's a few comments on the API:
> 
> I think almost anyone that registers for TextDataChanged is going to end up
> also registering for ChildlistChanged. It seems like the most common use
> case for TextDataChanged is wanting to track the textual contents of a
> specific node. This means that you'll also want to know when TEXT_NODEs are
> added and removed.
That is why there is currently DOMCharacterDataModified and DOMNodeInserted/Removed.
I wouldn't want to merge two listener types together. If script libraries need that, 
it is trivial to do it in scripts.

Comment 17

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

I wasn't suggesting merging them, I was suggesting that we change when the textdata notification was sent, but keep the child list one as-is. If almost all users of the textdata notification ends up having to listen to the childlist one as well, then the API is poorly designed and we're not helping authors as much as we could.

Comment 18

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

But, in case that wasn't clear. I'm fine with landing this as-is and taking the discussion about API details to the list.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

What would be the changetarget for the TextContentsChanged?
The API would be quite strange it the target could be an element in some
cases and in some cases a textnode.
And if the target would be always an element, then it would become quite difficult
to track which textnode was changed.

Comment 20

[Alex Vincent [:WeirdAl]]

I still need some way to determine before & after states, in order to extract nsITransaction objects from mutations.  (If I could get nsITransaction objects directly, that'd be freaking awesome.  I figured out how to construct nsITransaction objects for DOM mutations years ago.)

Reason:  An editor can use nsITransactionManager to track changes for undo/redo operations, but that takes nsITransaction objects.  The nsITransaction* API's are surprisingly powerful.

smaug suggests having a shadow copy of the DOM being tracked, which is doable, but still complicates things a bit.  I'd have to update the shadow with each change.  (Plus, I'm already bootstrapping my "custom DOM bindings" concept with shadow copies of the DOM.  Its complexity is already pretty staggering, and I'm thinking about a different approach altogether.)

Comment 21

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

(In reply to comment #19)
> What would be the changetarget for the TextContentsChanged?
> The API would be quite strange it the target could be an element in some
> cases and in some cases a textnode.
> And if the target would be always an element, then it would become quite
> difficult
> to track which textnode was changed.

Indeed, that is a problem. I really do suggest we bring this discussion to the webapps list though.

(In reply to comment #20)
> I still need some way to determine before & after states, in order to
> extract nsITransaction objects from mutations.

Why not use nsIMutationObserver directly?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Why not use nsIMutationObserver directly?

It's not scriptable.

Comment 23

[David Flanagan [:djf]]

For my purposes (dom.js) I need to be able to generate fine-grained mutation events for communication with a renderer running in a separate process.  The separate process means that changes to the tree need to be serialized.  I'd love to use a standard mutation event mechanism, but the ChildListChanged listener is too coarse for my purposes. dom.js will have a copy of the document tree, and the renderer will have its own copy of the tree.  I need to be able to tell the renderer additions and removals of individual nodes. It is not enough to just say "something has changed in the child list" because then I'd have to send the entire child list to the renderer process, including all the unchanged children. 

My requirements may simply be beyond the scope of the proposal, but I wanted to mention them, at least.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #23)
> It is not enough to just say "something has changed in the child list"
> because then I'd have to send the entire child list to the renderer process,
> including all the unchanged children. 
Why?
If dom.js (whatever it does) has its copy of DOM, it can compare the child
lists before sending any data to some other process.

But, ofc, if you have concrete suggestions how to improve the API,
all comments are very welcome :)

Comment 25

[David Flanagan [:djf]]

(In reply to comment #24)
> (In reply to comment #23)
> > It is not enough to just say "something has changed in the child list"
> > because then I'd have to send the entire child list to the renderer process,
> > including all the unchanged children. 
> Why?
> If dom.js (whatever it does) has its copy of DOM, it can compare the child
> lists before sending any data to some other process.

dom.js is a DOM implementation in JavaScript.  It doesn't have a copy of the DOM, it *is* the DOM.  So I'll be implementing these mutation events.  It would be really sweet if the renderer could simply listen for standard mutation events on the document it is rendering.  Then I wouldn't have to implement a public coarse-grained mutation API and a private fine-grained API.

Its not a big deal, but I wanted to mention this use case for finer-grained events. 

> But, ofc, if you have concrete suggestions how to improve the API,
> all comments are very welcome :)

I'll be posting a few responses to public-webapps.

Comment 26

[Boris Zbarsky [:bzbarsky]]

David, your needs quite different from this proposal.  In particular, you have one dedicated mutation listener which guarantees that it will NOT modify the state of the DOM synchronously from inside the mutation notification.  The API being designed here needs to be able to deal with listeners which perform such modifications.

Note that this is exactly why we have a private fine-grained API for notifying layout of DOM changes right now (nsIMutationObserver).  This API places significant constraints on what the observer implementations are allowed to do, of course, which is not practical for a public API.

Comment 27

[Mihai Sucan [:msucan]]

For the developer tools we are working on we need to monitor page changes, mutations in the document. For example for the web page inspector tool we are we have an old patch in bug 566085 that we want to resurrect - to update it and land it into Firefox.

In the old patch we used DOM mutation events, but we would prefer an API that performs better.

I would like to know what is the status of the work being done in this bug? Is this API something we should look into? Or is it too early?

Thank you!

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

All the comments about the API would be great.

The API and a similar proposal from Google are actively discussed in
W3C WebApps WG mailing list. I would assume, or hope, that the final API
is a merge of those two proposals.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

And note, Fx internal tools could always use nsIMutationObserver API, which
provides more than what we want to expose to web.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> And note, Fx internal tools could always use nsIMutationObserver API,

If they use a binary component and are very careful, yes.

Comment 31

[Mihai Sucan [:msucan]]

As far as I know, we want to stay away from binary components, that's why this API is of a lot of interest to us.

Comment 32

[Mihai Sucan [:msucan]]

Did some testing with this patch. The way we would use this API for the page Inspector developer tool would be as follows: we would add mutation change listeners for the root of the document. Something as follows:

document.mozAddChildlistChangedListener(onChildlistChanged);
document.mozAddAttributeChangedListener(onAttributeChanged);
document.mozAddTextDataChangedListener(onTextDataChanged);

(It would not be "productive" to add change listeners only for some nodes in the document. We need a "catch all" approach.)

Given the following code:

function onChildlistChanged(aNode, aTarget) {
  // ...
}

aNode holds a reference back to the document object. aTarget is a reference to the element that has a new/deleted child.

Given a big tree it would not be efficient for us to check which element is new/removed. Could we also get a reference to the new/removed node? And a flag that tells us if it's a removal or addition.

Given the following code:

function onAttributeChanged(aNode, aTarget) {
  // ...
}

aNode holds a reference back to the document object. aTarget is a reference to the element where an attribute change happened. This is useful, but can we also get a reference to the new/changed/removed attribute node itself? Also, a flag that indicates what happened (ADD/REMOVE/CHANGE) would be useful.

function onTextDataChanged(aNode, aTarget) {
  // ...
}

aNode holds a reference back to the document object. aTarget is a reference to the Text/Comment node that changed. This is sufficient for us.

(It would be fancy to get the old attribute value when a change happens, or the old text/comment node content, but these are not really important for our needs.)

I am asking for these API improvements simply because we want to avoid performance bottlenecks in our devtools as well (if it's possible, of course). Even with the current implementation it would seem we could use this API, but anything that the platform can do to make our code lighter is welcome.

Any thoughts? Thank you!

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

The current idea is to give a "list of changes" to the callbacks.
Jonas is, I think, writing a new proposal.
The list would contain attribute names and inserted/removed nodes, depending
on the callback type.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 540733 [details] [diff] [review]
patch

So the spec won't be quite like this, but I don't know what it will look like.
I still hope it will use the mostly-sync approach, since that way listener
handling can be sane [1].
(Or perhaps someone fixes the listener handling problem in almost-async approach)


[1]

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

[1] http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0419.html

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backup of the ModificationBatch approach

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better memory management

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Attachment: optional "dom.AlmostAsyncModificationBatch=true"

Comment 40

[Paul Rouget [:paul]]

What is the status of this bug?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

This is a top priority in my todo list. I need to do still some cycle collector related things
before finalizing my patch. Sorry for the delay.

Comment 43

[John Nagle]

Is this compatible with Google's implementation, which is in their current Google Chrome beta?  (http://www.youtube.com/watch?v=eRZ4pO0gVWw).

If not, it should be.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

It will be since I and Jonas designed the API with Chromium developers,
and the API is now part of DOM 4 draft.
The API will be probably first moz-prefixed.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backup

Seem to work reasonable well. Haven't done any performance measures though.
Passes http://code.google.com/p/mutation-summary/source/browse/ test.html
when webkit specific things in the test are fixed.
(test.html relies on various webkit bugs)

Comment 46

[John Nagle]

A useful performance test is to test it for tag "a", attribute "href". For ad-blocking programs, you usually want to monitor changes to links.  If that's efficient, ad blocker overhead will drop substantially.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

For attribute changes the patch is about 7x faster than using Mutation events.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ce971d294724/

That tryserver build does not have the API prefixed, and there are still few smaller
things to fix.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7

I need write still plenty of mochitests.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +some tests and fixes

This passes also
http://code.google.com/p/mutation-summary/source/browse/ testTreeMirror's
fuzzer.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +more tests

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +performance fixes

Not major changes, but improves transient observer cleanup, which was slow.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v11

Tiny change to sync APIs handling.
Only change in HandleMutationsInternal

Comment 55

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

Comment on attachment 609490 [details] [diff] [review]
v11

Review of attachment 609490 [details] [diff] [review]:
-----------------------------------------------------------------

I'm at nsDOMMutationObserver::GetAllSubtreeObserversFor

::: content/base/public/nsContentUtils.h
@@ +1599,5 @@
>     * will return viewport information that specifies default information.
>     */
>    static ViewportInfo GetViewportInfo(nsIDocument* aDocument);
>  
> +  static void EnterMicroTask() { ++sMicroTaskLevel; }

Add documentation

::: content/base/public/nsINode.h
@@ +1428,5 @@
>    // Optimized way to get classinfo.
>    virtual nsXPCClassInfo* GetClassInfo() = 0;
>  
> +  void BindObject(nsISupports* aObject);
> +  void UnbindObject(nsISupports* aObject);

Document what these do.

::: content/base/src/nsContentUtils.cpp
@@ +4835,5 @@
> +nsContentUtils::LeaveMicroTask()
> +{
> +  if (--sMicroTaskLevel == 0) {
> +    nsDOMMutationObserver::HandleMutations();
> +  }

It *might* be worth inlineing this. The inlined code isn't a whole lot bigger than a plain function call.

Up to you.

@@ +6606,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  JSString* str = nsnull;
> +  JS::Anchor<JSString *> deleteProtector(str);

Hmm.. I don't actually know what this JS::Anchor string is needed or what it does. Could you get someone else to verify that? (I'd also love to know when it's needed since I think we have a lot of code that uses JSString*s without them)

@@ +6619,5 @@
> +    if (!depStr.init(aCx, str)) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +
> +    nsCOMPtr<nsIAtom> a = do_GetAtom(depStr);

You technically should OOM check 'a'.

::: content/base/src/nsDOMMutationObserver.cpp
@@ +65,5 @@
> +  aType = mType;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP nsDOMMutationRecord::GetTarget(nsIDOMNode** aTarget)

Linebreak after the macro

I think it would be worth writing custom quick-stubs for the various methods that return nsIDOMNodes. That way we can avoid QIing to nsIDOMNode and back to nsINode. I forget if we could use the same trick for nsINodeList too.

@@ +68,5 @@
> +
> +NS_IMETHODIMP nsDOMMutationRecord::GetTarget(nsIDOMNode** aTarget)
> +{
> +  nsCOMPtr<nsIDOMNode> n = do_QueryInterface(mTarget);
> +  *aTarget = n.forget().get();

Just use CallQueryInterface. At the very least use n.forget(aTarget)

@@ +173,5 @@
> +               m->mAttrName.Equals(nsDependentAtomString(aAttribute)),
> +               "Wrong attribute!");
> +  if (!m->mTarget) {
> +    m->mTarget = aElement;
> +    m->mAttrName = nsDependentAtomString(aAttribute);

Actually, it would probably be faster to use nsAtomString. It will involve more buffer refcounting, but it will mean being able to share the actual string-data buffer which is definitely faster since there's a malloc and memcpy less.

@@ +178,5 @@
> +    if (aNameSpaceID == kNameSpaceID_None) {
> +      m->mAttrNamespace.SetIsVoid(true);
> +    } else {
> +      nsContentUtils::NameSpaceManager()->GetNameSpaceURI(aNameSpaceID,
> +                                                          m->mAttrNamespace);

We should only observe null-namespaced attributes when filters are applied. If we want support for specific namespaced attributes (which I don't think we do) then we should do it properly such that you define both localName+namespace when you specify a filter.

The current way this works will just lead to bugs because if someone observers the "value" attribute, they won't check to filter out changes to "xlink:value" attributes. I.e. they will treat "xlink:value" just like "value" which is wrong.

I would say let's punt on namespace support for now. If someone asks for it, we can always add it later.

@@ +185,5 @@
> +
> +  if (AttributeOldValue() && m->mPrevValue.IsVoid()) {
> +    nsAutoString v;
> +    if (aElement->GetAttr(aNameSpaceID, aAttribute, v)) {
> +      m->mPrevValue = v;

would the following work:

if (!aElement->GetAttr(aNameSpaceID, aAttribute, m->mPrevValue)) {
  m->mPrevValue.SetIsVoid();
}

@@ +188,5 @@
> +    if (aElement->GetAttr(aNameSpaceID, aAttribute, v)) {
> +      m->mPrevValue = v;
> +    }
> +  }
> +  Observer()->ScheduleForRun();

Can't this call happen automatically when CurrentRecord is called?

@@ +224,5 @@
> +                                    nsIContent* aFirstNewContent,
> +                                    PRInt32 aNewIndexInContainer)
> +{
> +  nsINode* parent = aContainer ?
> +    static_cast<nsINode*>(aContainer) : static_cast<nsINode*>(aDocument);

Use the NODE_FROM macro

@@ +232,5 @@
> +  }
> +
> +  if (nsAutoMutationBatch::IsBatching()) {
> +    if (!nsAutoMutationBatch::UpdateObserver(Observer(), wantsChildList)) {
> +      nsAutoMutationBatch::SetObserver(Observer(), wantsChildList);

Shouldn't you bail if parent != nsAutoMutationBatch::GetBatchTarget()?

@@ +264,5 @@
> +                                    nsIContent* aChild,
> +                                    PRInt32 aIndexInContainer)
> +{
> +  nsINode* parent = aContainer ?
> +    static_cast<nsINode*>(aContainer) : static_cast<nsINode*>(aDocument);

Use the NODE_FROM macro

@@ +272,5 @@
> +  }
> +
> +  if (nsAutoMutationBatch::IsBatching()) {
> +    if (!nsAutoMutationBatch::UpdateObserver(Observer(), wantsChildList)) {
> +      nsAutoMutationBatch::SetObserver(Observer(), wantsChildList);

Shouldn't you bail if parent != nsAutoMutationBatch::GetBatchTarget()?

@@ +303,5 @@
> +    return;
> +  }
> +
> +  nsINode* parent = aContainer ?
> +      static_cast<nsINode*>(aContainer) : static_cast<nsINode*>(aDocument);

Use the NODE_FROM macro

@@ +337,5 @@
> +  nsTArray<nsMutationObserver*> allObservers;
> +  Observer()->GetAllSubtreeObserversFor(Target(), allObservers);
> +  if (allObservers.Length()) {
> +    nsCOMArray<nsMutationObserver>* tmpObservers = nsnull;
> +    Observer()->mTmpObservers.Get(aChild, &tmpObservers);

Rename to mTransientObservers?

::: content/base/src/nsDOMMutationObserver.h
@@ +440,5 @@
> +    for (PRUint32 i = 0; i < l; ++i) {
> +      if (sCurrentBatch->mObservers[i] == aObserver) {
> +        if (aWantsChildList) {
> +          sCurrentBatch->mWantsChildList[i] = aWantsChildList;
> +        } 

Make this an array of structs instead.

Comment 56

[Josh Matthews [:jdm]]

>+  JSString* str = nsnull;
>+  JS::Anchor<JSString *> deleteProtector(str);
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    jsval v;
>+    if (!JS_GetElement(aCx, obj, i, &v) ||
>+        !(str = JS_ValueToString(aCx, v))) {
>+      return NS_ERROR_ILLEGAL_VALUE;
>+    }
>+
>+    nsDependentJSString depStr;
>+    if (!depStr.init(aCx, str)) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    nsCOMPtr<nsIAtom> a = do_GetAtom(depStr);
>+    aRetVal.AppendObject(a);
>+  }

Yes, this is correct. Technically, nsDependentJSString::Init should have also have an Anchor, as it accesses the underlying jschar* - I would file a bug about that, except that any use of nsDependentJSString which doesn't already use an Anchor would still be wrong, so it would just be wasted effort.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

nsDependentJSString::Init was discussed in another bug and it doesn't need Anchor.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v12

Jonas, you can continue review from this.
I renamed things and added some comments etc.

Comment 59

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

Comment on attachment 610588 [details] [diff] [review]
v12

Review of attachment 610588 [details] [diff] [review]:
-----------------------------------------------------------------

I think that the spec for replaceChild(someChild, someFragment) should be changed such that we first create one record which removes all children from the fragment, and then create a record which replaces the current child with the new list of children. I think that's what we might end up doing with the current code, but I'm not sure if that's what the spec actually mandates.

::: content/base/src/nsDOMMutationObserver.cpp
@@ +304,5 @@
> +      // context node, but needs to move elements around.
> +      return;
> +    }
> +    if (nsAutoMutationBatch::GetBatchTarget() != parent) {
> +      return;

Could potentially merge this if-statement with the above one. Up to you.

@@ +317,5 @@
> +    return;
> +  }                                                                   
> +
> +  nsMutationReceiver* orig = GetParent() ? GetParent() : this;
> +  if (Observer()->GetReceiverFor(aChild, false) != orig) {

Needs comment!

@@ +327,5 @@
> +      Observer()->mTransientReceivers.Put(aChild, transientReceivers);
> +    } else {
> +      for (PRInt32 i = 0; i < transientReceivers->Count(); ++i) {
> +        nsMutationReceiver* r = transientReceivers->ObjectAt(i);
> +        if (r == orig || r->GetParent() == orig) {

remove the r==orig part. It can never be true since r is always a transient observer and orig is always a transient observer.

@@ +426,5 @@
> +    if (n->MayHaveDOMMutationObserver()) {
> +      nsMutationReceiver* r = GetReceiverFor(n, false);
> +      if (r && r->Subtree() && !aReceivers.Contains(r)) {
> +        aReceivers.AppendElement(r);
> +        if (mReceivers.Count() == PRInt32(aReceivers.Length())) {

You can remove this if-statement. The same thing is checked further down.

@@ +440,5 @@
> +            aReceivers.AppendElement(parent);
> +          }
> +        }
> +      }
> +      if (mReceivers.Count() == PRInt32(aReceivers.Length())) {

Add a comment describing this optimization

@@ +711,5 @@
> +  return mCurrentMutations[last];
> +}
> +
> +bool
> +nsDOMMutationObserver::HasCurrentRecord(const nsAString& aType)

Not used any more

@@ +744,5 @@
> +  ++sMutationLevel;
> +}
> +
> +void
> +nsDOMMutationObserver::LeaveMutationHandling()

Document that all this is doing is to remove the appropriate mutation-records from mCurrentMutations as needed.

@@ +801,5 @@
> +
> +void
> +nsAutoMutationBatch::Done()
> +{
> +  if (sCurrentBatch == this) {

Flip this around to be an early-return instead, so that you can reduce indentation of the whole function

@@ +838,5 @@
> +          }
> +          for (PRUint32 k = 0; k < allObservers.Length(); ++k) {
> +            nsMutationReceiver* r = allObservers[k];
> +            nsMutationReceiver* orig = r->GetParent() ? r->GetParent() : r;
> +            if (ob->GetReceiverFor(removed, false) != orig) {

This is the same thing where you are avoiding adding a transient receiver where the main receiver already exists, right? It seems like it might be worth factoring this part out maybe.

::: content/base/src/nsDOMMutationObserver.h
@@ +48,5 @@
> +  nsCOMPtr<nsINode>             mPreviousSibling;
> +  nsCOMPtr<nsINode>             mNextSibling;
> +};
> + 
> +// *Base class just hides the implementation of getters and setters.

Could you say something like: "Base class just prevents direct access to members to make sure we go through getters/setters"

@@ +62,5 @@
> +  bool Subtree() { return mParent ? mParent->Subtree() : mSubtree; }
> +  void SetSubtree(bool aSubtree) {
> +    mSubtree = aSubtree;
> +    if (!mSubtree) {
> +      for (PRInt32 i = 0; i < mTransientReceivers.Count(); ++i) {

Do you need to actually make the transient observers stop observing for mutation events?

@@ +75,5 @@
> +  bool CharacterData()
> +  {
> +    return mParent ? mParent->CharacterData() : mCharacterData;
> +  }
> +  void SetCharacterData(bool aCharacterData) { mCharacterData = aCharacterData; }

In all the Set* methods, you could assert that mParent is null.

@@ +164,5 @@
> +      return false;
> +    }
> +
> +    nsCOMArray<nsIAtom>& filters = AttributeFilter();
> +    for (PRInt32 i = 0; i < filters.Count(); ++i) {         

Just do |return filters.Contains(aAttr);|

@@ +241,5 @@
> +    }
> +    if (mTarget && mObserver) {
> +      mTarget->UnbindObject(mObserver);
> +    }
> +    if (mParent) {

This stuff seems like it can be simplified.

@@ +451,5 @@
> +
> +
> +  static nsINode* GetBatchTarget() { return sCurrentBatch->mBatchTarget; }
> +
> +  static void NodeRemoved(nsIContent* aChild)

Document that this is called *while* the nodes are being removed.

@@ +462,5 @@
> +      }
> +    }
> +  }
> +
> +  void NodesAdded()

Document that this is called *after* the nodes have all been inserted.

@@ +471,5 @@
> +                       mBatchTarget->GetFirstChild();
> +      for (; c != mNextSibling; c = c->GetNextSibling()) {
> +        sCurrentBatch->mAddedNodes.AppendElement(c);
> +      }
> +      sCurrentBatch->Done();

Nit: I would remove "sCurrentBatch->" from these two places.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +913,5 @@
>    mozAutoSubtreeModified subtree(doc, nsnull);
>  
>    nsresult rv;
>    // Parse directly into destination if possible
> +  if (doc->IsHTML() && !OwnerDoc()->MayHaveDOMMutationObservers() &&

I suspect this isn't needed, but we should check with Henri. Please file a followup.

::: dom/base/nsDOMClassInfo.cpp
@@ +2393,5 @@
>  
> +  nsCOMPtr<nsIXPCFunctionThisTranslator> mctl = new nsMutationCallbackThisTranslator();
> +  NS_ENSURE_TRUE(elt, NS_ERROR_OUT_OF_MEMORY);
> +
> +  sXPConnect->SetFunctionThisTranslator(NS_GET_IID(nsIMutationCallback),

Rename nsIDOMMutationCallback or nsIMutationObserverCallback or some such.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #59)
> 
> You can remove this if-statement. The same thing is checked further down.
The whole point is to do simple optimization as early as possible, before
any extra hashtable lookup.
> 
> This is the same thing where you are avoiding adding a transient receiver
> where the main receiver already exists, right? It seems like it might be
> worth factoring this part out maybe.
It is similar, yet different enough that factoring it out is a bit difficult.
> Just do |return filters.Contains(aAttr);|
Except that nsCOMArray doesn't have Contains

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v19

Waiting for tryserver results

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/83dce280d871

I'll file followups a bit later.

Comment 63

[Jesse Ruderman]

I see several spec links. Which should I pay attention to?

http://www.w3.org/2008/webapps/wiki/MutationReplacement#MutationTarget_.28A_Mozilla_Proposal.29

http://www.w3.org/TR/dom/#mutation-observers

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers

Or should I just work from the test https://hg.mozilla.org/mozilla-central/file/83dce280d871/content/base/test/test_mutationobservers.html ?

Are we planning to remove the old mutation events?

Comment 64

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jesse Ruderman from comment #63)
> I see several spec links. Which should I pay attention to?

<http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers> is the one.

> Are we planning to remove the old mutation events?

Hopefully, at some point.

Comment 65

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

assigned to Jesse for fuzzing as he marked the bug, Jess if we need more than fuzzing on this let me know

Comment 66

[Jesse Ruderman]

I've added the feature to my fuzzer and haven't found many bugs.

I'm happy that native anonymous nodes are excluded.

What happens if someone forgets an nsAutoMicroTask (or nsAutoMutationBatch or nsAutoSyncOperation)? Do assertions fail right away? Could it lead to security holes?  How do we know which code needs those autos?

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jesse Ruderman from comment #66)
> What happens if someone forgets an nsAutoMicroTask
MutationObserver callback is called at wrong time. Either too early or too late, but
always at safe time.

(or nsAutoMutationBatch
this has only to do with MutationRecords. So if someone doesn't use it correctly, it is
possible that too many records are created.

> or nsAutoSyncOperation)?
It is possible to run the callbacks during sync operation, like it is now possible to
run scripts.

> How do we know which code needs those autos?
nsAutoMicroTask is for cases when there isn't necessarily JS on stack and C++ calls JS (content JS for now)
nsAutoMutationBatch is tightly for DOM tree management, so that right kinds of records are created.
nsAutoSyncOperation is for cases when we try to emulate sync operations (we don't actually have such).

Comment 68

[Eric Shepherd [:sheppy]]

Docs note: see this blog post: http://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-changes-without-killing-browser-performance/