181910 - Click on link in zombie doc during "Transferring", onload won't fire for loaded document

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Aaron Leventhal]

If you click on a link to load a document, and then on a different link before
the first doc was loaded (during the "Transferring" stage), the new doc will
load but the onload handler won't fire.

Comment 1

[Aaron Leventhal]

Attachment: Stops content as soon as link clicked

This fixes the problem, but is it the best way?

Who knows enough about these issues to review?

Comment 2

[Aaron Leventhal]

Attachment: Test case

To reproduce, click on "mba" and during "Transferring" click on "Google".

The caret won't be automatically in google's edit box, as it would be if the
onload was fired.

Comment 3

[Aaron Leventhal]

An alternate fix might be do test to see if the current document is a zombie (by
looking to see if there is a previous viewer still active). If there is, then do
the full Stop(STOP_ALL), otherwise just do the STOP_NETWORK as it currently does.

Comment 4

[Johnny Stenback (:jst)]

Here's what's going on here...

Using the testcase, you click on the mba link and we start loading mba.com,
which for whaterver reason seems like a really slow site. Before mba.com is done
loading you click on the link to google.com, now we'll go ahead and cancel all
loads that are currently part of the document's loadgroup, this includes
external scripts n' so on from mba.com. When canceling the load of an external
script (which is the case I saw in the debugger using the testcase) we
asynchronously fire the script loaders OnStreamComplete() code with an error
code indicating that the load was canceled, this ends up telling the sink that
the script was loaded (or failed to load, rather) and the sink tells the parser
to continue parsing. The parser then continues feeding the already received data
(the data that came in while we were waiting for the script to load) and that
data ends up containing yet one more script tag, so we start loading it, and we
now of course add that load to the loadgroup that should now only contain the
requests for google.com. All the data from google.com ends up coming in, but the
script from mba.com still hasn't arrived, so we don't fire the onload handler
for google.com, and we won't do that until the script from mba.com actually
arrives and is removed from the loadgroup.

IOW, we *do* fire the onload handler, it's just significantly delayed by a
request that shouldn't have been added to the loadgroup in the first place.

I think this fix is "correct", since it ends up telling the parser in the
document that whose load is canceled by clicking on the second link to
terminate, thus the second script is never loaded by the data that came in over
the wire before the second link was clicked. I do, however, think that it would
be worth making this change take effect only when the current document is a
zombie document, as Aaron suggested.

Rick, it turns out that the presshell can't leave dummy requests in the
loadgroup as I thought, they're all guaranteed to be removed already, so no need
for that change.



I had a look at what's really happening here, the onload handler *does* fire,
even if you follow the steps lined out for the testcase in this bug, however,
the onload event does *not* even if all the loads that google.com initiates are
done, there's more stuff in the loadgroup that's left over from the target of
the initial link that was clicked on. That's the problem. The one time I caught
this in the debugger I saw a JS file from mba.com be the last thing to be
removed from the loadgroup, and 

Comment 5

[Johnny Stenback (:jst)]

Um, ignore the last paragraph in my last comment...

Comment 6

[Aaron Leventhal]

Attachment: Stops content only when in zombie

Same stop content fix in nsDocShell::InternalLoad, but now only does it if
we're in a zombie document (otherwise does what it used to do).

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 108499 [details] [diff] [review]
Stops content only when in zombie

+    // Stop any current network activity.
+    // Also stop content if this is a zombie doc. otherwise 
+    // the onload won't fire for the new document.

This comment is incorrect. The onload handler *will* fire, but it will be
delayed by other loads initiated in the background by the first document that
didn't fully load before the next load was initiated.

>+    rv = Stop(zombieViewer? (nsIWebNavigation::STOP_ALL) : (nsIWebNavigation::STOP_NETWORK));

For readability's sake I'd rather see this go in as:

+    if (zombieViewer) {
+	 rv = Stop(nsIWebNavigation::STOP_ALL);
+    } else {
+	 rv = Stop(nsIWebNavigation::STOP_NETWORK);
+    }

Other than that, sr=jst

Comment 8

[Aaron Leventhal]

fixed