1038844 - Stop using __exposedProps__ in SpecialPowers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Right now SpecialPowers (and its associated wrappers) are by far the most complex use case of COWs that I'm aware of. Given that we eventually want to turn off COWs entirely, we're going to need to sort something out here.

It would be nice to eat our own dogfood here and use the various export helpers (exportFuntion and such) that we've been writing for addons. I played around with this a bit today though, and I'm concerned that it would be a lot of engineering effort and may well slow down our test code with a lot of unnecessary back-and-forth and security checks.

So I think I'm just going to do something simple (allowing security wrappers to be turned off for a given scope), and put it behind the "security.turn_off_all_security_so_that_viruses_can_take_over_this_computer" pref. This will speed up our SpecialPowers wrapper, because we will no longer need to bounce through the second ExposedPropsWaiver proxy on every property access.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1f4751dc5b6e

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=3efefc5892e3

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement a COW opt-out for automation. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Flip the scary automation pref for crashtests and marionette. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use the opt-out for the SpecialPowers scope. v1

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #2)
> https://tbpl.mozilla.org/?tree=Try&rev=3efefc5892e3

Green on linux64. Doing an all-platform try push to be sure:

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

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8457612 [details] [diff] [review]
Part 2 - Flip the scary automation pref for crashtests and marionette. v1

Review of attachment 8457612 [details] [diff] [review]:
-----------------------------------------------------------------

Yikes, scary name!

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8457613 [details] [diff] [review]
Part 3 - Use the opt-out for the SpecialPowers scope. v1

Review of attachment 8457613 [details] [diff] [review]:
-----------------------------------------------------------------

Passing r? to Ted, who knows this better than I.

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8457611 [details] [diff] [review]
Part 1 - Implement a COW opt-out for automation. v1

Review of attachment 8457611 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +338,5 @@
> +    } else if (CompartmentPrivate::Get(origin)->forcePermissiveCOWs) {
> +        // Similarly, if this is a privileged scope that has opted to make itself
> +        // accessible to the world (allowed only during automation), unwrap should
> +        // be allowed.
> +        MOZ_ASSERT(!handler->hasSecurityPolicy());

Nit: I would personally merge the two branches, or if not would separate isChrome from the other two... as IsUniversalXPConnectEnabled and forcePermissiveCOWs both are I-know-what-I'm-doing-turn-off-security flags.

@@ +444,1 @@
>          wrapper = &CrossCompartmentWrapper::singleton;

Nit: I don't see the reason for two if branches here either...

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8457613 [details] [diff] [review]
Part 3 - Use the opt-out for the SpecialPowers scope. v1

Review of attachment 8457613 [details] [diff] [review]:
-----------------------------------------------------------------

You know that I pretty much just rs all of your wrapper-touching patches, right?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8457613 [details] [diff] [review]
Part 3 - Use the opt-out for the SpecialPowers scope. v1

Review of attachment 8457613 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/specialpowers/content/specialpowersAPI.js
@@ +493,5 @@
>  
>    /*
>     * Create blank privileged objects to use as out-params for privileged functions.
>     */
>    createBlankObject: function () {

Can this function go away now in favor of just using the object constructor?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> Can this function go away now in favor of just using the object constructor?

If I'm understanding you correctly, not without a reference to the SpecialPowers global - I don't believe that's exposed explicitly at the moment, though it could be.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> Comment on attachment 8457611 [details] [diff] [review]
> Part 1 - Implement a COW opt-out for automation. v1
> 
> Review of attachment 8457611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/wrappers/WrapperFactory.cpp
> @@ +338,5 @@
> > +    } else if (CompartmentPrivate::Get(origin)->forcePermissiveCOWs) {
> > +        // Similarly, if this is a privileged scope that has opted to make itself
> > +        // accessible to the world (allowed only during automation), unwrap should
> > +        // be allowed.
> > +        MOZ_ASSERT(!handler->hasSecurityPolicy());
> 
> Nit: I would personally merge the two branches, or if not would separate
> isChrome from the other two... as IsUniversalXPConnectEnabled and
> forcePermissiveCOWs both are I-know-what-I'm-doing-turn-off-security flags.

In my mind, they're really two separate cases (though they end up with the same wrapper) - one of them involves 'target' having extra privileges, whereas the other involves 'origin' being lax about security. Each case needed its own comment, so I felt that they should be separate.

Comment 14

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2afafea0f09e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a84002a7359c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c682a6b343ba

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2afafea0f09e
https://hg.mozilla.org/mozilla-central/rev/a84002a7359c
https://hg.mozilla.org/mozilla-central/rev/c682a6b343ba

Comment 16

[Martijn Wargers (dead)]

I was just looking for this __exposedProps__ in specialpowers.js and then I found it was removed in this bug! Does this mean, that content scripts can access underscored methods and properties now from the SpecialPowers object?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #16)
> I was just looking for this __exposedProps__ in specialpowers.js and then I
> found it was removed in this bug! Does this mean, that content scripts can
> access underscored methods and properties now from the SpecialPowers object?

Yes. If we're concerned about that, we should move those properties somewhere else.