Closed Bug 756277 Opened 9 years ago Closed 9 years ago
Request started from Cc["@mozilla .org/xmlextras/xmlhttprequest;1"] ends up bound to some random window
When this happens, I see this in my terminal: NS_ENSURE_TRUE(!mHasOrHasHadOwner || mOwner) failed. This is from http://hg.mozilla.org/mozilla-central/annotate/895e12563245/content/events/src/nsDOMEventTargetHelper.h#l147 Per IRC discussion, smaug thinks a "perfect hack" to remove the randomness would be to use the hidden window. This seems to be directly causing bug 735688 where an HTTP request started by a back-end JS XPCOM component is aborted when a modal dialog is closed.
This is an old bug. I'll figure out some way to fix this.
If I'm interpreting this bug properly, I think this means that in-process XMLHttpRequest's can get killed if the "randomly selected" window the request is bound to closes. (I say "randomly selected" because it's quite likely that instead of random, the most recent window is selected...) CC'ing Mark / David / Jb because all three of our major features that we're hoping to ship in the next few versions of Thunderbird (Account Provisioner, Filelink, IM) rely on XMLHttpRequest to function properly. IM has already been running into this... so far, I haven't heard of failures in Filelink or Account Provisioner, but I think the potential is there.
Hm - so I think I was wrong about Account Provisioner on this one. Account Provisioner doesn't create an instance of XMLHttpRequest via Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] - it uses jQuery, which means it uses the vanilla XMLHttpRequest from the DOM.
This would impact add-ons severely. We can't have XHR's randomly failing depending on what window they happened to get attached to.
Yes. But this is an ancient bug, dates back at least to FF1.5, IIRC. So nothing catastrophic hasn't happened.
Oh, I have no idea who has updated the documentation and why. I do *hope* to get rid of createInstance for XHR, but I'm still surprised to see that documentation has been changed already. Anyhow, this particular bug is *old*.
...FF 2.0 certainly had this behavior. Also, I'm expecting to first fix this sort-of-random behavior somehow.
> I do *hope* to get rid of createInstance for XHR Can I ask why? What exactly is going on with XHR? Is it no longer going to be a component? Is this work detailed somewhere?
(In reply to Michael Kaply (mkaply) from comment #10) > > I do *hope* to get rid of createInstance for XHR > > Can I ask why? Because per spec XHR is bound to a document/window. It would make the implementation simpler if there was just one way to create and use XHR. But anyhow, I think for now we should just support createInstance.
Attachment #631158 - Attachment is obsolete: true
This should pass on try. Makes createInstance created XHRs always using system principal (if needed, we can add a scriptable init() to pass different principal). The changes to nsDOMClassInfo.cpp aren't strictly needed, but I want to make sure that XHR created in a web page using new XMLHttpRequest() has never mPrincipal pointing to the system principal. Either there isn't a principal, or the principal is the right one.
Comment on attachment 631303 [details] [diff] [review] patch r=me
Attachment #631303 - Flags: review?(bzbarsky) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 631303 [details] [diff] [review] patch Thanks for fixing this! I can confirm this patch fixes bug 735688. [Approval Request Comment] This bug was the real cause of bug 735688. Without this fix the configuration process of Twitter accounts in Thunderbird randomly breaks for a large proportion of users. This implementation of Twitter has been on comm-central for a few cycles already but is pref'ed off on current releases while we polish the remaining issues. Our current plan is to ship it (that is, have it pref'ed on) in Thunderbird 15 (currently in aurora), so I would really appreciate if this fix could land on the aurora branch.
Attachment #631303 - Flags: approval-mozilla-aurora?
This patch needs a risk evaluation before being considered for approval.
Should be reasonable safe, but might affect some addons which, for some strange reason, rely on the old behavior.
FYI, we started experiencing this problem in FF13. I tracked it down to: https://bugzilla.mozilla.org/show_bug.cgi?id=734057 So although you say this bug has been around for a while, the change for 734057 caused it to start happening in places where it didn't.
The fix for bug 734057 made this happen more than it used to. Our add-on that did an XHR in a modal window broke with FF13. I believe other addon developers will encounter this as well. We should fix it in FF14.
(In reply to Michael Kaply (mkaply) from comment #21) > https://bugzilla.mozilla.org/show_bug.cgi?id=734057 Ah, yes, that bug did affect the behavior. The old behavior caused leaks in some cases, the new one closes XHR when it isn't really usable anymore. (Note even with the old behavior event listeners wouldn't be called when the window goes away.)
I'm going to mark this as a regression because as of bug 734057, things are broke in FF13. If a FF13 update goes out, I think this fix should be in it. As it stands, FF13 breaks some addons.
Comment on attachment 631303 [details] [diff] [review] patch [Triage Comment] Let's land this on Aurora in preparation for possible beta/release uplift.
Attachment #631303 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There are over 1000 MXR results in addons for this contractid, so I expect that this could be a fairly common issue for addons and this may be worth taking for a potential 13.0.1 if we spin that.
(In reply to Alex Keybl [:akeybl] from comment #25) > Comment on attachment 631303 [details] [diff] [review] > patch > > [Triage Comment] > Let's land this on Aurora in preparation for possible beta/release uplift. It would also be great to get feedback on the risk to taking this on beta/release this week. Thanks!
(In reply to Michael Kaply (mkaply) from comment #24) > I'm going to mark this as a regression because as of bug 734057, things are > broke in FF13. > > If a FF13 update goes out, I think this fix should be in it. > > As it stands, FF13 breaks some addons. which addons exactly? The behavior change in FF13 affects addons only in certain cases.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #26) > There are over 1000 MXR results in addons for this contractid, so I expect > that this could be a fairly common issue for addons and this may be worth > taking for a potential 13.0.1 if we spin that. I'd assume this to affect relatively few addons.
Per discussions in the channel meeting today, I don't think this has a reproducible case. Can someone please clarify if and how QA can verify this fix?
The majority of add-ons use XHR in a way that is affected by this bug. However, this can only be reproduced (sometimes) by closing a window in a middle of a request, so it is very unlikely to happen consistently and it would be almost impossible to reproduce. This bug will probably happen a lot, but for various different users and various different add-ons. It's unlikely that it will constantly annoy a single user.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #30) > Per discussions in the channel meeting today, I don't think this has a > reproducible case. Can someone please clarify if and how QA can verify this > fix? I had a reproducible case that I used to verify that the patch here actually fixed bug 735688. (see bug 735688 comment 11)
Another issue here is that the workaround that was placed on MDN (to use hiddenDOMWindow) doesn't actually work. hiddenDOMWindow apparently doesn't have chrome privileges so cross site XHR's fail. So there's really no good way for an add-on to work around this problem, short of making sure XHRs are tied to a given browser window. Of course if that browser window is closed, it kills the XHR.
Even before bug 734057 event listeners wouldn't be called if the window was closed.
Before bug 734057, wasn't the XHR object keeping a strong reference to the window that kept it alive? Instantbird (currently based on Mozilla 12) uses the exact same Twitter code as Thunderbird, and we never received any report similar to bug 735688 for Instantbird, so I would be willing to believe that the annoying behavior this bug fixes started (being visible) with Mozilla 13.
(In reply to Florian Quèze from comment #35) > Before bug 734057, wasn't the XHR object keeping a strong reference to the > window that kept it alive? Yes, but that has nothing to do with event handling. If the inner window (which XHR kept alive) became not-the-current-inner-window, then event listeners would be called.
> Even before bug 734057 event listeners wouldn't be called if the window was closed. But clearly something changed because at least two add-ons are broken as a result of 734057. In our case, the XHRs clearly completed after the modal window was closed and called their appropriate onload functions before Firefox 13. With Firefox 13, closing the modal window immediately kills the XHR. You seem to be arguing that we were broke before Firefox 13 as well, but we weren't. The fix for 734057 caused this behavior (or at least made it happen in places where it didn't before).
(In reply to Michael Kaply (mkaply) from comment #37) > > Even before bug 734057 event listeners wouldn't be called if the window was closed. > > But clearly something changed because at least two add-ons are broken as a > result of 734057. > > In our case, the XHRs clearly completed after the modal window was closed > and called their appropriate onload functions before Firefox 13. > > With Firefox 13, closing the modal window immediately kills the XHR. Yes. That is the behavioral change, as I've said before. It was done to prevent certain kinds of leaks. > You seem to be arguing that we were broke before Firefox 13 as well, but we > weren't. We were broken before FF13. It just depends on what you were doing with XHR. Some things worked, some things didn't. In FF13, one more thing broke (or changed)
(Btw, it would be really really great if addon devs would actually test their addons with Aurora and Beta releases.)
XHRs have been bound to a window, when one available, since the end of 2007.
And yes, I should have fixed this bug earlier.
There are components (expressed as JSM or C++ module, for example) which do pure logic, independent of any window, and for all windows as a whole. Some of these need to make network calls to web services. These are typically done via XHR (I wouldn't even know another simple way to make an HTTP call and get XML back). The obvious way is to create an XPCOM component Ci.nsIXMLHttpRequest. If I do that from a backend component, I don't expect this to have anything to do anything with any window whatsoever. Binding this to a window is a bug. You claim the spec says this. If it does, it would be a bug in the spec, but I don't read it there, in fact I read the opposite, the spec talking explicitly about using XHR outside of window contexts: "4.1 Origin and base URL ... This specification defines their [the URL] values when the global object is represented by the Window object. When the XMLHttpRequest object is used in other contexts their values will have to be defined as appropriate for that context. That is considered to be out of scope for this specification." So clearly the spec allows to use XHR without any window. The only references to DOM are EventTarget, an API which in itself has nothing to do with the DOM either. That said, thanks for fixing this bug. It solves the bug for the addon. Please merge this into FF14, because existing functionality broke in FF13.
Source: http://www.w3.org/TR/XMLHttpRequest/#origin-and-base-url (the quoted text is the only reference to windows in the spec.)
Comment on attachment 631303 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:
Attachment #631303 - Flags: approval-mozilla-beta?
Ben, if you want your approval requests to be taken seriously, you need to actually fill in the info approval requests ask for. Otherwise drivers can't evaluate them sanely....
Well, that was in the 2 comments right before that. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 734057 User impact if declined: XHRs from logic components are silently aborted, breaking popular addons, Twitter in Thunderbird. XHR is a core component needed in many places, and a good deal of them are backend components, which all potentially break (in some cases) without this fix. For example, in one addon, the user does to a pref dialog and adds and accounts and logs in, and the login fails (and won't be repeated) when the user closes the pref window before the login call finished. Before FF13, this worked fine. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:
(In the example above, the XHR happens from a JSM, not directly from the window, so arguably the addon code is written reasonably, and used to work.)
Comment on attachment 631303 [details] [diff] [review] patch [Triage Comment] Recent regression in add-ons, approving for beta.
Attachment #631303 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Florian Quèze from comment #35) > Instantbird (currently based on Mozilla 12) uses the exact same Twitter code > as Thunderbird, and we never received any report similar to bug 735688 for > Instantbird, so I would be willing to believe that the annoying behavior > this bug fixes started (being visible) with Mozilla 13. Instantbird has just been updated to Mozilla 13.0.1 and now I can reproduce bug 735688 on Instantbird too.
Is there a way QA can verify this fix in Firefox?
Whiteboard: [qa+] → [qa?]
I don't think we have clear steps to reproduce, so we can probably remove the QA flags.
Flagging qa- based on comment 53.
Whiteboard: [qa?] → [qa-]
The hiddenWindow approach doesn't work on current Aurora (17), since XHR simply disappeared from hiddenDOMWindow... I don't know what and when the xhr is gone. Any clues?
Ouch, I mean Linux. On OS X the xhr in hiddenDOMWindow still works.
This bug was fixed in Firefox 14. Why do you need to do anything with the idden window in Firefox 17?
On Linux, you're touching the hidden window via an Xray, presumably. It's _possible_ that XHR doesn't show up through an Xray. Worth checking, at least.
And by "XHR" I mean "WebIDL objects".
Was there any version that method actually did work on? I haven't tried it in a while, but all I hear from people are complaints that it doesn't work because of cross-origin issues.
You need to log in before you can comment on or make changes to this bug.