409390 - Avoid nsCOMPtr's and some code in nsDocument::FlushPendingNotifications() when possible.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

nsDocument::FlushPendingNotifications() has code to notify the parent document etc when flushing layout (aType > Flush_ContentAndNotify), but that code is a no-op when mParentDocument is null and should then be avoided. Also, as the comment in the code says, a lot of that code can be replaced with using mParentDocument directly. See attached patch.

Comment 1

[Mike Schroepfer]

+ing for perf impact

Comment 2

[Johnny Stenback (:jst)]

Ben agreed to look into this, reassigning.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This look ok?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why is the SyncParentSubDocMap call needed?

Also, the assert in FlushPendingNotifications looks wrong; it's possible to have a subdocument have a null mParentDocument but still have a docshell which has a parent.  In those cases we don't want to be flushing the parent docshell...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 296618 [details] [diff] [review]
Patch, v1

sr- pending answers to my questions

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #4)
> Hmm.  Why is the SyncParentSubDocMap call needed?

I don't know for a fact that it is, but when Ben and I went through this code that was the one place where mDocument was set and it wasn't obvious that the map would be updated in this case. If you're saying it's not needed, then we'll just not add this.

> Also, the assert in FlushPendingNotifications looks wrong; it's possible to
> have a subdocument have a null mParentDocument but still have a docshell which
> has a parent.  In those cases we don't want to be flushing the parent
> docshell...

Uh? I don't understand what case that would be, but if you say so (and you know this stuff way better than I do, so I'm not going to argue with you)...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> If you're saying it's not needed, then we'll just not add this.

No, I was just wondering what cases it was needed in.  I'll look to see what the effect is of having or not having it here.

> Uh? I don't understand what case that would be,

During paint suppression, we've set the new doc in the subdoc map (so the old one has a null mParentDocument), but not called Show() on the new viewer, so the old viewer hasn't gotten a Destroy(), so its document still has the old container... It's a mess.  :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, looks like the only places where LoadStart() is called in our code on a document viewer with a null mDocument, the container of the document viewer is still null, so SyncParentSubDocMap is effectively a no-op.  I'd say we shouldn't add it here, and instead document the call order here and in the IDL.  Or we could add it (it's not a super-expensive no-op), and file a bug to clean up the document viewer mess.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Ok, this patch loses the unnecessary call and the assertion.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 301317 [details] [diff] [review]
Patch, v2

r=bzbarsky

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 301317 [details] [diff] [review]
Patch, v2

- In nsDocument::DispatchContentLoadedEvents():

+    nsIDocument* parent = mParentDocument;
+    while (parent) {
+      parent = parent->GetParentDocument();

It might be worth while to make parent an nsCOMPtr here as dispatching these events could in theory kill a document, which could leave parent dangling here next time through.

bz, if you know for a fact that that can't happen, then please speak up.

sr=jst with that.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 301317 [details] [diff] [review]
Patch, v2

This patch avoids some needless QI action and simplifies the code a bit.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 301317 [details] [diff] [review]
Patch, v2

Oh, nm, blocking+ already.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, making that a strong ref sounds like a good idea.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed, used a nsCOMPtr.