90983 - Don't fire mutation events during initial page load

Status: RESOLVED FIXED

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

Description

[Daniel Bratell]

2% of the parsing of a sample page is spent in 
nsGenericElement::HasMutationListeners. I guess that it is for dynamic changing 
of a page from JavaScript, but there can't be any mutation events during intial 
page load. (Can there?) So we shouldn't spend time checking for them.

Comment 1

[Fabian Guisset]

Marking P3. If the 2% number is confirmed, it would probably be cool to fix 
that.

Comment 2

[Daniel Bratell]

I don't remember all the details anymore, but the 2% is probably of the parser
time, so it will be < 1% of the total page load time. Not that there are that
many bigger fish to catch.

Comment 3

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 4

[Aaron Leventhal]

I can confirm that NodeInserted() mutation events are fired during page load.
In the accessibility library I actually have to throw these out, I only want the
ones after the page is finished loading.

Comment 5

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

I'll take this

Comment 6

[Jesse Ruderman]

I can imagine extension authors relying on mutation events firing during load.  It's easier to just listen to mutation events than to listen to both mutation events and onload, and listening to mutation events during load makes it possible for extensions to react to page content more quickly.

Comment 7

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

Attachment: Patch to fix

This makes us not fire mutations when aNotify is false. It also makes us fire mutations on nodes where IsInDoc is false as well as for modifications to the document node.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I'm a little concerned about breaking folks with this change.  Especially since we _will_ fire mutations for scripted changes during document load, right?  So up until onload fires mutation events will be wacky little things...

Bob, do you know whether mutation events are used much out there, and if so in what way?

Comment 9

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

I really don't think we want to fire mutations during loading. The only good argument I can think of if there are a lot of pages out there that depend on it already, but I doubt that's the case. Though of course it's possible.

One reason not to fire them is that if there you do register a listener parsing will slow down by *a lot*, so I don't think we should let people shoot themselfs in the foot to that extent. This is also an excellent argument for extensions to go find some other way of doing whatever they're trying to do.

I'm not too worried about inconsistencies between manual DOM mutations, and ones that happen due to parsing. I think this is something that people will understand how it works. You can always wait with doing whatever you want to do until the tree is fully parsed, I'd think most people do that already.

Comment 10

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

Attachment: patch -w

For review goodness (in case we want to do this, which I think we do).

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)
> 
> Bob, do you know whether mutation events are used much out there, and if so in
> what way?
> 

No, sorry. I don't expect to see this type of thing on the general public web, but for internal Gecko only type applications it is possible though still unlikely in my uninformed opinion.

Comment 12

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

As I said, i don't want us to be using this internally in firefox, or in extensions. So if that breaks I'm not so concerned.

There is still XULRunner apps though, but there aren't very many out there yet.

Comment 13

[Jesse Ruderman]

> This is also an excellent argument for extensions to go find some other way of
> doing whatever they're trying to do.

Suppose I wanted to write an extension to work around bug 221634.  Can you suggest a way for me to do that without using mutation listeners?

(Any hints you have for fixing bug 221634 in Firefox itself would be appreciated, too.)

Comment 14

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

I'd suggest adding an xbl binding that gets attached to all <input> elements and do your magic in the binding constructor.

If you were to use mutation events you would slow down page loading a lot, so you don't want to do that no matter what.

Comment 15

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

Alternativly, would it work to register a webprogresslistener and scan through the document using getElementsByTagName or xpath every time you get a progress notification?

Comment 16

[Jesse Ruderman]

The XBL trick could work, but it doesn't scale well to multiple extensions.

Wouldn't using getElementsByTagName on each progress notification be O(n^2)?

Comment 17

[Alex Vincent [:WeirdAl]]

Please, please consider bug 201236 (mutations in standalone documents) when reviewing this patch.  At the very least you're going to bitrot me again, this time seriously.  I don't want to start over.

