Don't fire mutation events during initial page load

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P3
minor
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Daniel Bratell, Assigned: sicking)

Tracking

(Depends on: 1 bug, {perf})

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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.

Updated

16 years ago
QA Contact: lchiang → stummala

Updated

16 years ago
Keywords: perf

Comment 1

16 years ago
Marking P3. If the 2% number is confirmed, it would probably be cool to fix 
that.
Priority: -- → P3
(Reporter)

Comment 2

16 years ago
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.
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs

Comment 4

14 years ago
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.
Depends on: 231676
I'll take this
Assignee: general → bugmail
Blocks: 330872
No longer depends on: 231676

Updated

12 years ago
Blocks: 201236

Comment 6

12 years ago
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.
Blocks: 331668
Created attachment 219400 [details] [diff] [review]
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.
Attachment #219400 - Flags: superreview?(bzbarsky)
Attachment #219400 - Flags: review?(bzbarsky)

Comment 8

11 years ago
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?
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.
Created attachment 219409 [details] [diff] [review]
patch -w

For review goodness (in case we want to do this, which I think we do).
(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.
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

11 years ago
> 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.)
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.
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

11 years ago
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)?
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...
(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.
Patch has been bitrotted due to the checkin for bug 201236.
Created attachment 221070 [details] [diff] [review]
Sync to tip

This just syncs to tip.
Attachment #219400 - Attachment is obsolete: true
Attachment #219409 - Attachment is obsolete: true
Attachment #219400 - Flags: superreview?(bzbarsky)
Attachment #219400 - Flags: review?(bzbarsky)
Created attachment 221071 [details] [diff] [review]
Sync to tip -w
Attachment #221071 - Flags: superreview?(bzbarsky)
Attachment #221071 - Flags: review?(bzbarsky)
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.
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.
Attachment #221071 - Flags: superreview?(jst)
Attachment #221071 - Flags: superreview?(bzbarsky)
Attachment #221071 - Flags: review?(jst)
Attachment #221071 - Flags: review?(bzbarsky)
Comment on attachment 221071 [details] [diff] [review]
Sync to tip -w

r+sr=jst
Attachment #221071 - Flags: superreview?(jst)
Attachment #221071 - Flags: superreview+
Attachment #221071 - Flags: review?(jst)
Attachment #221071 - Flags: review+
Created attachment 224135 [details] [diff] [review]
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
Attachment #224135 - Flags: superreview?(bzbarsky)
Attachment #224135 - Flags: review?(bzbarsky)

Comment 26

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

Looks good.  Makes me happy to see this go away!
Attachment #224135 - Flags: superreview?(bzbarsky)
Attachment #224135 - Flags: superreview+
Attachment #224135 - Flags: review?(bzbarsky)
Attachment #224135 - Flags: review+
Checked in, thanks for the reviews!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 341586

Updated

10 years ago
Blocks: 378963

Updated

10 years ago
Component: DOM: Abstract Schemas → DOM
OS: Windows 2000 → All
Hardware: PC → All
Summary: Why check for mutationlisteners during initial page load? → Don't fire mutation events during initial page load
Target Milestone: --- → mozilla1.9alpha1
Depends on: 438001

Comment 28

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
Can you do the mutations asynchronously?

Comment 32

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
Depends on how much impact it has on pageload, honestly.  Please spin off a separate bug on getting that implemented?
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

9 years ago
Thanks, I've created bug 470576.
You need to log in before you can comment on or make changes to this bug.