195562 - nsJSChannel calls Cancel before AsyncOpen and expects it to mean something

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[Darin Fisher]

see bug 193887 comment #31 for a description of the problem.

Comment 1

[Darin Fisher]

so, i didn't fully understand this "bug" until i took a look at some stack
traces.  turns out that we have the following stack (relevant functions only):

    nsJSChannel::Cancel
    nsLoadGroup::Cancel
    ...
    nsDocShell::LoadURI
    ...
    LocationImpl::SetHref
    ...
    nsJSThunk::EvaluateScript
    nsJSChannel::AsyncOpen
    ...
    nsDocShell::LoadURI

notice at the point where AsyncOpen calls EvaluateScript, mStreamChannel has not
had its AsyncOpen method called.  and if you recall, my fix for the original bug
was to make nsInputStreamChannel::Cancel "work" even if AsyncOpen hadn't been
called.

this stack trace is very troublesome because as you can see nsDocShell::LoadURI
is being called recursively :-(

it seems to me that nsJSChannel::AsyncOpen should post a PLEvent to issue the
call to EvaluateScript.  then nsJSChannel::AsyncOpen would be truly
asynchronous, and things would just make more sense and be less fragile IMO.

on the other hand, the current code works and is rather lightweight.  still, i
think i would prefer to do it right to avoid future regressions when something
legitimately changes, either in necko or docshell/docloader.

sadly i don't think i'll have time to do this for alpha.  maybe beta =)

Comment 2

[Brendan Eich [:brendan]]

If nsJSChannel::AsyncOpen were to use a plevent, and the script did not generate
a new document (did return an undefined result), then would it be able reliably
to operate on the current page's DOM?  That is a requirement for void scripts,
such as most bookmarklets and lots of DHTML page links.

It seems to me the greater evil here is LocationImpl::SetHref trying to load
while a URL is loading.  In olden days, in the classic codebase, we had a way to
unwind-protect, to defer nested loads until the outer load was either aborted or
completed.

/be

Comment 3

[Darin Fisher]

iirc, the current page only "goes away" when the new document gets an
OnStartRequest from the channel and the channel's status does not indicate any
errors.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Checkin for bug 351633 fixed this.