681556 - Default sandboxName to the caller's filename

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Jesse Ruderman]

Instead of relying on callers of Components.utils.Sandbox to specify a name, we should make the Sandbox constructor inspect its caller and use the caller's filename.

(If the caller does specify a name, the specified name could override the default or be appended.)

Comment 1

[Nicholas Nethercote [inactive]]

Sander, this has your name all over it :)

Bug 673331 put all the plumbing in place, the only tricky part is working out how to get the caller's filename from within nsXPCComponents_utils_Sandbox::CallOrConstruct.

Comment 2

[Mozilla RelEng Bot]

Try run for 721db2435ca7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=721db2435ca7
Results (out of 171 total builds):
    exception: 4
    success: 157
    warnings: 9
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-721db2435ca7

Comment 3

[(no longer active)]

Sander, comment 2 tells me that you have a patch here.  :-)

Comment 4

[Sander Mathijs van Veen]

Attachment: Set sandboxName to caller's filename per default

Example of the patch:

│  ├─────281,496 B (00.52%) -- compartment([System Principal], resource://jid0-qbniplfdfa4lpdrjhac6vbqn20q-at-jetpack-api-utils-lib/securable-module.js, 0xffffffffa4b91000)
│  │     ├──155,648 B (00.29%) -- gc-heap
│  │     │  ├───63,136 B (00.12%) -- arena-unused
│  │     │  ├───46,432 B (00.09%) -- objects
│  │     │  ├───26,720 B (00.05%) -- shapes
│  │     │  ├───15,600 B (00.03%) -- scripts
│  │     │  ├────2,464 B (00.00%) -- type-objects
│  │     │  ├──────608 B (00.00%) -- arena-headers
│  │     │  ├──────560 B (00.00%) -- arena-padding
│  │     │  └──────128 B (00.00%) -- strings
│  │     ├───65,536 B (00.12%) -- mjit-code
│  │     │   ├──64,752 B (00.12%) -- unused
│  │     │   ├─────704 B (00.00%) -- regexp
│  │     │   └──────80 B (00.00%) -- method
│  │     ├───17,256 B (00.03%) -- script-data
│  │     ├───16,304 B (00.03%) -- analysis-temporary
│  │     ├───11,264 B (00.02%) -- property-tables
│  │     ├────7,232 B (00.01%) -- object-slots
│  │     ├────3,664 B (00.01%) -- type-inference
│  │     │    ├──2,464 B (00.00%) -- object-main
│  │     │    └──1,200 B (00.00%) -- script-main
│  │     ├────3,072 B (00.01%) -- shape-kids
│  │     ├────1,200 B (00.00%) -- object-empty-shapes
│  │     └──────320 B (00.00%) -- string-chars

If bugzilla tweaks addon is installed on a new profile, 39 of 40 system compartments are identified (the only one missing is the "main system compartment").

Comment 5

[Nicholas Nethercote [inactive]]

> If bugzilla tweaks addon is installed on a new profile, 39 of 40 system
> compartments are identified (the only one missing is the "main system
> compartment").

Nice!

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 561520 [details] [diff] [review]
Set sandboxName to caller's filename per default

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

::: js/src/xpconnect/src/xpccomponents.cpp
@@ +3418,5 @@
> +    // specified, use the caller's filename as sandboxName.
> +    if (sandboxName.IsEmpty()) { 
> +        nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));
> +        if(NS_FAILED(rv))
> +            return rv;

Just use nsXPConnect::GetXPConnect().

@@ +3425,5 @@
> +        nsAXPCNativeCallContext *cc = nsnull;
> +        xpc->GetCurrentNativeCallContext(&cc);
> +
> +        if(!cc)
> +            return NS_ERROR_FAILURE;

This should return ThrowAndFail. Although I think just using the empty string in case GetCurrentNativeCallContext fails should be fine, no?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sander, is this still something you'd like to finish?

Comment 8

[Nicholas Nethercote [inactive]]

Sander, if you just need it landed let me know, I can do it.

Comment 9

[Sander Mathijs van Veen]

I'm sorry. I totally forgot that there was a patch to fix and close this bug. @njn: thanks if you can do that! 

When this patch is landed, I think we can also close Bug 681555. Since the patch sets the sandboxName by default to the script's filename, there is no reason to trigger a warning.

Given that the patch lands, how about Bug 680821? Is setting sandboxName to options.filename still necessary?

Comment 10

[Nicholas Nethercote [inactive]]

Sander, would you mind updating the patch for peterv's comments?

Also, in "resource://jid0-qbniplfdfa4lpdrjhac6vbqn20q-at-jetpack-api-utils-lib/securable-module.js" I think the "qbniplfdfa4lpdrjhac6vbqn20q" is a unique identifier for the add-on, is that right?  Is it possible to convert that to something human-readable?

> When this patch is landed, I think we can also close Bug 681555. Since the
> patch sets the sandboxName by default to the script's filename, there is no
> reason to trigger a warning.

Sounds reasonable.

> Given that the patch lands, how about Bug 680821? Is setting sandboxName to
> options.filename still necessary?

I don't know, I find that bug very confusing, esp. since I don't understand how the add-on team uses Github.

Comment 11

[Sander Mathijs van Veen]

Attachment: Set sandboxName to caller's filename per default

With the bugzilla tweaks add-on, I was unable to reproduce the unidentified system compartments. That add-on explicitly set sandboxName, so there were no more unidentified system compartments caused by that add-on.

I tried the patch this time with the add-on "pentadactyl". Example of the patch (irrelevant parts omitted):

│  ├─────181,184 B (00.21%) -- compartment([System Principal], resource://dactyl-rrr2hlk71/main.jsm, 0xffffffffa46f2000)
│  │     ├──131,072 B (00.15%) -- analysis-temporary
│  │     ├───45,056 B (00.05%) -- gc-heap
│  │     │  [...]
│  │     ├────3,112 B (00.00%) -- shapes-extra
│  │     │  [...]
│  │     ├────1,792 B (00.00%) -- object-slots
│  │     ├──────112 B (00.00%) -- string-chars
│  │     ├───────24 B (00.00%) -- script-data
│  │     └───────16 B (00.00%) -- type-inference
│  │             └──16 B (00.00%) -- script-main
│  ├─────142,064 B (00.17%) -- compartment([System Principal], jar:file:///home/sander/.mozilla/firefox/hb9s49f6.2012-jan/extensions/[e-mail]!/bootstrap.js, 0xffffffffabf72000)
│  │     ├──114,688 B (00.13%) -- gc-heap
│  │     │  [...]
│  │     ├───10,216 B (00.01%) -- shapes-extra
│  │     │  [...]
│  │     ├────7,992 B (00.01%) -- script-data
│  │     ├────6,624 B (00.01%) -- object-slots
│  │     ├────2,304 B (00.00%) -- string-chars
│  │     └──────240 B (00.00%) -- type-inference
│  │             └──240 B (00.00%) -- script-main

Notice the different location identifiers "resource://" and "jar:file//". The former is caused by this patch. If someone knows an add-on with many (different) sandboxName-less system compartments, please let me know.

@peterv: Please note that "do_GetService(nsIXPConnect::GetCID(), &rv))" is used at multiple places in js/xpconnect/src:

$ grep "do_GetService(nsIXPConnect::GetCID()" -Rn js/xpconnect/src
js/xpconnect/src/XPCComponents.cpp:2969:    nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));
js/xpconnect/src/XPCComponents.cpp:3149:            nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID()));
js/xpconnect/src/XPCComponents.cpp:3392:        nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID(), &rv);
js/xpconnect/src/nsXPConnect.cpp:2836:    nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));
js/xpconnect/src/nsXPConnect.cpp:2846:    nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));
js/xpconnect/src/nsXPConnect.cpp:2855:    nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));

Since you suggested to use "nsXPConnect::GetXPConnect()." Should we file a bug for those calls as well?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 589065 [details] [diff] [review]
Set sandboxName to caller's filename per default

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

Looks good to me.

Feel free to file a bug on the rest of the do_GetServices.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3259,5 @@
> +        // get the xpconnect native call context
> +        nsAXPCNativeCallContext *cc = nsnull;
> +        xpc->GetCurrentNativeCallContext(&cc);
> +
> +        if(!cc)

Nit: space after the if here.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hrm, |cc| seems unused in this patch?

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to Ms2ger from comment #13)
> Hrm, |cc| seems unused in this patch?

No, it's tested against NULL.

Sander, I can land this patch for you on Monday.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> (In reply to Ms2ger from comment #13)
> > Hrm, |cc| seems unused in this patch?
> 
> No, it's tested against NULL.

Well, yes.

Comment 16

[Nicholas Nethercote [inactive]]

Sorry for the delay, I just pushed to the try server, and if it all goes ok I'll land tomorrow after the FF12 deadline has past.

Comment 17

[Nicholas Nethercote [inactive]]

> has past.

Er, passed.

Comment 18

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b704544273

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9b704544273