Ted and I brainstormed this idea, and I got a mostly-working prototype going over the weekend. The basic idea is that we do SpecialPowers.wrap(obj), after which everything else is transparent thanks to a transitively-maintained wrapper layer. There are still a few bugs to be fixed, then I'll post the patch.

So after several weeks of fighting various platform bugs, I've finally got something that seems to work well (once the dependent patches have been applied). Huzzah! \o/ Flagging mrbkap for review on the wrapper itself, and ted for review on the API. As a proof-of-concept, I'm also attaching a patch that uses SpecialPowers.wrap() for 3 content tests that would be a big PITA to fix any other way.

While we're waiting for review, john is going to start playing with this. For reference, here's what I suggested over IRC: 1 - grab the SpecialPowers.wrap patch and play with it (the dependent platform bugs have all landed, so no need to apply anything else) 2 - if it doesn't work like you expect, ping me, since it's probably a bug 3 - take a look at the specialpowers stuff in the mochitest harness (the code that binds DOMWindowUtils is a good analogous example), and see if you can replace Components with SpecialPowers.wrap(Components) in all mochitest windows 4 - Run the tests, and see if they all pass. If they don't, see if you can figure out why 5 - If they all pass, try removing all the calls to enablePrivilege in our mochitests, and see which tests still pass and which ones fail. This would ideally be done with a script. Theoretically, the ones that fail are the ones that were using enablePrivilege for something other than Components.foo access, and require manual insertion of SpecialPowers.wrap. You don't need to fix all of them, but it would be good to know how many of them there are.

Blake, Ted - Any timeline for reviews here? I'd really like to be able to move forward with this.

Sorry, I've been tied up in some other work. I'll try to give it a first pass this week.

(In reply to Ted Mielczarek [:ted, :luser] from comment #5) > Sorry, I've been tied up in some other work. I'll try to give it a first > pass this week. Ok sounds good. I'm mostly looking for an API review from you (since blake is going to review the proxy stuff), so it should just be a 5 minute thing.

Comment on attachment 578935 [details] [diff] [review] Implement Proxy-Based XPCOM Wrappers in SpecialPowers. v1 Review of attachment 578935 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good. There are a couple of things that could potentially be confused by poorly-written test code (e.g. isWrapper can be tricked into thinking that a random object is one of its wrappers) but given the context here, I don't think it's a problem. One thing that we are going to have to document, however, is that it doesn't look like the wrapper preserves identity. This should be fine as long as tests know to unwrap these things before testing for identity. That was the only real issue that I saw, though. ::: testing/mochitest/tests/SimpleTest/specialpowersAPI.js @@ +179,5 @@ > + // If the object is callable, make a function proxy. > + if (typeof obj === "function") { > + > + var callTrap = function() { > + What's with the crazy extra newlines here? @@ +390,5 @@ > +}; > + > +SpecialPowersHandler.prototype.fix = function() { return undefined; /* Throws a TypeError. */ }; > + > +// Per the ES5 spec this is a derived trip, but it's fundamental in spidermonkey trap

> One thing that we are going to have to document, however, is > that it doesn't look like the wrapper preserves identity. Good thought! This is easy to do with WeakMaps, so I figured I'd go for it. Flagging for review.

ted, can you ACK the SpecialPowers.{wrap,unwrap}() API here? Should only take a minute...

Comment on attachment 588529 [details] [diff] [review] Preserve identity with a WeakMap cache. v1 Is this worth doing now in light of bug 673468?

(In reply to Blake Kaplan (:mrbkap) from comment #10) > Is this worth doing now in light of bug 673468? Hm. It would certainly be nice, since the next step is going to be to try exposing |Components| as |SpecialPowers.wrap(Components)| in mochitest code and see how many instances of enablePrivilege we can just rip out. So it would be nice for the wrapper to be as transparent as possible. I'm not sure how many tests require identity preservation on instantiated components though, so it might not be a big deal. mccr8, is bug 673468 easily fixed? It would probably make weak maps much more useful for chrome code.

I don't really know enough about wrapped JS objects to make that judgement. You basically need some way to disable the wrapper disposal optimization. Then during a GC we iterate over all of the keys in all weak maps and disable the optimization somehow for any keys we see. For native things, we do this using wrapper preservation. Maybe for JS it would work just to mark these wrappers, but that may make things leaky.

(In reply to Andrew McCreight [:mccr8] from comment #12) > I don't really know enough about wrapped JS objects to make that judgement. > You basically need some way to disable the wrapper disposal optimization. > Then during a GC we iterate over all of the keys in all weak maps and > disable the optimization somehow for any keys we see. For native things, we > do this using wrapper preservation. Maybe for JS it would work just to mark > these wrappers, but that may make things leaky. Hrm, ok. I guess it's probably best to land without identity preservation for now, then. I've filed bug 718543 to keep track of it.

Comment on attachment 578935 [details] [diff] [review] Implement Proxy-Based XPCOM Wrappers in SpecialPowers. v1 Review of attachment 578935 [details] [diff] [review]: ----------------------------------------------------------------- Okay, the API looks fine. Sorry I didn't get around to this earlier. But man, that wrapper code is inscrutable. ::: testing/mochitest/tests/SimpleTest/specialpowersAPI.js @@ +402,4 @@ > SpecialPowersAPI.prototype = { > > + wrap: wrapPrivileged, > + unwrap: unwrapPrivileged, Can you stick some comments here about what these do?

For listToArray, can't we use Array.prototype.slice()?

Comment on attachment 578936 [details] [diff] [review] Bug 702353 - Proof-of-concept on some pretty involved tests. v1 Review of attachment 578936 [details] [diff] [review]: ----------------------------------------------------------------- I'm not wild about how this plays out in practice, but I don't think we have the resources to really fix all these tests without a global solution like this, so I think we should just do it. We can pretty easily fix all the remaining tests with this, and then drop enablePrivilege. ::: content/base/test/test_bug166235.html @@ +54,1 @@ > transferable.getTransferData(mime, data, {}) ; Things like this are going to be confusing corner-cases, I suspect. You have to pass a wrapper in here so you can touch the data when it returns, right?

(In reply to Ms2ger from comment #15) > For listToArray, can't we use Array.prototype.slice()? We sure can! Thanks for the tip.

(In reply to Ted Mielczarek [:ted, :luser] from comment #16) > Comment on attachment 578936 [details] [diff] [review] > Bug 702353 - Proof-of-concept on some pretty involved tests. v1 > > Review of attachment 578936 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not wild about how this plays out in practice To be fair, I chose the most involved tests I could find for this patch. These were the ones where, back in November when I was trying to convert tests manually, I punted entirely. Some of them could indeed be converted to chrome tests, but it would require a lot of effort to grok the test more deeply. Like you said, resources. > Things like this are going to be confusing corner-cases, I suspect. You have > to pass a wrapper in here so you can touch the data when it returns, right? Not really, actually. I just inserted the wrap() call here because the test later does |data.value.QueryInterface|. We have no way to transitively wrap out-params (and I'm not even sure that we'd want to), so we have to handle the new object somewhere.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ca62671c9323

Looks green. Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/679b3a8d94c1 http://hg.mozilla.org/integration/mozilla-inbound/rev/a904454ebfbb

https://hg.mozilla.org/mozilla-central/rev/679b3a8d94c1 https://hg.mozilla.org/mozilla-central/rev/a904454ebfbb