So here's the backstory. We want to rip out enablePrivilege for a number of reasons. First, it's dangerous, since it's actually exposed to the web - so any exploit there is an automatic sg:crit. Second, it complicates our security code, since after every failed security check we have to say "but wait, is UniversalXPConnect enabled?". Third, the whole notion of content doing privileged stuff and waltzing around in chrome code stops making sense with e10s. But we've got ~800 uses of it in our mochitests, which poses a major road block. So we need another story for those. Thus, SpecialPowers was born. SpecialPowers runs with system principal and is attached to content objects. It was designed to be a catch-all API, so anytime you wanted to do something privileged in a content Mochitest, you check whether SpecialPowers had an API for what you wanted to do. If it didn't, you added it. Sounds simple enough. But it turned out to be really tedious, and didn't get a whole lot of traction. A major issue was that it was never quite clear what was e10s-safe and what wasn't, because e10s wasn't ready yet. Moreover, manually converting tests, even when SpecialPowers had the API you needed, was really time-consuming. And when the API wasn't there...you get the picture. But then the winds shifted, some mail was sent to dev-planning, and e10s-for-Desktop got shifted to the back burner. So the need for this stuff to work across processes went away (at least for the near term). But the need to kill enablePrivilege was still there. So, being an XPConnect-er, I decided the solve the problem with a layer of wrappers. ;-) The basic idea is that you call SpecialPowers.wrap(somePrivilegedObject), which proxies all your operations (getting/setting properties, calling methods, etc) into privileged code where they Just Work. Moreover, these wrappers transitively wrap their return values. So you just need need to call wrap() once. For example, SpecialPowers.wrap(somePrivilegedObject).foo.bar.baz() works, even if foo, bar, and baz are also privileged. So this is a huge step forward, because it makes it a piece of cake to convert individual tests. Just call SpecialPowers.wrap in one or two places, and the test starts passing. Here's the API: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/specialpowersAPI.js#356 And here are some examples of it in action: http://mxr.mozilla.org/mozilla-central/search?string=SpecialPowers.wrap By far the most common use of enablePrivilege in mochitests today (~90% at least) is to access something off of the |Components| object. For example, people want to createInstance, getService, etc. Since we've got the transitive behavior, we can exploit this choke point. Most tests right now can be converted by just replacing |Components| with |SpecialPowers.wrap(Components)|. But we can do even better than that. First, manually converting 800 tests sucks. Second, we're actually planning on removing the Components object from content scope entirely. So relatively soon, the plan is stop defining |Components| on content globals, and simultaneously configure the mochitest harness to inject SpecialPowers.wrap(Components) as |Components| into every content scope. So Components is replaced with a wrapper that automagically works. Once we do that, we would like to be able to just write a script to find/remove every call to enablePrivilege in the test suite. But unfortunately, there's a handful of other issues that the wrapped-Components injection can't solve. These are random things like accessing file input values and so on. We can still probably use the SpecialPowers wrappers there, but we have to apply them manually, since they have nothing to do with Components. So this bug is about fixing those other tests ahead of time, so that the switch goes smoothly. Here's how you do it: 1 - Grab jdm's patch over from bug 724299. In that patch, he added some hacks that do the wrapped-Components injection manually (not the way we'll eventually do it, but good enough for now). 2 - Get the tests passing with that. Tryserver is your friend here, use it copiously. The last activity on bug 724299 was that jdm was stuck with instanceof. But Camilo has a patch over in bug 726053 (reviewed, but never landed) that hopefully fixes that. So apply that work, test, fix, repeat. See what comes up, and don't hesitate to ping me about the various issues that arise. 3 - Once the tests are all passing with Components injection, that means we're doing the injection right, and that's it's going to work when we do it for real. So the next (and most important) step is to find/remove all instances of enablePrivilege from the test suite, re-run, and see what breaks. 4 - Once we have an idea of what breaks for #3, we need to fix these issues manually, since those won't be covered by our automated approach. So this bug is basically about getting through step 4. Getting there would be a major milestone, and open the door for finally ripping out that damn enablePrivilege API. Just thinking about it makes me giddy. Gijs said he might be interested in working on this.

Sticking this from mail into the bug for posterity: >> 2) Got my first errors very very early from some of the tests sanity checks of the API, where stuff like: >> >> SpecialPowers.wrap(Components).ID('{00000000-0000-0000-0000-000000000000}'); >> >> fails now because it wraps twice (as Components is already wrapped, and attempting double-wrapping throws). >> >> Should I just look for instances of that, fix, attach a patch, and keep going? >> Seems there are quite some already: >> http://mxr.mozilla.org/mozilla-central/search?string=SpecialPowers.wrap%28Components%29 >> >> Not sure if we don't want a clever-er solution (like not throwing if we pass an object which is already wrapped). > I think we should do the clever-er solution. Let's just change SpecialPowers.wrap > to function(obj) { return isWrapper(obj) ? obj : wrapPrivileged(obj); }. I think > the internal one should still throw (so we catch bugs), but there's no reason to > have the public API have such a silly restriction. Will add a patch for that, hopefully today.

(In reply to Gijs Kruitbosch from comment #1) > Will add a patch for that, hopefully today. \o/

OK, so that'd be this... solves some problems, but still two more issues in the test safety checks: 66 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_sanityEventUtils.html | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass at http://mochi.test:8888/tests/Harness_sanity/test_sanityEventUtils.html:152 97 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_sanityException2.html | expectUncaughtException was called but no uncaught exception was detected! First can be fixed by just wrapping the rv of synthesizeQuerySelectedText (after which removing the enablePrivilege call doesn't affect the test, yay!), need to look at the other in more detail. Probably for the former the best fix is to make EventUtils aware of specialpowers, will add a proper patch for that later...

We should probably just make SpecialPowers.getDOMWindowUtils return a wrapped object. That would solve the problem here, and probably a number of other problems: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#700 I was going to experiment with that over in bug 553102 as well.

