951948 - Remove same-compartment Components wrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

(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

Comment 6

[Bobby Holley (:bholley)]

Uploading patches and flagging for review.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make nsJSID.initialize [noscript]. v1

This lets us remove the usage of nsISecurityCheckedComponent. See the next patch.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop using nsISecurityCheckedComponent for nsJSID. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove most nsIXPCScriptable junk on nsXPCComponents. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make AttachComponentsObject a non-static method on XPCWrappedNativeScope. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 9 - Store nsXPCComponentsBase on XPCWrappedNativeScope, so that we can have either. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 10 - Add a way for automation to force the creation of a privileged Components object for an unprivileged scope. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 11 - Use nsXPCComponentsBase for everything but system-principaled scopes. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 18

[Bobby Holley (:bholley)]

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.

Comment 19

[(not currently active) Ted Mielczarek]

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.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

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))?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 24

[Bobby Holley (:bholley)]

(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.

Comment 25

[Bobby Holley (:bholley)]

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

Comment 26

[Andrew McCreight [:mccr8]]

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

Comment 27

[Bobby Holley (:bholley)]

(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

Comment 28

[Bobby Holley (:bholley)]

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

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f9de8b26721
https://hg.mozilla.org/mozilla-central/rev/f24fce132e1a
https://hg.mozilla.org/mozilla-central/rev/1469d62ddbf4
https://hg.mozilla.org/mozilla-central/rev/8773a793e758
https://hg.mozilla.org/mozilla-central/rev/9469174aff9b
https://hg.mozilla.org/mozilla-central/rev/efffc53426b4
https://hg.mozilla.org/mozilla-central/rev/fbdd66a8b18f
https://hg.mozilla.org/mozilla-central/rev/4c687bba563b
https://hg.mozilla.org/mozilla-central/rev/b61deb1bee05
https://hg.mozilla.org/mozilla-central/rev/707abbb92a8b
https://hg.mozilla.org/mozilla-central/rev/7cc30ae56811

Comment 30

[Joshua Cranmer [:jcranmer]]

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.

Comment 31

[Bobby Holley (:bholley)]

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