On top of that, the patch to bug 201236 *does* require a document be passed into HasMutationListeners (which I move from nsGenericElement to nsContentUtils).

The patch has r=smaug, and is awaiting sr from peterv.  I'm on my knees, begging...

Comment 18

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

(In reply to comment #16)
> The XBL trick could work, but it doesn't scale well to multiple extensions.

Good point, XBL2 will solve this though. But we don't have that yet.

> Wouldn't using getElementsByTagName on each progress notification be O(n^2)?

Actually, thanks to bz, the getElementByTagName code is smart enough that it would only be O(n). It's not super fast though, but probably faster than using mutation events.

Basically the current situation is that there is no really good solution for extensions. Using mutation events will slow down pageload a lot, and getElementsByTagName is not super fast if there are a lot of elements.

Still I actually think that getElementsByTagName would work for your case.

Comment 19

[Alex Vincent [:WeirdAl]]

Patch has been bitrotted due to the checkin for bug 201236.

Comment 20

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

Attachment: Sync to tip

This just syncs to tip.

Comment 21

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

Attachment: Sync to tip -w

Comment 22

[Alex Vincent [:WeirdAl]]

Comment on attachment 221071 [details] [diff] [review]
Sync to tip -w

>Index: content/base/src/nsContentUtils.cpp
>@@ -2854,54 +2854,29 @@ PRBool
>-  NS_PRECONDITION(!aContent || aContent->GetCurrentDoc() == aDocument,
>-                  "Incorrect aDocument");
>-  if (!aDocument) {
>-    // We do not support event listeners on content not attached to documents.
>+  nsIDocument* doc = aNode->GetOwnerDoc();
>+  if (!doc) {

I found out the hard way there is a significant difference between GetCurrentDoc and GetOwnerDoc.  Namely, current doc and owner doc are the same when the node is actually a descendant of the owner document.  Detached nodes do not have a current doc.  For the purposes of the mutation listener, I think GetCurrentDoc is correct here.

I see later references that indicate you intended GetOwnerDoc() here.  However, by checking GetCurrentDoc(), we can probably save some processor time bailing out here in not iterating up the tree later in this function.

>Index: content/base/src/nsGenericElement.cpp
>===================================================================
@@ -2330,32 +2330,32 @@ nsGenericElement::doInsertChildAt(nsICon
>+    if (aNotify &&
>+        nsContentUtils::HasMutationListeners(container,
>+          NS_EVENT_BITS_MUTATION_NODEINSERTED)) {
>       nsMutationEvent mutation(PR_TRUE, NS_MUTATION_NODEINSERTED);
>       mutation.mRelatedNode = do_QueryInterface(aParent);

Per bug 331668, mRelatedNode should probably be container.

>@@ -609,24 +606,24 @@ nsSVGElement::DidModifySVGObservable(nsI
>+  if (hasListeners || IsInDoc()) {

If you take my first suggestion above, hasListeners can't be true unless IsInDoc() is true.

> nsXULElement::SetInlineStyleRule(nsICSSStyleRule* aStyleRule, PRBool aNotify)
>+  PRBool hasListeners = aNotify &&
>+    nsContentUtils::HasMutationListeners(this,
>         NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
...
>+  else if (aNotify && IsInDoc()) {

Same thing here:  if you take the first suggestion, you might be able to salvage some of the indentation.

Comment 23

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

No, I want to fire mutation events on nodes that are in a detached subtree. That change is intentional.

The comment about container is right though, thanks! I won't attach a new patch for that though.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 221071 [details] [diff] [review]
Sync to tip -w

r+sr=jst

Comment 25

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

Attachment: Patch to fix fragment insertions

I realized that the last patch on its own will make us not send mutation events during insertions of fragments. This patch makes us notify as normal when inserting fragments causing mutations to get fired correctly even with the above patch.

This seems like a good thing in general since it simplifies the code, though there is some risk that inserting huge fragments (such as when setting .innerHTML) will be slower

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224135 [details] [diff] [review]
Patch to fix fragment insertions

Looks good.  Makes me happy to see this go away!

Comment 27

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

Checked in, thanks for the reviews!

Comment 28

[Jacek Caban]

This patch has horrible effect on Wine (www.winehq.org). We use Gecko to implement mshtml.dll. For complex applications (like Photoshop installer), we had to implement our own JavaScript engine and conditional comments. I can't see any way to do this without mutation events on page load. I haven't noticed this bug before, because we used old Gecko. Now, when I have to update it, I came to this bug and it seems to be blocker for me. I guess I will have to somehow revert this patch in my build, unless you have any other suggestion, which would be very appreciated.

> If you were to use mutation events you would slow down page loading a lot, so
you don't want to do that no matter what.

I prefer working slow solution than nothing (and since our listeners are C code, the slowdown is not that bad).

Comment 29

[Boris Zbarsky [:bzbarsky]]

The slowdown comes from just dispatching the DOM event, not from the speed of execution of the listeners.

In any case, if you're haking in on the level you say you're hacking in, can you use nsIMutationObserver to get mutation notifications?  That _does_ fire during pageload.

I don't think you want to revert this patch, because it fixed some security bugs (see dependencies) in addition to being a performance win.

Comment 30

[Jacek Caban]

Thanks for your response. I'm trying to implement nsIMutationObserver based solution ATM. We try to avoid internal Gecko interfaces in Wine, but I guess at some point we can't avoid it anymore.

The problem that I can see with it is that we can't perform any mutations from nsIMutationObserver. Is there any proper way to do mutations on node insertion event?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Can you do the mutations asynchronously?

Comment 32

[Jacek Caban]

Not really, we have to run scripts exactly after script node insertion, because they may use document.write(). If there was a way to run them after nsIMutationObserver notification, but before next mutation, that should work for us.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm.  If you're patching core code anyway, you could add a little shim that would let you call nsContentUtils::AddScriptRunner from your code, I guess.  Not sure how that would order with script node insertions, though (inline script; it would run before external script loaded).

Comment 34

[Jacek Caban]

I haven't tested AddScriptRunner because I've found another problem. All elements insertions except html element are notified by ContentAppended, which is called when all child elements of aContainer are appended, so I can't catch script elements (and comments) early enough.

Comment 35

[Boris Zbarsky [:bzbarsky]]

You'll certainly catch script elements before they execute...  But yes, comments would be a problem for ContentAppended.  Does ParentChainChanged not do what you want, though?  You'd have to do some work to filter out removes and non-parser inserts if you don't want them, but you had to do the latter anyway with mutation events (and removes are just cases where in ParentChainChanged the parent chain doesn't hit the document).

Comment 36

[Jacek Caban]

I've played more with it. What I do is I set mutation observed on document object. I can confirm that AddScriptRunner works fine and ContentAppended is called too late. It looks like ParentChainChanged idea would be a good solution for me, but I don't know how to use it. As far as I understand, it's called on descendants of target node, so I'd have to add observer for all nodes before they are inserted to DOM tree by parser. How can I do that?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  We don't propagate ParentChainChanged up the tree, I guess...

I suppose worst-case you could add a new notification that happens at BindToTree time and does propagate up the tree (probably only for cases where the node is being bound to a new parent).  Jonas, do you have any better ideas here?

Comment 38

[Jacek Caban]

That would be exactly what I need, I will write a patch and test it. Could patch with such notification be committed to Gecko or should I maintain it myself?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Depends on how much impact it has on pageload, honestly.  Please spin off a separate bug on getting that implemented?

Comment 40

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

I'm weary of adding notifications for the explicit purpose of calling into extensions when it's not safe to evaluate scripts in general. We've seen several times that extensions don't always stay away from the things that they should.

In any case, I think a new bug is in order to get Wine up and running again. Please cc me.

Comment 41

[Jacek Caban]

Thanks, I've created bug 470576.