30627 - <frameset> onload handler must fire _after_ child documents have loaded

Status: VERIFIED WORKSFORME

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

Description

[John Morrison]

The following issue came up during the course of looking at bug #23777.
That bug was fixed (it was a floating point error in handling "HTTP/0.9",
surprisingly enough).

However, this problem still remains. Using today's mozilla.org bits on
win95, on *one* occasion, an alert is thrown up that said something like
'... thePage has no properties ...'. [I haven't been able to reproduce this
error a second time.]

There is a crude testcase at 
  http://qsilver.queensu.ca/~buslib/frameload/parent.html
(Run with '-console' to see the output). 

Here are comments by vidur and I from bug #23777. CC: travis for webshell.

  ------- Additional Comments From [e-mail]  2000-01-18 17:40 ---
  One thing that I noticed about members.xoom.com/whatever pages was
  that they all have this kind of structure (abbreviated):
  
          <frameset onload='doRewrite();'>
             <frame name='pb'>
             <frame name='thePage'>
          </frameset>
  
   Now, in 4.x and the js client docs, the onload handler for <frameset>
  fires *after* the two child frames have loaded (have fired
  onload). But in mozilla it fires without waiting for the child frames
  to load.
  
   In the particular case of 'members.xoom.com/whatever', doRewrite()
  manipulates the DOM of <frame name='thePage'>. But "thePage" hasn't
  loaded at the time that doRewrite() tries to rewrite it's links. This
  may be the trigger of the eventual crash. (At the very least, it's a
  bug in its own right -- I'll set up a simple test case and update this
  bug or file a new one.) But cc: vidur, in case this is already
  something he knows about.
  
  ------- Additional Comments From [e-mail]  2000-01-19 16:59 ---
  Well...the described bug could well be considered a webshell issue,
  but I'll hold onto it. Currently onload handlers are firing from
  notifications that end up in the webshell. The notification for the
  frameset probably happens when the frameset document itself completes
  loading. This doesn't match DOM expectations, which might mean we need
  to store additional state.

Comment 1

[John Morrison]

One note about the above -- I refer to crash in my comments of jan 18. However, 
that does not apply, as far as I know, to this bug report. It does not crash.

Comment 2

[vidur (gone)]

Currently the onload handler for a page fires in nsWebShell::OnEndDocumentLoad, 
when the parser is done with the page. We probably need to get notification when 
child webshells load and fire the onload only when the number of notifications 
equals the number of children we have.

Comment 3

[ekrock's old account (dead)]

Nominating nsbeta2. We have to start drawing a line on DOM0 backward 
compatibility; these bugs are supposed to be a high priority for nsbeta2 per the 
beta2 criteria.

Comment 4

[John Morrison]

This is also what the behaviour of xul:iframe's should be (and is not at this
time): the parent onload handler must fire after the child iframe's have fired 
their onload. The xul iframes should pick up this change when it's done in dom, 
but need to check that this is so (i.e., xul does not need any further changes 
to make this work).

Comment 5

[leger]

Per chofmsnn
all user pages on xoom.com are busted.
if this is webshell lets get valeski/adam lock kicking some butt on it.
Putting on [nsbeta2+] radar for beta2 fix. 

Comment 6

[locka]

Adding jst to the list since I know he had a fix for this...

Comment 7

[John Morrison]

The pages load ok for me, but what is broken is the the frameset is trying
to rewrite all the <A HREF> in the lower frame -- since this code executes
before the lower frame has loaded, this throws a bunch of js exceptions, and
the rewrites fail (thus breaking the xoom.com navigation 'scheme')

Comment 8

[Johnny Stenback (:jst)]

Adam, I can't say I have a fix, but I have a "hack" in my tree that
fixes this, but there's still a lot to be done before it's ready to be
checked in.

What I did so far was to add a "state" getter to nsIDocShellTreeItem
and based on that the docshell root checks its children when one of
them is done loading, and if all child docshells are done, it fires
the onload handlers. Two problems still remain:

1. there's no clean way for a docshell child to tell its parent that
it's done loading.

2. There's no way for a docshell parent to cleanly fire the onload
event, it's possible to fire the event from a parent, but the child
doesn't know about the event being fired and it needs to know this to
prevent futher fireing of the onload handler if a sibling docshell is
reloaded.

Here's what I propose doing to continue cleaning this up:

Make the tree item state read/write, the different states should be
something like this:

  stateNotLoadingYet
  stateLoading
  stateLoaded
  stateDone

(please suggest better names) and then we make the onload event fire
when the state is changed from stateLoaded to stateDone.

We also need to add a method to nsIDocShell that lets a child inform
it's parent (or root, actually) that the childs state changed,
something like this:

  [noscript] childStateChanged(in nsIDocShellTreeItem child,
                               long oldState);

I think we'd be able to cover all possible scenarios with something
close to the above suggestion, the possible cases we need to handle
properly is:

a. Loading a standalone document.

b. Loading a frameset document with one or more frames in it, possibly
even nesetd frames of course.

c. Loading a new document into one frame in a frameset (ie if a link
is clicked in the frame), ie the frameset itself doesn't reload, nor
the other frames.

(Cc:ing rpotts since I think he's working on related things in the
docshell.)

Adam (and others), does this sound reasonable?

Comment 9

[vidur (gone)]

Adding rpotts to the cc: list to get his input.

Comment 10

[Johnny Stenback (:jst)]

Taking this bug off Vidurs list.

Comment 11

[Nisheeth Ranjan]

Adam, I spoke to Johnny about this and he feels that this bug should belong to 
the docshell owners.  He's away for the DOM face to face meeting in Germany, so 
I'm doing the re-assignment for him.

Comment 12

[locka]

Rick - what effects do your recent changes to nsIWebProgressListener have on 
this problem?

Comment 13

[rpotts (gone)]

I was planning to dive into this mess next :-)

I hope to rework how javascript OnLoad handlers are fired...

First, I want to move the code out of the nsIDocumentLoaderObserver 
implementation and into nsIWebProgressListener

Next, I want to push the responsibility of firing the javascript handlers into 
the docviewer, since it *knows* about javascript onhandlers()...

-- rick

Comment 14

[Judson Valeski]

related? http://bugzilla.mozilla.org/show_bug.cgi?id=28572

Comment 15

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 16

[Judson Valeski]

-> rpotts

Comment 17

[Johnny Stenback (:jst)]

*** Bug 45369 has been marked as a duplicate of this bug. ***

Comment 18

[rpotts (gone)]

-> jst

Johnny has agreed to look at my onLoad/onUnload bugs while I'm gone...

Comment 19

[Johnny Stenback (:jst)]

Accepting bug.

Comment 20

[Johnny Stenback (:jst)]

I don't know what fixed this nor when that happened, but I'm unable to reproduce
this now, it used to be trivial to reproduce, but now it's much less so...

Marking WORKSFORME.

Comment 21

[John Morrison]

Yeah, this is doing the right ordering now (even with multiple nested framesets
(win32/linux)).

Comment 22

[John Morrison]

marking verified worksforme with the 2000080404 binaries on win2k/linux/mac.

Prashant, there are some testcases for this here:

  A simple two-frame frameset (dumps to console)
      http://jrgm.mcom.com/testcase/dom/frames/frameset-onload/001/
  A nested two-frameset/two-frames-per-frameset (dumps to console)
      http://jrgm.mcom.com/testcase/dom/frames/frameset-onload/002/
  A nested two-frameset/two-frames-per-frameset (throws up alerts)
   suitable for testing the Mac or Nav4.x
      http://jrgm.mcom.com/testcase/dom/frames/frameset-onload/003/