Last Comment Bug 681556 - Default sandboxName to the caller's filename
: Default sandboxName to the caller's filename
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla13
Assigned To: Sander Mathijs van Veen
:
Mentors:
Depends on: 673331
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-23 20:08 PDT by Jesse Ruderman
Modified: 2012-02-01 11:13 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Set sandboxName to caller's filename per default (1.41 KB, patch)
2011-09-21 10:55 PDT, Sander Mathijs van Veen
peterv: review+
Details | Diff | Splinter Review
Set sandboxName to caller's filename per default (1.48 KB, patch)
2012-01-16 16:57 PST, Sander Mathijs van Veen
mrbkap: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-08-23 20:08:19 PDT
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 [:njn] 2011-08-29 00:07:04 PDT
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 2011-09-21 07:11:58 PDT
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/sandervv@gmail.com-721db2435ca7
Comment 3 :Ehsan Akhgari 2011-09-21 10:30:40 PDT
Sander, comment 2 tells me that you have a patch here.  :-)
Comment 4 Sander Mathijs van Veen 2011-09-21 10:55:29 PDT
Created attachment 561520 [details] [diff] [review]
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 [:njn] 2011-09-27 11:25:57 PDT
> 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] - away till Aug 1st 2011-10-17 12:22:45 PDT
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 2012-01-07 05:58:45 PST
Sander, is this still something you'd like to finish?
Comment 8 Nicholas Nethercote [:njn] 2012-01-07 12:30:52 PST
Sander, if you just need it landed let me know, I can do it.
Comment 9 Sander Mathijs van Veen 2012-01-09 10:44:11 PST
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 [:njn] 2012-01-09 15:37:29 PST
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 2012-01-16 16:57:56 PST
Created attachment 589065 [details] [diff] [review]
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/pentadactyl@dactyl.googlecode.com.xpi!/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) 2012-01-27 01:42:21 PST
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 2012-01-27 05:18:23 PST
Hrm, |cc| seems unused in this patch?
Comment 14 Nicholas Nethercote [:njn] 2012-01-27 11:54:08 PST
(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 2012-01-27 12:57:26 PST
(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 [:njn] 2012-01-30 20:07:48 PST
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 [:njn] 2012-01-30 20:08:11 PST
> has past.

Er, passed.
Comment 18 Nicholas Nethercote [:njn] 2012-01-31 15:37:28 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b704544273
Comment 19 Ed Morley [:emorley] 2012-02-01 11:13:42 PST
https://hg.mozilla.org/mozilla-central/rev/d9b704544273

Note You need to log in before you can comment on or make changes to this bug.