756277 - XMLHttpRequest started from Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] ends up bound to some random window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is an old bug. I'll figure out some way to fix this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 4

[Mike Kaply [:mkaply]]

This would impact add-ons severely.

We can't have XHR's randomly failing depending on what window they happened to get attached to.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Yes. But this is an ancient bug, dates back at least to FF1.5, IIRC. So nothing catastrophic hasn't happened.

Comment 6

[Mike Kaply [:mkaply]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Yes. But this is an ancient bug, dates back at least to FF1.5, IIRC. So
> nothing catastrophic hasn't happened.

This bug was opened on February 7, 2012.

The documentation here:

https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#Using_XMLHttpRequest_from_JavaScript_modules_.2F_XPCOM.C2.A0components

Was recently changed to tell add-on developers to stop using Components.interfaces.nsiXMLHttpRequest and instead use 

const { XMLHttpRequest } = Components.classes["@mozilla.org/appshell/appShellService;1"]
                                     .getService(Components.interfaces.nsIAppShellService)
                                     .hiddenDOMWindow;
var req = XMLHttpRequest();


It also says:

Warning: The following code will create an XMLHttpRequest associated with the nearest available window object. If this window is closed then it will cancel your XMLHttpRequest.


So if add-on developers are being told to stop using one way, and the new method randomly fails, that's a problem.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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*.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

...FF 2.0 certainly had this behavior.

Also, I'm expecting to first fix this sort-of-random behavior somehow.

Comment 9

[Mike Kaply [:mkaply]]

Neil changed it on May 18

https://developer.mozilla.org/index.php?title=en/DOM/XMLHttpRequest/Using_XMLHttpRequest&action=diff&revision=148&diff=149

Comment 10

[Mike Kaply [:mkaply]]

> 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?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

https://tbpl.mozilla.org/?tree=Try&rev=653a7880da71

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2

https://tbpl.mozilla.org/?tree=Try&rev=8e37e263cb43

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631303 [details] [diff] [review]
patch

r=me

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/95d1bb200f4e

Comment 18

[Florian Quèze [:florian]]

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.

Comment 19

[Alex Keybl [:akeybl]]

This patch needs a risk evaluation before being considered for approval.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Should be reasonable safe, but might affect some addons which, for some strange reason, rely on
the old behavior.

Comment 21

[Mike Kaply [:mkaply]]

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.

Comment 22

[Mike Kaply [:mkaply]]

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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(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.)

Comment 24

[Mike Kaply [:mkaply]]

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 25

[Alex Keybl [:akeybl]]

Comment on attachment 631303 [details] [diff] [review]
patch

[Triage Comment]
Let's land this on Aurora in preparation for possible beta/release uplift.

Comment 26

[Benjamin Smedberg]

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.

Comment 27

[Alex Keybl [:akeybl]]

(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!

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 30

[u279076]

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?

Comment 31

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 32

[Florian Quèze [:florian]]

(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)

Comment 33

[Mike Kaply [:mkaply]]

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.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Even before bug 734057 event listeners wouldn't be called if the window was closed.

Comment 35

[Florian Quèze [:florian]]

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.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 37

[Mike Kaply [:mkaply]]

> 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).

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(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)

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

(Btw, it would be really really great if addon devs would actually test their addons with Aurora and 
Beta releases.)

Comment 40

[Mike Kaply [:mkaply]]

> (Btw, it would be really really great if addon devs would actually test their addons with Aurora and 
Beta releases.)

I resent that remark. We do extensive testing with all builds including nightlies. This was a very unique scenario in our add-on.

And BTW, it would be really great if Firefox devs would actually listen when someone opens a bug (like this one) indicating they saw different behavior than they did before. This bug was found as a result of 734057 (almost two months ago) and you simply ignored it and kept claiming that it wasn't a new bug. 


Can you please explain to me how a call from a Javascript moduile to instantiate a Javascript component is so dependent on having a window?

That's the part that really bothers me about this.

Are you saying that if you build a headless Firefox, XHRs don't work?

What changed to make XHRs so dependent on windows? Have they always been this way?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

XHRs have been bound to a window, when one available, since the end of 2007.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

And yes, I should have fixed this bug earlier.

Comment 43

[Ben Bucksch (:BenB)]

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.

Comment 44

[Ben Bucksch (:BenB)]

Source: http://www.w3.org/TR/XMLHttpRequest/#origin-and-base-url
(the quoted text is the only reference to windows in the spec.)

Comment 45

[Ben Bucksch (:BenB)]

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:

Comment 46

[Boris Zbarsky [:bzbarsky]]

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....

Comment 47

[Ben Bucksch (:BenB)]

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:

Comment 48

[Ben Bucksch (:BenB)]

(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 49

[Alex Keybl [:akeybl]]

Comment on attachment 631303 [details] [diff] [review]
patch

[Triage Comment]
Recent regression in add-ons, approving for beta.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/787d44019d72
https://hg.mozilla.org/releases/mozilla-beta/rev/aa1fd608d88a

Comment 51

[Florian Quèze [:florian]]

(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.

Comment 52

[Ioana (away)]

Is there a way QA can verify this fix in Firefox?

Comment 53

[Jorge Villalobos [:jorgev] (he/him)]

I don't think we have clear steps to reproduce, so we can probably remove the QA flags.

Comment 54

[u279076]

Flagging qa- based on comment 53.

Comment 55

[Hsiao-Ting Yu (Littlebtc)]

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?

Comment 56

[Hsiao-Ting Yu (Littlebtc)]

Ouch, I mean Linux. On OS X the xhr in hiddenDOMWindow still works.

Comment 57

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug was fixed in Firefox 14.  Why do you need to do anything with the idden window in Firefox 17?

Comment 58

[Mike Kaply [:mkaply]]

I'm guessing he's following the docs:

https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest/Using_XMLHttpRequest

Scroll to the bottom to 

Using XMLHttpRequest from JavaScript modules / XPCOM components

Comment 59

[Boris Zbarsky [:bzbarsky]]

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.

Comment 60

[Boris Zbarsky [:bzbarsky]]

And by "XHR" I mean "WebIDL objects".

Comment 61

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Michael Kaply (mkaply) from comment #58)
> I'm guessing he's following the docs:
> 
> https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest/
> Using_XMLHttpRequest
> 
> Scroll to the bottom to 
> 
> Using XMLHttpRequest from JavaScript modules / XPCOM components

Sigh.

I edited the docs.

Comment 62

[Kris Maglione [:kmag]]

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.