If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Stop using __exposedProps__ in SpecialPowers

RESOLVED FIXED in mozilla33

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
https://tbpl.mozilla.org/?tree=Try&rev=1f4751dc5b6e
https://tbpl.mozilla.org/?tree=Try&rev=3efefc5892e3
Created attachment 8457611 [details] [diff] [review]
Part 1 - Implement a COW opt-out for automation. v1
Attachment #8457611 - Flags: superreview?(mrbkap)
Attachment #8457611 - Flags: review?(gkrizsanits)
Created attachment 8457612 [details] [diff] [review]
Part 2 - Flip the scary automation pref for crashtests and marionette. v1
Attachment #8457612 - Flags: review?(jgriffin)
Created attachment 8457613 [details] [diff] [review]
Part 3 - Use the opt-out for the SpecialPowers scope. v1
Attachment #8457613 - Flags: review?(jgriffin)
Attachment #8457613 - Flags: review?(mrbkap)
(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 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!
Attachment #8457612 - Flags: review?(jgriffin) → review+
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.
Attachment #8457613 - Flags: review?(jgriffin) → review?(ted)
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...
Attachment #8457611 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8457613 - Flags: review?(ted) → review+

Updated

3 years ago
Attachment #8457611 - Flags: superreview?(mrbkap) → superreview+
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?
Attachment #8457613 - Flags: review?(mrbkap) → review+
(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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/2afafea0f09e
https://hg.mozilla.org/mozilla-central/rev/a84002a7359c
https://hg.mozilla.org/mozilla-central/rev/c682a6b343ba
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
(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.

Updated

3 years ago
Depends on: 1051783
Blocks: 1121663
You need to log in before you can comment on or make changes to this bug.