In bug 790732, I'm planning on removing the |Components| object from unprivileged scopes. Most of the sub-objects on Components have long been guarded by a security check, but Components.interfaces (along with a few odds and ends like Components.lookupMethod) are accessible to page script. This means that pages can QI DOM nodes to all sorts of stuff, which is nasty. One of the major things preventing us from doing this up until now was that XBL code runs with page principal, and needs to be able to QI (a11y breaks if we disallow this). So the solution over in bug 790732 is to make Components a property on an object available only on the scope chain of XBL functions. A major hurdle here though is that our test suite makes copious use of Components.interfaces (and a few other things). Moreover, most of our conversion of mochitests away from enablePrivilege has relied on defining something like |const Cc = SpecialPowers.wrap(Components)|. This is well-intentioned, but will break when |Components| disappears. So here's the strategy here: 1 - Refactor some XPConnect code so that we can provide an API on Components.utils called Cu.getComponentsForScope(aScope). This will fetch the Components object that's normally hidden away. Because it's on Cu, it's only available to privileged callers. 2 - Instrument the SpecialPowers API to define window.SpecialPowers.Components = Cu.getComponentsForScope(window). This is the raw Components object. In general, use of it will be discouraged, as we'll define SpecialPowers.{Cc,Ci,Cu,Cr,Cu} which we'll encourage people to use directly. Aside from Ci, all of these are SpecialPowers.wrap()-ed versions of the relevant objects, meaning that they can be used to do privileged things transparently from content tests. Ci is unwrapped, because in general it's used as an argument to QI, and passing in a wrapped object breaks things. 3 - Manually fix up some tests. There are hundreds and hundreds of unprivileged tests in our suite that use the Components object - too many to fix by hand. So I've implemented a giant Regexp that does mostly of the job. But rather than spending a lot of time making it handle corner cases, I've made some patches that just manually fix the corner cases (and do some code reformatting to put relevant context on the same line). This patch was hand-coded and should be quickly reviewable. 4 - Run the aforementioned regexp on the test suite, now that the corner-cases are removed. This generates a giant diff, which can probably be reviewed by scanning and reviewing the regexp itself. I'm going to start by getting Blake and Ted's review on the underlying infrastructure, and then flag relevant people for the rest of the test reviews. Given that "the whole test suite" is a bit of a moving target, fast reviews would be appreciated here. ;-)

The patches as I have now are green on try: https://tbpl.mozilla.org/?tree=Try&rev=dcd7c1955e0a (the m-2 issue has been fixed).

Oh, also: I'm only fixing up tests that don't call enablePrivilege. For those that do, we'll actually define the raw Components object on |window| during the call to enablePrivilege.

This isn't totally necessary for these patches, but I want to be able to dynamically attach Components in my XBL patches, and there were some merge conflicts re-ordering them. So let's just do this here. From the commit message: The aGlobal API is currently unnecessary, since it should always be equal to the global obtained from the scope. But we'll want to manually specify the target object in subsequent patches, so make it an optional argument that's currently never passed.

This allows the API in the upcoming patch.

Now that window.Components is no longer acccessible to page JS, we need a way to access it in mochitests. So this patch provides SpecialPowers.Components, which is the bonafide Components object for the window upon which SpecialPowers is defined. It also provides SpecialPowers.{Cc,Ci,Cr,Cu}, which are SpecialPowers-wrapped versions of their respective sub-objects (with the exception of Ci, which is unwrapped). Flagging ted for review on the SpecialPowers bits, mrbkap for review on the XPConnect bits.

How hard would it be to replace the QI to nsIDOMWindowUtils with the direct SpecialPowers access to it? That would look nicer and more future-proof. For instance, I see this in the patch: 23.12 - window.QueryInterface(Components.interfaces.nsIInterfaceRequestor) 23.13 - .getInterface(Components.interfaces.nsIDOMWindowUtils) 23.14 + window.QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor) 23.15 + .getInterface(SpecialPowers.Ci.nsIDOMWindowUtils)

