310319 - nsUnicharStreamLoader::Init() screws up onload firing

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The testcase is attachment 197700 [details], but needs to be saved to disk to reproduce
the problem.  The sequence of events is as follows:

1)  We parse the stylesheet PI and try to load the sheet.  AsyncOpen() on the
channel fails (usual file channel games), so the unichar stream loader's Init()
creates this observer proxy, calls onStreamComplete on it, and returns NS_OK.
2)  The CSSLoader thinks the load started fine and returns, blocking the parser.
 It'll unblock the parser when the load completes.
3)  We unwind to the event loop and finish reading data from the file channel
for the XUL document.  When the file:// request is removed from the loadgroup,
onload fires.  But since the parser is blocked, none of that stuff has been
parsed.  So the <window>'s onload handler does not fire, since we haven't seen
it yet.
4)  We get our onStreamComplete, unblock the parser, etc, but it's too late --
onload has fired.

I don't really recall why we did this proxy stuff.  That is, why are we
bothering calling onStreamComplete at all, if Init() is basically failing?  I
think in this case we should just throw from Init().

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Darin Fisher]

Same problem exists with nsStreamLoader.cpp, right?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, but unlike nsIUnicharStreamLoader, nsIStreamLoader documentation says:

 75      * @note Failure to open the channel will be indicated by an async callback
 76      *       to the observer.

So yes, the same problem could happen for nsIStreamLoader, but we can't really
fix it without effectively changing the API.  Which I would be happy to do, but
I'd have to check over consumers first... Want me to?

Comment 4

[Darin Fisher]

These guys were meant to behave the same, so if you can make it happen, that
would be best.  If not, okay.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with both loaders fixed

The calendar code just expected init() to never throw, and XULDocument handled
it throwing wrong....  The manual refcounting in nsXULDocument was just
unnecessary.

Comment 6

[Darin Fisher]

Comment on attachment 197911 [details] [diff] [review]
Patch with both loaders fixed

>Index: netwerk/base/public/nsIStreamLoader.idl

>+     * This method will always be called asynchronously by the
>+     * nsIStreamLoader involved, on the thread on which init() was
>+     * called on the loader.

3 "on"s is a tongue twister... how about this instead:

       * This method will always be called asynchronously by the
       * nsIStreamLoader involved, on the thread that the loader's
       * init() method was called.


>+     * @note Failure to open the channel will cause init() will throw.  A

s/will throw/to throw/


>+     *       successful call to init() means that onStreamComplete is
>+     *       guaranteed to be called on aObserver asynchronously (so after
>+     *       init() returns).

s/so/i.e.,/


>Index: netwerk/base/public/nsIUnicharStreamLoader.idl

same comment nits here


r+sr=darin

Comment 7

[Darin Fisher]

even better:

       * This method will always be called asynchronously by the
       * nsIStreamLoader involved, on the thread that called the
       * loader's init() method.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 10

[James Ross]

Is there any chance of this fix landing on the 1.8 branch? It turns out (thanks tH!) this fixes bug 338581, which is exceptionally annoying to all concerned.

Comment 11

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

I think we should do that, if the patch applies.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Note that this is somewhat of an API change (as evidenced by the fact that I had to change a JS caller to deal)...  The IDL didn't change, but callers might need to.  Not sure what our policy on such things is for 1.8.1.

Comment 13

[Mike Schroepfer]

Bz - yeah the policy is no API changes that require changing the callers (signature the same or not).   So as is this would not be suitable for branch.  What's the necessity of having this on the 1.8 branch?