(In reply to Bobby Holley (:bholley) from comment #4) > We should probably just make SpecialPowers.getDOMWindowUtils return a > wrapped object. That would solve the problem here, and probably a number of > other problems: > > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/ > SimpleTest/EventUtils.js#700 > > I was going to experiment with that over in bug 553102 as well. Mm, that's easier. Note to self/others: see also bug 649124, bug 643803. Don't yet 'get' why there's both a DOMWindowUtils prop and a bindDOMWindowUtils method (which is... interesting), but won't have time to look at that until I finish work for today.

A lot of the goofiness in SpecialPowers is due to trying to harmonize things across mochitest/mochitest-chrome/mochitest-browser-chrome.

(In reply to Gijs Kruitbosch from comment #5) > (In reply to Bobby Holley (:bholley) from comment #4) > > We should probably just make SpecialPowers.getDOMWindowUtils return a > > wrapped object. That would solve the problem here, and probably a number of > > other problems: > > > > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/ > > SimpleTest/EventUtils.js#700 > > > > I was going to experiment with that over in bug 553102 as well. > > Mm, that's easier. Note to self/others: see also bug 649124, bug 643803. > Don't yet 'get' why there's both a DOMWindowUtils prop and a > bindDOMWindowUtils method (which is... interesting), but won't have time to > look at that until I finish work for today. (In reply to Ted Mielczarek [:ted] from comment #6) > A lot of the goofiness in SpecialPowers is due to trying to harmonize things > across mochitest/mochitest-chrome/mochitest-browser-chrome. Yes, but AFAICT bug 676274 broke something here, because this.DOMWindowUtils used to be initialized by a call to bindDOMWindowUtils, and now it is, AFAICT, never set. Which means some of the methods that use it without nullchecking/fallback to bindDOMWindowUtils (I spotted gc(), there may be others) would be broken now. But I'm checking veeeery casually right now, as said, will have more time after 5.30pm today.

(In reply to Gijs Kruitbosch from comment #7) This was bogus, forget I said that. (In reply to Bobby Holley (:bholley) from comment #4) > We should probably just make SpecialPowers.getDOMWindowUtils return a > wrapped object. That would solve the problem here, and probably a number of > other problems: > > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/ > SimpleTest/EventUtils.js#700 > > I was going to experiment with that over in bug 553102 as well. In the meantime, is this patch what you meant? And, getting ahead of myself slightly, who should I eventually request review from for these?

(In reply to Gijs Kruitbosch from comment #8) > In the meantime, is this patch what you meant? Yep! > And, getting ahead of myself slightly, who should I eventually request > review from for these? ted, probably.

(In reply to Bobby Holley (:bholley) from comment #9) > (In reply to Gijs Kruitbosch from comment #8) > > In the meantime, is this patch what you meant? > > Yep! > > > And, getting ahead of myself slightly, who should I eventually request > > review from for these? > > ted, probably. OK, great. The next few things I'm seeing: 92 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_sanityException2.html | expectUncaughtException was called but no uncaught exception was detected! 1113 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_bug364677.html | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access property 'documentElement' at http://mochi.test:8888/tests/browser/base/content/test/test_bug364677.html:27 1119 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | Test timed out. TBQH, really unsure what's going on with the first one. I tried switching to SpecialPowers.executeSoon to fire the callback functions, that didn't make any difference. The other two seem like they should be easy to fix, I'll look at doing that tomorrow. I haven't looked much further yet, I'm sure there'll be more structural things that I run into once I get to the rest of things (and I should probably spin some try builds as they'll be faster than running the tests on my mbp).

(In reply to Gijs Kruitbosch (traveling until 29/05) from comment #10) > (In reply to Bobby Holley (:bholley) from comment #9) > > (In reply to Gijs Kruitbosch from comment #8) > > > In the meantime, is this patch what you meant? > > > > Yep! > > > > > And, getting ahead of myself slightly, who should I eventually request > > > review from for these? > > > > ted, probably. > > OK, great. The next few things I'm seeing: > > 92 ERROR TEST-UNEXPECTED-FAIL | > /tests/Harness_sanity/test_sanityException2.html | expectUncaughtException > was called but no uncaught exception was detected! > 1113 ERROR TEST-UNEXPECTED-FAIL | > /tests/browser/base/content/test/test_bug364677.html | an unexpected > uncaught JS exception reported through window.onerror - Error: Permission > denied to access property 'documentElement' at > http://mochi.test:8888/tests/browser/base/content/test/test_bug364677.html:27 > 1119 ERROR TEST-UNEXPECTED-FAIL | > /tests/browser/base/content/test/test_contextmenu.html | Test timed out. > > TBQH, really unsure what's going on with the first one. I tried switching to > SpecialPowers.executeSoon to fire the callback functions, that didn't make > any difference. The other two seem like they should be easy to fix, I'll > look at doing that tomorrow. I haven't looked much further yet, I'm sure > there'll be more structural things that I run into once I get to the rest of > things (and I should probably spin some try builds as they'll be faster than > running the tests on my mbp). In some spare moments in NYC, looking at this some more. The documentElement case in test_bug364677 is indeed easy, the contextmenu one turns out to be harder. The trivial initial fix was wrapping the subwindow variable on assignment (the window.open call at the bottom). But that runs afoul of an instanceof check in menulist.xml, because the window it sees there is a wrapper rather than the real window (haven't checked exactly how). As that's real rather than test code, I presume I shouldn't change that... So I tried fixing the individual cases (the QI to get a chrome window, the uses of document/inputElement.controllers). That mostly worked, but I got stuck on the OS checks on lines 702 and 718. They use the 'Services' API, which was Cu.import'd at the top. That seems to not work: JavaScript error: http://mochi.test:8888/tests//browser/base/content/test/test_contextmenu.html, line 702: ReferenceError: Services is not defined I don't really understand *why* that wouldn't work, as Components should be wrapped, and therefore I figured the Cu.import calls would work. I actually think here we don't need Services.appinfo - we can just use navigator.platform for which we don't need privs. But in case I run into this again, any idea why the import call isn't "working"? (I couldn't find any errors related to it, although I may have missed something...) PS: I will be in Toronto Wed-Fri, and plan on spending at least half a day in the MoCo office there, assuming they let me in. Hopefully more stuff will get done then. :-)

Egh, did not mean to remove those deps/blockers - wonder what happened there.

(In reply to Gijs Kruitbosch (traveling until 29/05) from comment #11) > But that runs afoul of an > instanceof check in menulist.xml, because the window it sees there is a > wrapper rather than the real window (haven't checked exactly how). As that's > real rather than test code, I presume I shouldn't change that... I haven't looked at the code, but is there any way to SpecialPowers.unwrap() that window before it gets there? > So I tried fixing the individual cases (the QI to get a chrome window, the > uses of document/inputElement.controllers). That mostly worked, but I got > stuck on the OS checks on lines 702 and 718. They use the 'Services' API, > which was Cu.import'd at the top. That seems to not work: > > JavaScript error: > http://mochi.test:8888/tests//browser/base/content/test/test_contextmenu. > html, line 702: ReferenceError: Services is not defined > > I don't really understand *why* that wouldn't work, as Components should be > wrapped, and therefore I figured the Cu.import calls would work. Ah! So Cu.import imports stuff from JSMs onto the global of the calling object. But in this case, since the special powers wrapping code is essentially proxying this kind of access into a scope with system principal, I'll be that Services is getting defined on that scope instead. The solution here is probably to explicitly pass |window| as the optional scope argument: https://developer.mozilla.org/en/Components.utils.import That should work. If it doesn't, there's always the nuclear option of converting those tests to chrome tests. Cu.import _really_ was never designed to be used by content. > PS: I will be in Toronto Wed-Fri, and plan on spending at least half a day > in the MoCo office there, assuming they let me in. Hopefully more stuff will > get done then. :-) Sounds good. This actually just became really high priority, because it kind of blocks IonMonkey. enablePrivilege uses stack frame annotations, and those are going away when IonMonkey lands on June 15th. I'm going to talk to jst tomorrow and see what he thinks, but in general I think we should push hard on this, make sure you have the resources you need, and possibly throw some other folks on board to help out. Please keep posting your progress and the patches you generate like you've been doing. :-) Have you done a try push to get a sense of the number of total failures we're looking at here? It would be helpful to have a sense of that. I know we have ~800 uses of enablePrivilege, but my guess that 90% of them would be fixed automagically by SpecialPowers.wrap(Components) injection was really just that - a guess. I'm assuming you have try access, but if you don't let me know and I'll make sure you get it pronto.

(In reply to Bobby Holley (:bholley) from comment #13) > (In reply to Gijs Kruitbosch (traveling until 29/05) from comment #11) > > > But that runs afoul of an > > instanceof check in menulist.xml, because the window it sees there is a > > wrapper rather than the real window (haven't checked exactly how). As that's > > real rather than test code, I presume I shouldn't change that... > > I haven't looked at the code, but is there any way to SpecialPowers.unwrap() > that window before it gets there? I'll check if that's possible... in this case the fix (in patch above) is OK, I guess? > > JavaScript error: > > http://mochi.test:8888/tests//browser/base/content/test/test_contextmenu. > > html, line 702: ReferenceError: Services is not defined > The solution here is probably to explicitly pass |window| as the optional > scope argument: https://developer.mozilla.org/en/Components.utils.import > > That should work. If it doesn't, there's always the nuclear option of > converting those tests to chrome tests. Cu.import _really_ was never > designed to be used by content. Ah, fair point. I'll check that if I run into this more. > > PS: I will be in Toronto Wed-Fri, and plan on spending at least half a day > > in the MoCo office there, assuming they let me in. Hopefully more stuff will > > get done then. :-) > > Sounds good. This actually just became really high priority, because it kind > of blocks IonMonkey. enablePrivilege uses stack frame annotations, and those > are going away when IonMonkey lands on June 15th. I'm going to talk to jst > tomorrow and see what he thinks, but in general I think we should push hard > on this, make sure you have the resources you need, and possibly throw some > other folks on board to help out. Please keep posting your progress and the > patches you generate like you've been doing. :-) Right. If I'm too slow, feel free to have someone else take this, or split it up or whatever, that makes total sense. In the meantime, I'll do my best. :-) > > Have you done a try push to get a sense of the number of total failures > we're looking at here? It would be helpful to have a sense of that. I know > we have ~800 uses of enablePrivilege, but my guess that 90% of them would be > fixed automagically by SpecialPowers.wrap(Components) injection was really > just that - a guess. I'm assuming you have try access, but if you don't let > me know and I'll make sure you get it pronto. Eh, I thought I posted this yesterday, but clearly I did not: https://tbpl.mozilla.org/?tree=Try&rev=2408e33672c4 . Quick count says 229 + 35 + 12 + ??? (mochitest-4 seems to have crashed everywhere, after 30-odd failures) + 14 =~ 320 failures? Actually what's most surprising to me is that mochitest-other lists 900-odd failures, and some crashes. Looks like the mochitest-chrome stuff is being affected by the auto-wrapping Components? Isn't that not supposed to happen?

(In reply to Gijs Kruitbosch (traveling until 29/05) from comment #15) > I'll check if that's possible... in this case the fix (in patch above) is > OK, I guess? Clever. > Right. If I'm too slow, feel free to have someone else take this, or split > it up or whatever, that makes total sense. In the meantime, I'll do my best. > :-) Sounds good. I'll brainstorm some options with jst when I meet with him in a few hours. > Eh, I thought I posted this yesterday, but clearly I did not: > https://tbpl.mozilla.org/?tree=Try&rev=2408e33672c4 . > > Quick count says 229 + 35 + 12 + ??? (mochitest-4 seems to have crashed > everywhere, after 30-odd failures) + 14 =~ 320 failures? Yeah, looks like the results aren't totally useful though - they're skewed in both directions. The first failure count is inflated by one test failing hundreds of times in the same way. And the rest of the numbers may be artificially low, because some tests time out when they break, and if there are too many timeouts (4?) mochitest just gives up and quits. > > Actually what's most surprising to me is that mochitest-other lists 900-odd > failures, and some crashes. Looks like the mochitest-chrome stuff is being > affected by the auto-wrapping Components? Isn't that not supposed to happen? Yeah I wouldn't worry abut it. I think we put SpecialPowers into chrome tests for various utility functions, and so the injection is probably happening there as well. Shouldn't be affected when we do this for real.

(In reply to Bobby Holley (:bholley) from comment #16) > (In reply to Gijs Kruitbosch (traveling until 29/05) from comment #15) > > Quick count says 229 + 35 + 12 + ??? (mochitest-4 seems to have crashed > > everywhere, after 30-odd failures) + 14 =~ 320 failures? > > Yeah, looks like the results aren't totally useful though - they're skewed > in both directions. The first failure count is inflated by one test failing > hundreds of times in the same way. And the rest of the numbers may be > artificially low, because some tests time out when they break, and if there > are too many timeouts (4?) mochitest just gives up and quits. > Right, in MoTo office, trying to poke at mochitest-2 (which seemed like fewer failures, hence easier to fix to see some green?), but not having much luck. It seems that the navigationUtils.js code is invoking specialpowers wrapping code when accessing Components.classes from a setTimeout callback, at which point accessing the 'real' Components.classes fails (Permission denied). I haven't managed to figure out *why* that fails though. To see the failures, comment out the try/catch in poll(). I'll try to go back to mochitest-1 and fix some of the other tests that give so many failures, see if I can get somewhere with those.

Background: it seems that when invoked from a timeout/setinterval, somehow the wrappers aren't quite working correctly (permission denied errors trying to access properties on the wrapped objects). Bobby: you asked for a reduced testcase. I'm guessing this isn't quite reduced enough, but it's the best I could come up with short-term. Basically, if you apply this and add getComponents to the specialPowersAPI: getComponents: function() { return this.wrap(Components); }, you'll see the issue on a clean trunk build. Still not sure what is causing this... will work on other issues with this patch in the meantime.

Comment on attachment 624004 [details] [diff] [review] Part 1: double-wrapping objects should be allowed Fixed already.

Comment on attachment 627438 [details] Sanity check on wrappers (currently fails) The setTimeout issue is now bug 762790.

Sadly, not working on this at the moment...

I think bug 1149966 (and 871323) will cover this.