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().
Created attachment 197706 [details] [diff] [review] Proposed patch
Same problem exists with nsStreamLoader.cpp, right?
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?
These guys were meant to behave the same, so if you can make it happen, that would be best. If not, okay.
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.
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
even better: * This method will always be called asynchronously by the * nsIStreamLoader involved, on the thread that called the * loader's init() method.
Created attachment 197913 [details] [diff] [review] Updated to comments
Attachment #197911 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta
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.
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.
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-
You need to log in before you can comment on or make changes to this bug.