792036 - Prepare test suite for disappearance of |Components| in content scopes

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Refactor AttachComponentsObject API to allow an explicit target. v1

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.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor out glue code to get the JS Components object for a scope. v1

This allows the API in the upcoming patch.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a SpecialPowers API to provide access to the Components object in various forms. v1

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.

Comment 6

[Andrew McCreight [:mccr8]]

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)

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Andrew McCreight [:mccr8]]

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

Comment 10

[Bobby Holley (:bholley)]

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 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Expose |Components| to content in the indexedDB test harness. v1

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Fix sandbox xpcshell tests. v1

I want gabor to look at these.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Fix up tests to avoid relying on the existence of window.Components (MANUAL). v1

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.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Bug 792036 - Automated fixups. v1

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

Comment 16

[Gabor Krizsanits (INACTIVE)]

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.

Comment 17

[Alexandre Poirot [:ochameau]]

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 18

[Andrew McCreight [:mccr8]]

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 19

[Andrew McCreight [:mccr8]]

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

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=84c844150e41

Comment 23

[Bobby Holley (:bholley)]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Philip Chee]

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

Comment 26

[Philip Chee]

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