Remove same-compartment Components wrapper

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

1.38 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.06 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
19.57 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.45 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.44 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.87 KB, patch
mrbkap
: review+
gkrizsanits
: feedback+
Details | Diff | Splinter Review
4.93 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
10.66 KB, patch
mrbkap
: review+
ted
: review+
Details | Diff | Splinter Review
1.83 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.90 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Along with bug 825392, this will let us kill same-compartment security wrappers.

This wrapper only applies (at present) in the XBL scope, which is pseudo-privileged already. So I think we can just do some simple stuff to prevent the creation of .classes and .utils and whatnot, and get rid of the security wrapper.

Doing so will let us rip out nsISecurityCheckedComponent, which will be awesome.
(Assignee)

Updated

5 years ago
Blocks: 958324
(Assignee)

Updated

5 years ago
Assignee: nobody → bobbyholley+bmo
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5ba063f385b7

Also, an experimental simplification of the Components object in automation:

https://tbpl.mozilla.org/?tree=Try&rev=a434439faf13
(Assignee)

Comment 5

5 years ago
(In reply to Bobby Holley (:bholley) from comment #4)
> https://tbpl.mozilla.org/?tree=Try&rev=903345a90058

This full try push looks good, modulo one Windows build issue. Doing another followup push for that:

https://tbpl.mozilla.org/?tree=Try&rev=f3887bac5ff4
(Assignee)

Comment 6

5 years ago
Uploading patches and flagging for review.
(Assignee)

Comment 7

5 years ago
Created attachment 8358452 [details] [diff] [review]
Part 1 - Make nsJSID.initialize [noscript]. v1

This lets us remove the usage of nsISecurityCheckedComponent. See the next patch.
Attachment #8358452 - Flags: review?(mrbkap)
(Assignee)

Comment 8

5 years ago
Created attachment 8358453 [details] [diff] [review]
Part 2 - Stop using nsISecurityCheckedComponent for nsJSID. v1
Attachment #8358453 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
Created attachment 8358454 [details] [diff] [review]
Part 3 - Remove most nsIXPCScriptable junk on nsXPCComponents. v1
Attachment #8358454 - Flags: review?(mrbkap)
(Assignee)

Comment 10

5 years ago
Created attachment 8358455 [details] [diff] [review]
Part 4 - Remove nsISecurityCheckedComponents stuff from Components. v1

This thing is only created in non-content scopes for XBL scopes, and during
automation (with Cu.getComponentsForScope).

At present, we currently have the same-compartment Components wrapper which
should do the right thing in those situations. Next, we'll focus on replacing
that.
Attachment #8358455 - Flags: review?(mrbkap)
(Assignee)

Comment 11

5 years ago
Created attachment 8358456 [details] [diff] [review]
Part 5 - Make AttachComponentsObject a non-static method on XPCWrappedNativeScope. v1
Attachment #8358456 - Flags: review?(mrbkap)
(Assignee)

Comment 12

5 years ago
Created attachment 8358458 [details] [diff] [review]
Part 6 - Get rid of manual nsIClassInfo and nsIXPCScriptable implementations for nsXPCComponents. v1

The macro-driven ClassInfo stuff doesn't do getClassDescription, so we need to
change that test.
Attachment #8358458 - Flags: review?(mrbkap)
(Assignee)

Comment 13

5 years ago
Created attachment 8358459 [details] [diff] [review]
Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1
Attachment #8358459 - Flags: review?(mrbkap)
(Assignee)

Comment 14

5 years ago
Created attachment 8358461 [details] [diff] [review]
Part 9 - Store nsXPCComponentsBase on XPCWrappedNativeScope, so that we can have either. v1
Attachment #8358461 - Flags: review?(mrbkap)
(Assignee)

Comment 15

5 years ago
Created attachment 8358462 [details] [diff] [review]
Part 10 - Add a way for automation to force the creation of a privileged Components object for an unprivileged scope. v1
Attachment #8358462 - Flags: review?(ted)
Attachment #8358462 - Flags: review?(mrbkap)
(Assignee)

Comment 16

5 years ago
Created attachment 8358463 [details] [diff] [review]
Part 11 - Use nsXPCComponentsBase for everything but system-principaled scopes. v1
Attachment #8358463 - Flags: review?(mrbkap)
(Assignee)

Comment 17

5 years ago
Created attachment 8358464 [details] [diff] [review]
Part 12 - Remove Components wrappers. v1

We fix up the tests here to test the new behavior, and fix some bugs in the test
while we're at it.
Attachment #8358464 - Flags: review?(mrbkap)
(Assignee)

Comment 18

5 years ago
Comment on attachment 8358459 [details] [diff] [review]
Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1

Flagging gabor for feedback here just so that he's in the loop of the new setup.
Attachment #8358459 - Flags: feedback?(gkrizsanits)
Comment on attachment 8358462 [details] [diff] [review]
Part 10 - Add a way for automation to force the creation of a privileged Components object for an unprivileged scope. v1

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

This is pretty much Greek to me, so consider this rs=me on the specialpowers changes.
Attachment #8358462 - Flags: review?(ted) → review+
Attachment #8358459 - Flags: feedback?(gkrizsanits) → feedback+
Attachment #8358452 - Flags: review?(mrbkap) → review+
Attachment #8358453 - Flags: review?(mrbkap) → review+
Comment on attachment 8358454 [details] [diff] [review]
Part 3 - Remove most nsIXPCScriptable junk on nsXPCComponents. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3776,5 @@
> +    XPCContext* xpcc = XPCContext::GetXPCContext(aCx);
> +    if (!xpcc)
> +        return NS_ERROR_FAILURE;
> +    nsresult res = xpcc->GetLastResult();
> +    *aOut = JS_NumberValue((double)(uint32_t) res);

I guess you're using what was there before, but any reason to not make this static_cast<double>(static_cast<uint32_t>(res))?
Attachment #8358454 - Flags: review?(mrbkap) → review+
Attachment #8358455 - Flags: review?(mrbkap) → review+
Attachment #8358456 - Flags: review?(mrbkap) → review+
Comment on attachment 8358458 [details] [diff] [review]
Part 6 - Get rid of manual nsIClassInfo and nsIXPCScriptable implementations for nsXPCComponents. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3606,5 @@
> +/**********************************************/
> +
> +class ComponentsSH : public nsIXPCScriptable {
> +public:
> +    ComponentsSH(unsigned dummy) {}

Please make this class conform with the recent decisions about brace placement.

@@ +3609,5 @@
> +public:
> +    ComponentsSH(unsigned dummy) {}
> +    NS_DECL_ISUPPORTS
> +    NS_DECL_NSIXPCSCRIPTABLE
> +    static ComponentsSH singleton;

For general C++ cleanliness, mind making the singleton object private?
Attachment #8358458 - Flags: review?(mrbkap) → review+
Comment on attachment 8358459 [details] [diff] [review]
Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3503,2 @@
>      NS_IF_RELEASE(mID);
>      NS_IF_RELEASE(mException);

Mind filing a bug on converting all of these to use smart pointers?
Attachment #8358459 - Flags: review?(mrbkap) → review+
Attachment #8358461 - Flags: review?(mrbkap) → review+
Attachment #8358462 - Flags: review?(mrbkap) → review+
Attachment #8358463 - Flags: review?(mrbkap) → review+
Comment on attachment 8358464 [details] [diff] [review]
Part 12 - Remove Components wrappers. v1

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

With this patch, do we still need to call JS_WrapObject in nsXPCComponents_Utils::GetComponentsForScope?
Attachment #8358464 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 24

5 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #22)
> Mind filing a bug on converting all of these to use smart pointers?

filed bug 959413.

(In reply to Blake Kaplan (:mrbkap) from comment #23)
> With this patch, do we still need to call JS_WrapObject in
> nsXPCComponents_Utils::GetComponentsForScope?

I think so. In general, the |scope| param to Cu.getComponentsForScope is not same-compartment with the caller.
(Assignee)

Updated

5 years ago
Blocks: 959413
Blocks: 959484
Comment on attachment 8358453 [details] [diff] [review]
Part 2 - Stop using nsISecurityCheckedComponent for nsJSID. v1

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

::: js/xpconnect/src/XPCJSID.cpp
@@ +261,5 @@
>  { 0x00000000, 0x0000, 0x0000,                                                 \
>    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 } }
>  
> +// We pass nsIClassInfo::DOM_OBJECT so that nsJSIID instances may be created
> +// in unprivilged scopes.

nit: unprivileged
(Assignee)

Comment 27

5 years ago
(In reply to Bobby Holley (:bholley) from comment #25)
> https://tbpl.mozilla.org/?tree=Try&rev=301083af0a93

Looks mostly green, except potentially some b2g failures. Re-running those to make sure they're real before I spend too much time on it:

https://tbpl.mozilla.org/?tree=Try&rev=2b72786de839
(Assignee)

Comment 28

5 years ago
Looks like the marionette failures were thankfully related to the base revision, and disappeared on the next try run. Pushed to inbound, with try pushes in comment 25 and comment 27:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7cc30ae56811
Depends on: 961054
Actually, I think this patch queue is broken, specifically part 3. Please bear with me while I explain.

Components.lastResult is supposed to get the result of the last exception thrown. However, when going through the xpconnect dispatch mechanism, the call is first routed through CallMethodHelper::Call() here: <http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#1868>. Several lines later, the last result is reset to NS_ERROR_UNEXPECTED. I'm guessing that the xpcscriptable doesn't go through this helper, so the last result isn't reset and properly reflects the last result.

This change is what is causing bug 961054.
You need to log in before you can comment on or make changes to this bug.