nsUnicharStreamLoader::Init() screws up onload firing

RESOLVED FIXED in mozilla1.9alpha8

Status

()

RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.9alpha8
x86
Linux
Points:
---
Bug Flags:
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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().
(Assignee)

Comment 1

13 years ago
Created attachment 197706 [details] [diff] [review]
Proposed patch
Attachment #197706 - Flags: superreview?(darin)
Attachment #197706 - Flags: review?(darin)

Comment 2

13 years ago
Same problem exists with nsStreamLoader.cpp, right?
(Assignee)

Comment 3

13 years ago
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

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

Updated

13 years ago
Attachment #197706 - Attachment is obsolete: true
Attachment #197706 - Flags: superreview?(darin)
Attachment #197706 - Flags: superreview-
Attachment #197706 - Flags: review?(darin)
Attachment #197706 - Flags: review-
(Assignee)

Comment 5

13 years ago
Created attachment 197911 [details] [diff] [review]
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.
Attachment #197911 - Flags: superreview?(darin)
Attachment #197911 - Flags: review?(darin)

Comment 6

13 years ago
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
Attachment #197911 - Flags: superreview?(darin)
Attachment #197911 - Flags: superreview+
Attachment #197911 - Flags: review?(darin)
Attachment #197911 - Flags: review+

Comment 7

13 years ago
even better:

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

Comment 8

13 years ago
Created attachment 197913 [details] [diff] [review]
Updated to comments
Attachment #197911 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta

Comment 10

12 years ago
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.
I think we should do that, if the patch applies.
Flags: blocking1.8.1?
(Assignee)

Comment 12

12 years ago
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

12 years ago
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?
Flags: blocking1.8.1? → blocking1.8.1-

Updated

12 years ago
Blocks: 338581
You need to log in before you can comment on or make changes to this bug.