When inserting fragments into the DOM, use ContentAppended notification when possible

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 391198 [details] [diff] [review]
Patch to fix

Currently when fragments are inserted, we call ContentInserted, one node at a time, for each node. It'd be much better if we could just make a single ContentAppended call when possible.

The reason this actually matters is that innerHTML is inserted using fragments.

Patch coming up.

The only behavioral difference this makes is that we call mutation observers after all nodes are inserted, rather than after each node is inserted.
Attachment #391198 - Flags: superreview?(jst)
Attachment #391198 - Flags: review?(jst)

Updated

8 years ago
Attachment #391198 - Flags: superreview?(jst)
Attachment #391198 - Flags: superreview+
Attachment #391198 - Flags: review?(jst)
Attachment #391198 - Flags: review+
Comment on attachment 391198 [details] [diff] [review]
Patch to fix

-  mozAutoDocConditionalContentUpdateBatch(aDocument,
+  mozAutoDocConditionalContentUpdateBatch batch(aDocument,
     aReplace || nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE);

Nice :)

r+sr=jst
Ah, so the key is that we fire all the mutation events after the whole thing is done, and hence avoid the problems this used to have?

I can live with that.

I'm not sure about the first part of the patch, though; BindToTree can still run scripts in some cases (though hopefully only chrome ones?).  Smaug might know more about that.
Oh, and we'll want to revisit this once we have lazy frame construction.
Oh, and what do performance numbers on this end up looking like?
> I'm not sure about the first part of the patch, though; BindToTree can still
> run scripts in some cases (though hopefully only chrome ones?).  Smaug might
> know more about that.

We put script blockers around all BindToTree calls so I really hope that still can't run scripts. In fact, I thought that we had an assertion in BindToTree that it was only called with scripblockers on the stack, we should definitely add one.
(In reply to comment #3)
> Oh, and we'll want to revisit this once we have lazy frame construction.

It always seems like a good idea to use ContentAppended though, no? Even if for no other reason to make fewer nsIMutationObserver notifications.

(In reply to comment #4)
> Oh, and what do performance numbers on this end up looking like?

I don't see that this could ever be a perf-hit, so I haven't checked.
> so I really hope that still can't run scripts.

Can't run <script>s, sure.  Can certainly trigger necko notifications and content policy (for now; smaug's working on it as I recall) which can run extension script which could screw up and trigger page script.

For trunk, I think we should just fix that stuff to all be async.  If we want this patch for 1.9.2 we should think about whether we need to worry about the above.

> Even if for no other reason to make fewer nsIMutationObserver notifications.

Ah, true.

> I don't see that this could ever be a perf-hit, so I haven't checked.

Sure; I was more interested in how much of a win it is.
I'm pretty sure that if page script run while there are script blockers we are already hosed. I'd expect that to be exploitable in other ways, and I know that it's bad for other reasons. For example an alert() brings down the browser since layout event handling is disabled and so you can't click the OK button.
This was checked in about a week ago btw.

http://hg.mozilla.org/mozilla-central/rev/3222b99f441c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.