Last Comment Bug 741367 - Creating second XMLHttpRequest via Components.Constructor throws NS_ERROR_FAILURE
: Creating second XMLHttpRequest via Components.Constructor throws NS_ERROR_FAI...
: addon-compat, regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
: 747682 (view as bug list)
Depends on:
Blocks: abp 740069
  Show dependency treegraph
Reported: 2012-04-02 05:50 PDT by Wladimir Palant
Modified: 2012-06-22 05:08 PDT (History)
14 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Restartless extension for testing (2.21 KB, application/x-xpinstall)
2012-04-02 05:50 PDT, Wladimir Palant
no flags Details
v1 (1.03 KB, patch)
2012-04-02 06:45 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v2 (1.03 KB, patch)
2012-04-05 14:58 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v2 (1.12 KB, patch)
2012-05-02 06:56 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wladimir Palant 2012-04-02 05:50:55 PDT
Created attachment 611424 [details]
Restartless extension for testing

A change in the recent nightlies broke subscription downloads in Adblock Plus. The reduced testcase looks like this:

> var XMLHttpRequest = Components.Constructor(";1", "nsIXMLHttpRequest");
> new XMLHttpRequest();
> new XMLHttpRequest();

The attempt to create a second XMLHttpRequest with the same constructor throws NS_ERROR_FAILURE. This only happens in the context of a JavaScript code module, running the same code in a sandbox or in the window context works fine. Also, it only seems to affect XMLHttpRequest - I tried to create an nsIFile instance and that worked. And using createInstance() directly works as well, the issue only affects Components.Constructor.

A restartless extension to test this is attached. The important code is in Module.js, it will run every time this extension starts up. Normally it should print "XMLHttpRequest objects created successfully" to Error Console, instead it prints "Error creating XMLHttpRequest objects" (an exception occurred in the code).

An Adblock Plus user says that the first hourly build to experience this issue is 1333220903, this means that the regression range is I suspect bug 740069 to be the cause.
Comment 1 Peter Van der Beken [:peterv] 2012-04-02 06:45:30 PDT
Created attachment 611438 [details] [diff] [review]
Comment 2 Boris Zbarsky [:bz] (TPAC) 2012-04-02 09:52:50 PDT
This doesn't quite seem right...  In particular, wouldn't JS_HasProperty test true for a Window which has a <div id="XMLHttpRequest"> in it?

We really want a JS_HasOwnProperty here.  Maybe.  At least in the window context that should work better.  Modulo whatever happens when you reenter resolve hooks, of course.
Comment 3 Peter Van der Beken [:peterv] 2012-04-02 11:30:43 PDT
Yup, it should be HasOwnProperty.
Comment 4 Peter Van der Beken [:peterv] 2012-04-05 14:58:15 PDT
Created attachment 612696 [details] [diff] [review]

For some reason I can't make this fail in an xpcshell test :-/.
Comment 5 Peter Van der Beken [:peterv] 2012-04-05 14:59:06 PDT
Comment on attachment 612696 [details] [diff] [review]

Actually, I forgot to test the id case.
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-04-05 19:13:12 PDT
And my question about resolve hook reentry stands, btw....
Comment 7 Wladimir Palant 2012-04-23 09:05:15 PDT
*** Bug 747682 has been marked as a duplicate of this bug. ***
Comment 8 Boris Zbarsky [:bz] (TPAC) 2012-04-23 09:27:51 PDT
This needs to happen for fx14, since it breaks Adblock Plus if nothing else.
Comment 9 Wladimir Palant 2012-04-23 09:34:55 PDT
I stopped using Components.Constructor in Adblock Plus, new version should be released soon. Still, this is quite a bit of platform breakage - XMLHttpRequest should be the most common use of Components.Constructor.
Comment 10 Peter Van der Beken [:peterv] 2012-04-23 11:25:54 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> And my question about resolve hook reentry stands, btw....

The JS engine already protects resolve hooks, see AutoResolving.
Comment 11 Boris Zbarsky [:bz] (TPAC) 2012-04-23 11:30:51 PDT
Yes, the question was what that protection will end up doing in this case.
Comment 12 Peter Van der Beken [:peterv] 2012-04-23 11:36:05 PDT
Ah, I think it makes the JS_AlreadyHasOwnProperty not find the property.
Comment 13 Nils Maier [:nmaier] 2012-04-25 16:10:47 PDT
Definitively an addon-compat worthy issue. CC'ing Jorge.
Comment 14 Peter Van der Beken [:peterv] 2012-05-02 06:56:58 PDT
Created attachment 620297 [details] [diff] [review]

This doesn't change anything in the order of name resolution on windows from before the landing of the new DOM bindings.

I think I've answered the questions about this patch, let me know if I didn't.
Comment 15 Boris Zbarsky [:bz] (TPAC) 2012-05-02 11:56:25 PDT
Comment on attachment 620297 [details] [diff] [review]

Comment 16 Peter Van der Beken [:peterv] 2012-05-03 06:56:40 PDT
Comment 17 Ed Morley [:emorley] 2012-05-04 04:02:01 PDT
Comment 18 Peter Van der Beken [:peterv] 2012-05-04 04:25:28 PDT
Comment on attachment 620297 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): bug 740069
User impact if declined: Broken addons (Addblock Plus was fixed, but this might affect others)
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: None
Comment 19 Nils Maier [:nmaier] 2012-05-04 06:56:49 PDT
(In reply to Peter Van der Beken [:peterv] from comment #18)
> User impact if declined: Broken addons (Addblock Plus was fixed, but this
> might affect others)

It in fact does affect other add-ons, at least 3 of mine, having 2M+ ADU. addon-mxr shows a bunch of other users. Not sure how much are indeed affected, as this requires calling the constructor at least twice.
Comment 20 Alex Keybl [:akeybl] 2012-05-06 19:20:36 PDT
Comment on attachment 620297 [details] [diff] [review]

[Triage Comment]
New regression in FF14 that has the potential to affect a large population. Approving for Aurora 14.
Comment 21 Peter Van der Beken [:peterv] 2012-05-10 05:14:45 PDT
Comment 22 Virgil Dicu [:virgil] [QA] 2012-06-22 05:08:49 PDT
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Verified in F14 beta 8, on Ubuntu 12.04, Windows 7 and Mac OS 10.6 using attachment test case.

Timestamp: 06/22/2012 02:57:09 PM
Error: XMLHttpRequest objects created successfully
Source File: jar:file:///home/virgildicu/.mozilla/firefox/d4sz6vma.qdd/extensions/!/Module.js
Line: 15

Note You need to log in before you can comment on or make changes to this bug.