(In reply to Andrew McCreight [:mccr8] from comment #6) > How hard would it be to replace the QI to nsIDOMWindowUtils with the direct > SpecialPowers access to it? That would look nicer and more future-proof. Probably easy, but out of scope here. I'm trying to make the minimal set of changes required to get this stuff green, and that's still a ton of changes. Believe me, there are dozens of low-risk fixes I've wanted to do "on the way", but one out of ten will break something in some unforeseen way, which slows down a big operation like this one. I'd r+ such a regexp in a followup bug in a heartbeat, though. ;-)

I did a push here just to make sure this is also green without the XBL patches (which were contained in my last try push): https://tbpl.mozilla.org/?tree=Try&rev=240fe8980087

(In reply to Bobby Holley (:bholley) from comment #7) > Probably easy, but out of scope here. Sounds reasonable. If you could post your regexps somewhere that would be helpful.

The regexp I use for each automated commit is in the commit message. For reference, here's the most up-to-date one: find /objdir/_tests/testing/mochitest/tests/ | egrep "\.(xhtml|html|xml|js)$" | grep -v SimpleTest | grep -vi mochikit | grep -v simpleThread | grep -v test_ipc_messagemanager_blob.html | grep -v "indexedDB/test" | xargs grep -l Components | xargs grep -L enablePrivilege | perl -pe 's#.*mochitest/tests/##' | xargs perl -p -i.bakkk -e 's/Components\.interfaces(\s|;|\.|\[)/SpecialPowers\.Ci$1/g, s/SpecialPowers\.wrap\(Components\)\.(.)(lasses|tils|nterfaces|esults)/SpecialPowers.C$1/g, s/(?<![\.a-zA-Z])Components/SpecialPowers\.Components/g, s/window\.Components/window\.SpecialPowers\.Components/g' I'll dissect it in detail for the reviewer of the automated patch when the time comes ;-)

Comment on attachment 662162 [details] [diff] [review] Part 3 - Add a SpecialPowers API to provide access to the Components object in various forms. v1 Review of attachment 662162 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +17,5 @@ > > +// This file is used both in privileged and unprivileged contexts, so we have to > +// be careful about our access to Components.interfaces. We also want to avoid > +// naming collisions with anything that might be defined in the scope that imports > +// this script. This is sort of ugly, but I guess so is most of our code that gets used in both regular mochitests and mochitest-chrome.

This fixes up the indexedDB tests. Flagging bent for review.

I want gabor to look at these.

This is the kitchen sink of manual fixes that the ensuing auto-generation can't deal with. Some of them just fix up formatting (such as Components.\nFoo, so that subsequent auto-generation can work better). Sorry for the mess, Andrew. Hopefully it's not too annoying to review.

Here's the patch of automated fixes. It probably just needs a quick skim. Here's the full regexp: find /objdir/_tests/testing/mochitest/tests/ | egrep "\.(xhtml|html|xml|js)$" | grep -v SimpleTest | grep -vi mochikit | grep -v simpleThread | grep -v test_ipc_messagemanager_blob.html | grep -v "indexedDB/test" | xargs grep -l Components | xargs grep -L enablePrivilege | perl -pe 's#.*mochitest/tests/##' | xargs perl -p -i.bakkk -e 's/Components\.interfaces(\s|;|\.|\[)/SpecialPowers\.Ci$1/g, s/SpecialPowers\.wrap\(Components\)\.(.)(lasses|tils|nterfaces|esults)/SpecialPowers.C$1/g, s/(?<![\.a-zA-Z])Components/SpecialPowers\.Components/g, s/window\.Components/window\.SpecialPowers\.Components/g' Here's a breakdown: > find /objdir/_tests/testing/mochitest/tests/ Find the mochitest files that actually get staged. > | egrep "\.(xhtml|html|xml|js)$" Grep for reasonable suffixes. > | grep -v SimpleTest | grep -vi mochikit | grep -v simpleThread | grep -v test_ipc_messagemanager_blob.html | grep -v "indexedDB/test" Filter out a basic blacklist. > | xargs grep -l Components | xargs grep -L enablePrivilege Select only files that have Components and _not_ enablePrivilege (since we handle enablePrivilege tests separately) > | perl -pe 's#.*mochitest/tests/##' Make the prefix relative to topsrcdir > | xargs perl -p -i.bakkk -e 's/Components\.interfaces(\s|;|\.|\[)/SpecialPowers\.Ci$1/g, s/SpecialPowers\.wrap\(Components\)\.(.)(lasses|tils|nterfaces|esults)/SpecialPowers.C$1/g, s/(?<![\.a-zA-Z])Components/SpecialPowers\.Components/g, s/window\.Components/window\.SpecialPowers\.Components/g' Do the following replacements, in order: 1 - Replace Components.interfaces with SpecialPowers.Ci 2 - Replace SpecialPowers.wrap(Components).foo with the appropriate SpecialPowers.Cf in tests with existing SpecialPowers wrapping 3 - Replace Components with SpecialPowers.Components as a final catch-all

Alex: could you take a look at this if it requires changes on jetpack side? So for example if from content-script or some other none chrome sandbox we need Components, we need to 'inject' it manually first with getComponentsForScope.

Bobby, would you have a try build with Components being removed? I'd like to run jetpack tests on it. (Would be cool if there is a Windows build, but I can handle a linux one too)

Comment on attachment 662836 [details] [diff] [review] Fix up tests to avoid relying on the existence of window.Components (MANUAL). v1 Review of attachment 662836 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/mochitest/browserElement_Auth.js @@ +80,5 @@ > } > > function testFinish() { > // Clear login information stored in password manager. > + var authMgr = SpecialPowers.wrap(Components).classes['@mozilla.org/network/http-auth-manager;1'] In some places, you turn this into SpecialPowers.wrap(Components) and other places you turn this into SpecialPowers.wrap(SpecialPowers.Components). Okay, it looks like that is just going to be fixed up in the later test fixing patch. ::: dom/tests/mochitest/bugs/test_bug317448.html @@ +29,5 @@ > QueryInterface(Components.interfaces.nsIClassInfo). > getInterfaces(count). > map(function(id) { > id = SpecialPowers.wrap(id); > + return SpecialPowers.wrap(SpecialPowers.Components).interfacesByID[id].toString(); Do you really need to SpecialPowers.wrap SpecialPowers.Components? I guess it is unprivileged. ::: js/xpconnect/tests/mochitest/test_lookupMethod.html @@ +20,5 @@ > /** Test for Components.lookupMethod in content scope. **/ > SimpleTest.waitForExplicitFinish(); > > +// We're testing things from an unprivileged perspective, so don't use > +// SpecialPowers.Components and friends. This comment seems weird. You say not to use SpecialPowers.Components, but then you use it? Maybe SpecialPowers.Components is actually unprivileged? That would explain why you had to wrap it elsewhere, I suppose. Can you just define a lookupMethod that is the right kind of privilege, and call it unprivLookupMethod or something, then call it here? As that's the only thing from SC you are using. Would at least one of these tests fail if you accidentally were calling the privileged version?

Comment on attachment 662838 [details] [diff] [review] Bug 792036 - Automated fixups. v1 Review of attachment 662838 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/navigation/NavigationUtils.js @@ +99,5 @@ > > function xpcEnumerateContentWindows(callback) { > > + var Ci = SpecialPowers.Ci; > + var ww = SpecialPowers.wrap(SpecialPowers.Components) This could be changed to use SpecialPowers.Cc. ::: dom/tests/mochitest/geolocation/test_mozsettings.html @@ +41,3 @@ > SpecialPowers.setBoolPref("dom.mozSettings.enabled", true); > SpecialPowers.addPermission("settings", true, document); > comp.utils.import("resource://gre/modules/SettingsChangeNotifier.jsm"); You could just delete the |var comp| above and replace "comp.utils" with "SpecialPowers.Cu", as it is the only use. ::: dom/tests/mochitest/geolocation/test_mozsettingsWatch.html @@ +41,3 @@ > SpecialPowers.setBoolPref("dom.mozSettings.enabled", true); > SpecialPowers.addPermission("settings", true, document); > comp.utils.import("resource://gre/modules/SettingsChangeNotifier.jsm"); likewise ::: toolkit/components/passwordmgr/test/test_prompt_async.html @@ +79,5 @@ > logins.push(login); > } > > //need to allow for arbitrary network servers defined in PAC instead of a hardcoded moz-proxy. > + var ios = SpecialPowers.wrap(SpecialPowers.Components) Could be SpecialPowers.Cc @@ +86,2 @@ > > + var pps = SpecialPowers.wrap(SpecialPowers.Components) SpecialPowers.Cc

(In reply to Andrew McCreight [:mccr8] from comment #18) > In some places, you turn this into SpecialPowers.wrap(Components) and other > places you turn this into SpecialPowers.wrap(SpecialPowers.Components). > Okay, it looks like that is just going to be fixed up in the later test > fixing patch. Yeah, sorry for the inconsistency / sloppiness. :-( > > + return SpecialPowers.wrap(SpecialPowers.Components).interfacesByID[id].toString(); > > Do you really need to SpecialPowers.wrap SpecialPowers.Components? I guess > it is unprivileged. Yeah, the idea is that SpecialPowers.Components is just the regular old object from before (which we need, in some cases). The pre-wrapped versions come in the SpecialPowers.Cx variants, but there are a couple that are seldom-enough used that I didn't bother making an API for them (like interfacesByID). Hence the manual wrapping here. > ::: js/xpconnect/tests/mochitest/test_lookupMethod.html > @@ +20,5 @@ > > /** Test for Components.lookupMethod in content scope. **/ > > SimpleTest.waitForExplicitFinish(); > > > > +// We're testing things from an unprivileged perspective, so don't use > > +// SpecialPowers.Components and friends. > > This comment seems weird. Because it's totally wrong. :\ What it should say is "Don't use SpecialPowers.Cu and friends", because those are the pre-wrapped versions. I'll fix the comment and flesh it out a little bit. Sorry for the confusion!

(In reply to Andrew McCreight [:mccr8] from comment #19) > Comment on attachment 662838 [details] [diff] [review] I'll address these review comments in a patch on top of the automated one to preserve its purity. ;-)

Looks green: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0be04eee5f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/65e5f5c48ffc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1460d44ec8de remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc84f6a237d7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f7731dd39c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8583bee3c37 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7c4fae49c0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/633082d3e546

https://hg.mozilla.org/mozilla-central/rev/9f0be04eee5f https://hg.mozilla.org/mozilla-central/rev/65e5f5c48ffc https://hg.mozilla.org/mozilla-central/rev/1460d44ec8de https://hg.mozilla.org/mozilla-central/rev/dc84f6a237d7 https://hg.mozilla.org/mozilla-central/rev/d3f7731dd39c https://hg.mozilla.org/mozilla-central/rev/a8583bee3c37 https://hg.mozilla.org/mozilla-central/rev/1f7c4fae49c0 https://hg.mozilla.org/mozilla-central/rev/633082d3e546

Ugh. I'm trying to do something like: ("@mozilla.org/suite/shell-service;1" in SpecialPowers.Components.classes) ? And getting: Error: TypeError: invalid 'in' operand SpecialPowers.Components.classes

> Error: TypeError: invalid 'in' operand SpecialPowers.Components.classes Fixed by using SpecialPowers.Cc