299553 - [FIXr]ASSERTION: Overwriting an existing document channel!

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

The test "randomly" loads one of two different documents into an iframe which is
document.written into the page. It tests if the correct document has been loaded
and alerts if it finds the wrong document otherwise it reloads.

In an opt build you may see the alert after a few loads, but if not click
refresh. In a debug build the alert will appear quickly and the following assert
will appear

###!!! ASSERTION: Overwriting an existing document channel!: '(loadFlags &
nsIChannel::LOAD_REPLACE) || !(mDocumentRequest.get())',
file c:/work/mozilla/builds/ff/trunk/mozilla/uriloader/base/nsDocLoader.cpp,
line 486

This is not a regression, it goes back to at least Mozilla 1.5.

MSIE will run the test with no alert until you click refresh.

I was trying to write a testcase for bug 295813 which has similar behavior, but
in that case the ASSERTION never appeared.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so there are two things going on here:

1)  The assertion.  This is happening because the docloader has become empty but
hasn't figured that out yet.  As a result, when we do the reload() there is
nothing to stop, so we don't clear the mDocumentRequest, but clobber the
existing one with a new one.  This is all happening in the _parent_ docshell. 
The fix here, imo, is to call DocLoaderIsEmpty() in nsDocLoader::Stop() after
we've stopped the kids and canceled the loadgroup.  I'll attach that.

2)  Subframe loads.  Per our history design, when we reload the toplevel page we
reload the _current_ uri in the subframe, not whatever URI was loaded there
initially (the src value of the subframe).  In other words, we should get an
alert on that testcase as soon as the value of the src attribute changes from
what it was when the page was initially loaded.  IE has a different behavior --
it will reload the current URI unless the initial URI has changed; in that case
it will load the new initial URI.

I suppose we could try to duplicate IE's behavior (by storing the "src"
attribute of the iframe in the session history entry), but to me it seems like a
very counterintuitive behavior and can lead to dataloss for users (reload
suddenly loading some totally different page from the one they were looking at).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

[Darin Fisher]

Comment on attachment 188632 [details] [diff] [review]
Proposed patch

maybe you should only call DocLoaderIsEmpty if the
mLoadGroup->GetActiveCount(&count) returns 0.  granted that is going to be the
case because of the way Cancel is implemented on the loadgroup, but that
behavior is not well documented.

hrm... so, DocLoaderIsEmpty will be called for us in the case where the
loadgroup was not empty.  should we check to see if the loadgroup is empty
before canceling it?  then we could know that DocLoaderIsEmpty will not be
called and only call it if necessary.  maybe?

Comment 4

[Darin Fisher]

I should add that I worry about what might happen during those OnStopRequest
calls made by the loadgroup.  If we unconditionally call DocLoaderIsEmpty after
calling LoadGroup->Cancel, might we inadvertantly affect some other document
request (assuming something new was loaded from OnStateChange(STATE_STOP))?

Comment 5

[Boris Zbarsky [:bzbarsky]]

DocLoaderIsEmpty is idempotent if nothing has happened.  Also, it does nothing
if there are any active loads in the loadgroup or any subframe's loadgroup, and
it does nothing if we're not currently in the middle of a load (as determined by
mIsLoadingDocument).  So I think just calling it unconditionally here is quite
safe.  The method would perhaps be more accurately named "DocLoaderCouldBeEmpty"...

Comment 6

[Darin Fisher]

Comment on attachment 188632 [details] [diff] [review]
Proposed patch

ok, then perhaps we should rename this method? ;-)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe you should add a comment to nsDocLoader.h stating that DocLoaderIsEmpty is
idempotent

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 188632 [details] [diff] [review]
Proposed patch

requesting 1.8b4 approval for this assertion fix.  This is quite low-risk.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed