Closed
Bug 1038844
Opened 10 years ago
Closed 10 years ago
Stop using __exposedProps__ in SpecialPowers
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(3 files)
10.12 KB,
patch
|
gkrizsanits
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
7.47 KB,
patch
|
ted
:
review+
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8457611 -
Flags: superreview?(mrbkap)
Attachment #8457611 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8457612 -
Flags: review?(jgriffin)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8457613 -
Flags: review?(jgriffin)
Assignee | ||
Updated•10 years ago
|
Attachment #8457613 -
Flags: review?(mrbkap)
Assignee | ||
Comment 6•10 years ago
|
||
(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•10 years ago
|
||
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 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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•10 years ago
|
Attachment #8457611 -
Flags: superreview?(mrbkap) → superreview+
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 16•10 years ago
|
||
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?
Assignee | ||
Comment 17•10 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•