I've been saying I'd do this since we decided to do it in march. And now I've finally started really working on it. The basic idea is that we don't want Components to be exposed to web content, but we still need it in content scopes because XBL bindings are cloned into content scopes, and they (occasionally) need it for QI and Components.lookupMethod. So we define Components on a special, hidden object, and splice that object onto the scope chain for cloned XBL methods. Assuming the XBL code doesn't manually leak the Components object, there should be no other-way (from a object-capability perspective) for web content to get its hands on |Components|. Conceptually this is pretty simple, but there are hiccups. One of them is that anonymous XBL methods (constructors and destructors) already use the bound element on the scope chain. Jorendorff is going to expose an API that allows us to use |With| objects for this in bug 790676. Another hitch is what to about all the existing code in our test suite that uses Components. We can't simply expose Components as SpecialPowers.wrap(systemWindow.Components), because that means that Components.interfaces.nsIFoo isn't a bonafide nsJSIID object, which break QI. So as a first step, I'm going to tweak the enablePrivilege code to just define the Components object on the window, and also add a SpecialPowers mechanism to get the local Components object. Hopefully those two together should give put us in a decent place.

> anonymous XBL methods (constructors and destructors) already use the bound element Why is this a problem? You can handle that today, without new APIs, with pseudocode like so: scopeObj = JS_NewObject(cx, nullptr, nullptr, element); JS_DefineProperty(cx, scopeObj, "Components", ...); // Use scopeObj as the parent for your method It'll change behavior in cases when the element or the document had a property called "Components", but those should be quite rare I would think.

