Closed
Bug 792036
Opened 12 years ago
Closed 12 years ago
Prepare test suite for disappearance of |Components| in content scopes
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla18
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: dev-doc-needed)
Attachments
(7 files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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. ;-)
Assignee | ||
Comment 1•12 years ago
|
||
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).
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #662159 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•12 years ago
|
||
This allows the API in the upcoming patch.
Attachment #662160 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Attachment #662162 -
Flags: review?(ted.mielczarek)
Attachment #662162 -
Flags: review?(mrbkap)
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
(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. ;-)
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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.
Attachment #662162 -
Flags: review?(ted.mielczarek) → review+
Updated•12 years ago
|
Attachment #662159 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #662160 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #662162 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•12 years ago
|
||
This fixes up the indexedDB tests. Flagging bent for review.
Attachment #662833 -
Flags: review?
Assignee | ||
Comment 13•12 years ago
|
||
I want gabor to look at these.
Attachment #662834 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #662836 -
Flags: review?(continuation)
Assignee | ||
Comment 15•12 years ago
|
||
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
Attachment #662838 -
Flags: review?(continuation)
Assignee | ||
Updated•12 years ago
|
Attachment #662833 -
Flags: review? → review?(bent.mozilla)
Updated•12 years ago
|
Attachment #662834 -
Flags: review+
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #662833 -
Flags: review?(bent.mozilla) → review+
Comment 18•12 years ago
|
||
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?
Attachment #662836 -
Flags: review?(continuation) → review+
Comment 19•12 years ago
|
||
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
Attachment #662838 -
Flags: review?(continuation) → review+
Updated•12 years ago
|
Attachment #662834 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 20•12 years ago
|
||
(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!
Assignee | ||
Comment 21•12 years ago
|
||
(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. ;-)
Assignee | ||
Comment 23•12 years ago
|
||
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•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
![]() |
||
Comment 25•12 years ago
|
||
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•12 years ago
|
||
> Error: TypeError: invalid 'in' operand SpecialPowers.Components.classes
Fixed by using SpecialPowers.Cc
Updated•12 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•