294114 - [FIX]Replace the DummyLayoutRequest with nsIDocument::BlockOnload

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I _think_ we can replace the dummy layout request with nsIDocument::BlockOnload.
 The only hitch is that at the moment the dummy layout request has a nontrivial
Cancel() method -- calling Cancel() on it will actually cancel all reflow commands.

As biesi pointed out, this means that the user pressing the stop button at just
the right moment (while we have a dummy layout request posted) will actually
lose reflows that should have happened.  So this looks like something we can do
without; just have to be a little careful with teardown.

Comment 1

[Boris Zbarsky [:bzbarsky]]

There may be more presshell code I can cut out (and more code elsewhere?) if I
make the nsIDocument api unblock async.  That takes a bit more work in the
nsDocument code, though.  Anyway, I'll see which way works out cleaner.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reflow command cancelling stuff is gone on REFLOW_20050429_BRANCH, I think.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Excellent!

I did check, and if I just make the document-level unblocking happen async I can
remove the code I added for bug 281157 and company.  Doing this in the document
should be much simpler, since I don't have to worry about the Destroy issues
presshell has and can simply hold a strong ref to the document and check for a
non-null mScriptGlobalObject if needed before messing around with the loadgroup.

darin, jst, does that seem reasonable?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch. No more dummy layout request.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: With the nsIDocument change too

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: And without public methods

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure why your changing nsIDocument's IID for a comment change.

The definitions of your two event handling callbacks in nsDocument.cpp need
PR_CALLBACK.  They also probably shouldn't be member functions, since in theory
the calling conventions could be different.  I think.

Should the early return in nsDocument::UnblockOnload have an NS_NOTREACHED?

Given the last change in nsPresShell, should the document have a check that it's
not firing onload while it is being destroyed?  Or is there already such a check?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, now that I understand the change, maybe it is worth changing the IID.

In your change to PresShell::Destroy, could you replace the removed code with an
assertion (!mDocumentOnloadBlocked) that CancelAllReflowCommands called
UnblockOnload?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> The definitions of your two event handling callbacks in nsDocument.cpp need
> PR_CALLBACK.

Done.

> They also probably shouldn't be member functions

We do this elsewhere, and I've asked people to make sure this patch works on
Windows...

> Should the early return in nsDocument::UnblockOnload have an NS_NOTREACHED?

Done.

> should the document have a check that it's not firing onload while it is being
> destroyed?

This is basically what the mScriptGlobalObject check in DoUnblockOnload is. 
That makes sure we fire onload only if the document is the current document in
its docshell.

> In your change to PresShell::Destroy, could you replace the removed code

Done.

I think the IID change is worth it, since I'm changing the semantics of the
method in a pretty major way.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> > They also probably shouldn't be member functions
> 
> We do this elsewhere, and I've asked people to make sure this patch works on
> Windows...

It wasn't the Windows compiler I was worried about.  It was conformance to the
C++ standard.  Although the issue actually has nothing to do with being a member
function (it's just the issue of C++ vs. C linkage, which is sort of what I was
thinking when I wrote them comment, but I didn't say it correctly).  And as far
as I can tell this violates the standard and a diagnostic is required of
conformant implementations, although I'm not anywhere near enough of an expert
to say that for sure.  The only compiler I've actually seen give a diagnostic
for it was, I think, the Sun Workshop compiler.  (But it was only a warning.) 
And the actual fix for the problem would be to give the function type "C"
linkage, which is required since function types with different language linkages
are distinct types (and I haven't found any rule allowing implicit conversion
between them).

In theory, you should be able to fix this by just declaring the members in
nsDocument.h as 
  static PLHandleEventProc HandleOnloadBlockerEvent;
  static PLDestroyEventProc DestroyOnloadBlockerEvent;
although I'm a little skeptical as to how portable that would be.  If it's not
portable, the workaround would involve not using member functions.

That said, we *do* do this all over, but *as far as I can tell* a diagnostic is
required of implementations for this mistake.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Yeah, it's the Sun native compiler, see bug 81385 and bug 92571.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the linkage issues

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Er... this is the right diff

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 193245 [details] [diff] [review]
Er... this is the right diff

sr=jst

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This whacked Tp pretty hard on btek, 930->970/960, and on luna, 880->930

Comment 17

[Boris Zbarsky [:bzbarsky]]

My #1 culprit for that is having to get the event queue service and the current
event queue a bunch of times... I'll try to profile tomorrow morning to check
and to write up a patch to mitigate this problem in any case.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to try to fix Tp a bit

I landed this with r+sr=jst.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Something else that may help

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 193579 [details] [diff] [review]
Something else that may help

- In nsDocument::PostUnblockOnloadEvent():

+    // If there is only one active request, then its our onload
+    // blocker; in that case we should fire onload as soon as our even

"event", even? :)

r+sr=jst

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Might this have caused bug 305639, or am I way off?

Comment 22

[Boris Zbarsky [:bzbarsky]]

The "something else that may help" patch didn't help Tp and hurt Tdhtml, so I
backed it out.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Another attempt to restore prior behavior

This makes us fire onload sync when we used to do it before... If the patch for
bug 294114 doesn't help, I'll try this.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Er, I meant for bug 305639.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 193602 [details] [diff] [review]
Another attempt to restore prior behavior

r+sr=jst

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK, that last patch did the trick with fixing Tp.  Marking fixed.

Comment 27

[Steve England [:stevee]]

Could this have caused a jump in the reported memory leakage from balsa? See bug
305612.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Not very likely, since we leak none of the objects involved here.