(In reply to Boris Zbarsky (:bz) from comment #1) > > anonymous XBL methods (constructors and destructors) already use the bound element > > Why is this a problem? You can handle that today, without new APIs, with > pseudocode like so: > > scopeObj = JS_NewObject(cx, nullptr, nullptr, element); > JS_DefineProperty(cx, scopeObj, "Components", ...); > // Use scopeObj as the parent for your method > > It'll change behavior in cases when the element or the document had a > property called "Components", but those should be quite rare I would think. Yes, but I'm trying to avoid making N XBL scope objects. It doesn't matter too much, but I'd like the ability to make dynamic updates to the XBL scope object if need be, and I don't like the idea of that many Components references floating around. Certainly could work if bug 790676 turns out to be a chore, though.

> Yes, but I'm trying to avoid making N XBL scope objects I'm not sure how bug 790676 will prevent the need for that, exactly... Would you just end up making the scope chain be elements -> document -> window -> singleton (per-compartment?) xbl scope object or something?

The idea is to expose the ability for JSAPI users to make |With| environments (which, perhaps in the future, don't even need to be JS objects). So the scope chain would be: With(element) -> mXBLScopeObject -> window

But right now the scope chain goes through all the element's ancestors and the document before getting to the window. Are you sure you can change that without breaking things?

(In reply to Boris Zbarsky (:bz) from comment #5) > But right now the scope chain goes through all the element's ancestors and > the document before getting to the window. Are you sure you can change that > without breaking things? For HTML documents (where we're actually going to apply this), that parent chain is generally element->document (with the exception of form elements). So we could probably just do With() for each element in the parent chain without significant practical overhead. Or just With(element)->With(document)->... might be good enough.

Oh, we're not doing this for chrome XBL? In that case it might be ok, yeah.

(In reply to Boris Zbarsky (:bz) from comment #7) > Oh, we're not doing this for chrome XBL? In that case it might be ok, yeah. We could, but I'm assuming we might as well avoid introducing extra unnecessary objects for chrome scopes, since it would add up with all the JSMs we have (though, realistically, it's probably negligible).

A lot of this turns out to be getting the test suite fixed up, since lots and lots of tests (that don't use enablePrivilege) still QI using Components.interfaces. I've got some approaches. Trying now: https://tbpl.mozilla.org/?tree=Try&rev=7d2e6d98c3bb

bz, sheppy: What kind of communication are we going to need to do for this one? The outcome of this bug will be that |Components| is no longer visible to content. This could break various random things (people checking for Components to do browser-detection, for example). The major thing that this will break (on purpose!) is the ability for content scripts to QI random DOM objects to various interfaces. For this part, we could push out a warning whenever a content script accessed Components.interfaces. Does doing this for one cycle before actually removing it sound necessary and/or sufficient?

(also, smaug says he's seen people getting XHR interface constants off of Components.interfaces.nsIXMLHttpRequest).

Bobby, is this blocked by bug 781689? That bug removes some code in content that does QI and getInterface using things in Components.interfaces.

(In reply to Andrew McCreight [:mccr8] from comment #13) > Bobby, is this blocked by bug 781689? That bug removes some code in content > that does QI and getInterface using things in Components.interfaces. It sure does.

tps://tbpl.mozilla.org/?tree=Try&rev=2133df1c1ea8

Joel, one thing about this patch is that it's going to bork the dozen-or-so crashtests that use |Components|. Now, those crashtests are green right now, because currently they'll just throw when they access the undefined property and the test will finish, which is considered PASS for crashtests. But they're not testing what they're supposed to. You had a patch to hook SpecialPowers up to the reftest harness, right? Is there any way we could still do that, and just skip the jsreftest bit that was causing problems?

The patch I had in in bug 762908. The problem with it is that it was only tested on jsreftests which there was a couple failures that kept cropping up as a side effect.

(In reply to Joel Maher (:jmaher) from comment #18) > The patch I had in in bug 762908. The problem with it is that it was only > tested on jsreftests which there was a couple failures that kept cropping up > as a side effect. Ok. Do you think it's worth repurposing that patch to make SpecialPowers available to crashtests? Seems like an easy change, and most certainly worthwhile IMO (and I'm sure Jesse would agree).

since it doesn't work in jsreftests, we might actually make it work for crashtests :)

(In reply to Joel Maher (:jmaher) from comment #20) > since it doesn't work in jsreftests, we might actually make it work for > crashtests :) I filed bug 792029 for this. IIRC, the reason it didn't work for jsreftests wasn't in the making-SpecialPowers-available part, but in the using-SpecialPowers-to-rewrite-the-harness part. It seems like converting this to be available to crashtests without mucking with harness code should be a 5-minute job.

Ok, this is green on try now (at least for linux-debug). I've decided to split the testsuite-wrangling into bug 792036, which can land separately as a prereq.

> What kind of communication are we going to need to do for this one? You want to talk to Jorge, I think.

(In reply to Boris Zbarsky (:bz) from comment #23) > > What kind of communication are we going to need to do for this one? > > You want to talk to Jorge, I think. For addon compat? It seems to me like that's not likely to be the biggest issue here (since addons are generally privileged). I'd think that web compat would be the bigger issue (in the same vein as enablePrivilege), but then again I really have no idea.

Could you add a telemetry probe to get some more information?

(In reply to Olli Pettay [:smaug] from comment #25) > Could you add a telemetry probe to get some more information? We could, though I'm not sure what kind of delay that would impose on this project. Is it worth doing a telemetry probe just on Nightly? Though at that point, we almost might as well just land it and see if the breakage is significant enough to warrant a backout and more outreach. This stuff has to go away sooner or later...

I landed telemetry and a console warning in bug 795275 for mozilla18, which recently became the release version. Originally, about 20% of Nightly/Aurora users were hitting an access to Components somewhere in their session. About 2 weeks before FF18 was released, the number dropped significantly, which presumably meant that one or more major websites noticed the warning during testing and fixed it. The numbers are now 10-11%, and hopefully will fall more. This goes to show the effectiveness of console warnings!

I found this patch lying around. It's going to need to change due to the recent XBL scope stuff that landed, but I wanted to post it here to avoid forgetting about it.

Now that we have XBL scopes, we no longer need any extra machinery from the JS engine to do this.

Changing the bug summary, since the XBL scope stuff means that we can just turn it off for content and XBL will still work.

Hm, not sure how that happened. Anyway, pushing again: https://tbpl.mozilla.org/?tree=Try&rev=c6401e6e5dce

I added this when I thought we'd be defining Components on an object on the XBL scope chain. At this point, it's not necessary anymore.

This method is larely deprecated. The only two consumers are JSD and the setup for the safe JSContext, neither of which use system principal and thus neither of which should have |Components|.

\o/

This may break greasemonkey scripts that depend on inheriting |Components| from the content window. The compat solution for now is just to pass { wantComponents: true } as a sandboxOption when creating the sandbox for content script evaluation.

Thanks for the heads up. By quick estimate from my last snapshot of userscripts.org: about 500 of 80,000 scripts do this. Mostly platypus generated ones, but not exclusively.

(In reply to Anthony Lieuallen from comment #40) > Thanks for the heads up. By quick estimate from my last snapshot of > userscripts.org: about 500 of 80,000 scripts do this. Mostly platypus > generated ones, but not exclusively. By "do this", I assume you mean accessing Components in content?

Yes-ish. I mean are (the source of) a user script which contains the string "Components.", not preceded by "//".

Comment on attachment 717966 [details] [diff] [review] Part 4 - Omit Components object for content scopes. v1 \o/

Try push on all platforms to flush out any last dependencies in the test suite: https://tbpl.mozilla.org/?tree=Try&rev=e02eec70a603 I'll send some mail to dev-platform letting people know this is coming.

I _think_ this try run looks good, except for two late-breaking usages of Components in M-5 which I've now fixed. There are some other oranges, but I'm pretty sure they're due to dbaron's predicted orange spike due to NS_ASSERTION changes and not due to this patch. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1f22477180 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cac9eeba156e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/68c3cee0d464 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00b10e10d356 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8d36cce3e3

Unfortunately, this appears to have caused talos-other hangs/crashes. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/bef7c417aa93

For reference, these are some of the logs: https://tbpl.mozilla.org/php/getParsedLog.php?id=20354319&tree=Mozilla-Inbound&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=20354319&tree=Mozilla-Inbound&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=20354134&tree=Mozilla-Inbound&full=1

So, I fixed the Talos stuff, but I've held off on relanding because I wanted to sort out the compat issues. The consensus on dev-platform seems to be that we want to shim in support for the _existence_ of a Components object, as well as the associated interface constants. It also seems good to have this behavior behind a pref. So I've now done that. I've re-organized my patches here such that we no longer back out the warning/telemetry, but instead make the associated tests for that stuff pref-controlled. I'm also planning on landing this stuff preffed off, and enabling the pref in a followup push. Patches coming up.

Comment on attachment 727969 [details] [diff] [review] Part 4 - Define a lazily-resolved shim for Components. v1 Review of attachment 727969 [details] [diff] [review]: ----------------------------------------------------------------- r=me with my questions answered or addressed. ::: dom/base/nsDOMClassInfo.cpp @@ +5223,5 @@ > + const char *geckoName; > + const char *domName; > +}; > + > +const InterfaceShimEntry kInterfaceShimMap[] = This needs a comment explaining which interfaces were chosen and why. @@ +5260,5 @@ > + { "nsIDOMXULCheckboxElement", "XULCheckboxElement" }, > + { "nsIDOMXULPopupElement", "XULPopupElement" } }; > + > +const uint32_t kInterfaceShimCount = > + sizeof(kInterfaceShimMap) / sizeof(kInterfaceShimMap[0]); I'm sure there's either a macro or a template function that does this for us. @@ +5269,5 @@ > + // Create a fake Components object. > + JSObject *components = JS_NewObject(cx, nullptr, nullptr, global); > + NS_ENSURE_TRUE(components, NS_ERROR_OUT_OF_MEMORY); > + bool ok = JS_DefineProperty(cx, global, "Components", JS::ObjectValue(*components), > + JS_PropertyStub, JS_StrictPropertyStub, JSPROP_ENUMERATE); Components is currently readonly/permanent. Should the shim likewise be non-configurable? @@ +5277,5 @@ > + JSObject *interfaces = JS_NewObject(cx, nullptr, nullptr, global); > + NS_ENSURE_TRUE(interfaces, NS_ERROR_OUT_OF_MEMORY); > + ok = JS_DefineProperty(cx, components, "interfaces", JS::ObjectValue(*interfaces), > + JS_PropertyStub, JS_StrictPropertyStub, > + JSPROP_ENUMERATE | JSPROP_PERMANENT); (Here and below) Why PERMANENT without READONLY? @@ +5320,5 @@ > return NS_OK; > } > > + if (id == XPCJSRuntime::Get()->GetStringID(XPCJSRuntime::IDX_COMPONENTS)) { > + *_retval = true; Nit: I think *_retval is guaranteed to be true on entry to this function.

(In reply to Blake Kaplan (:mrbkap) from comment #55) > This needs a comment explaining which interfaces were chosen and why. Ok. > I'm sure there's either a macro or a template function that does this for us. Do you happen to know what that might be? > > + // Create a fake Components object. > > + JSObject *components = JS_NewObject(cx, nullptr, nullptr, global); > > + NS_ENSURE_TRUE(components, NS_ERROR_OUT_OF_MEMORY); > > + bool ok = JS_DefineProperty(cx, global, "Components", JS::ObjectValue(*components), > > + JS_PropertyStub, JS_StrictPropertyStub, JSPROP_ENUMERATE); > > Components is currently readonly/permanent. Should the shim likewise be > non-configurable? I don't think so. In particular, that will break the UniversalXPConnect hack that defines Components in the content scope as soon as enablePrivilege is called, because it won't be able to redefine the property. More to the point, this thing is just a shim, so I don't see a huge reason to do anything like that. > (Here and below) Why PERMANENT without READONLY? Happy to either remove PERMANENT or add READONLY. Which would you prefer? The latter is probably better, I guess... > Nit: I think *_retval is guaranteed to be true on entry to this function. Ok, I'll MOZ_ASSERT it.

(In reply to Bobby Holley (:bholley) from comment #56) > Do you happen to know what that might be? MOZ_ARRAY_LENGTH appears to be the way to go. > Happy to either remove PERMANENT or add READONLY. Which would you prefer? > The latter is probably better, I guess... Yeah, I'd prefer adding READONLY.

(In reply to Blake Kaplan (:mrbkap) from comment #57) > (In reply to Bobby Holley (:bholley) from comment #56) > > Do you happen to know what that might be? > > MOZ_ARRAY_LENGTH appears to be the way to go. Looks like mozilla::ArrayLength is even better :-) https://tbpl.mozilla.org/?tree=Try&rev=98dce9438d21

Looks like that test had a load ordering race condition which manifested only on try. Fixed that, and pushed to inbound: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/29acf1494fed remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/48bc6259ca24 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0f76ecfefd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fdc85753f3a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a94932a1fa7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6eef579bea7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5aabc5770767 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e59146e161e Note that I'm landing this prefed off to make sure the infrastructure sticks, and will land a followup patch to twiddle the pref. I'm going to do them in the same bug to avoid confusion, so marking [leave open] for now.

Try push for flipping the pref: https://tbpl.mozilla.org/?tree=Try&rev=dfabfc7a0681

(In reply to Bobby Holley (:bholley) from comment #58) > https://tbpl.mozilla.org/?tree=Try&rev=98dce9438d21 In a flabbergasting state of affairs, the test which failed on your push to try also failed when you pushed to inbound. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8695e3fd2d34

btw, had you included Android, you would have found that test failing in mochitest-8, if that proves to be useful information.

(In reply to Blake Kaplan from comment #55) > > +const uint32_t kInterfaceShimCount = > > + sizeof(kInterfaceShimMap) / sizeof(kInterfaceShimMap[0]); > > I'm sure there's either a macro or a template function that does this for us. Except on compilers that support constexpr, the template function will cause a static initaliser here, so the macro is preferred. Also you probably want static const so it's a true constant and not just a variable that happens to live in read-only memory.

I thought the test (added in the push) was failing due to a loading race condition, because it didn't reproduce locally. But it looks like it was a path issue that manifested only when running the whole suite (rather than an individual test). remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b54db9ddb6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dfcbbcd176 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2998da65e3a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2f668e489d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a835569488a8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/68842f61d58b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca7defc8a0f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab716af95256

https://hg.mozilla.org/mozilla-central/rev/e5b54db9ddb6 https://hg.mozilla.org/mozilla-central/rev/d5dfcbbcd176 https://hg.mozilla.org/mozilla-central/rev/a2998da65e3a https://hg.mozilla.org/mozilla-central/rev/7b2f668e489d https://hg.mozilla.org/mozilla-central/rev/a835569488a8 https://hg.mozilla.org/mozilla-central/rev/68842f61d58b https://hg.mozilla.org/mozilla-central/rev/9ca7defc8a0f https://hg.mozilla.org/mozilla-central/rev/ab716af95256

(In reply to Bobby Holley (:bholley) from comment #66) > https://tbpl.mozilla.org/?tree=Try&rev=323749bbcbc2 Looks green for flipping the pref. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2047c6e723 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f379f0acbbc0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1892fbe77d9 \o/

Hm, this appears to be on m-c now. Should it be marked FIXED?

Doesn't look like comment #67 made it over to m-c yet...

(In reply to Ryan VanderMeulen [:RyanVM] from comment #69) > Doesn't look like comment #67 made it over to m-c yet... Ah, right! I thought bug 856257 was a regression from flipping the pref, but it was actually a regression from one of the patches in the first set. Sorry for the confusion.

https://hg.mozilla.org/mozilla-central/rev/3e2047c6e723 https://hg.mozilla.org/mozilla-central/rev/f379f0acbbc0 https://hg.mozilla.org/mozilla-central/rev/f1892fbe77d9

Release noted for FF22 as "For user privacy, the |Components| object is no longer accessible from web content".

Can we remove ComponentsWarning now? http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#118

I think we should display the warning when the Components shim is used. https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?rev=e1cb74a38196#4480 Currently only telemetry is accumulated.

(In reply to Masatoshi Kimura [:emk] from comment #75) > I think we should display the warning when the Components shim is used. > https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo. > cpp?rev=e1cb74a38196#4480 > Currently only telemetry is accumulated. Yeah, that's probably a good idea. I'm unlikely to get to it in the near term, but I'd take a patch.