Closed Bug 681556 Opened 13 years ago Closed 12 years ago

Default sandboxName to the caller's filename

Categories

(Core :: XPConnect, enhancement)

Product:
Core
Component:
XPConnect
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: jruderman, Assigned: sandervv)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

Set sandboxName to caller's filename per default
1.48 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review 
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.)
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.
Assignee: nobody → sandervv
Whiteboard: [MemShrink] → [MemShrink:P2]
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
Sander, comment 2 tells me that you have a patch here.  :-)
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").
Attachment #561520 - Flags: review?(peterv)
> 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 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?
Attachment #561520 - Flags: review?(peterv) → review+
Sander, is this still something you'd like to finish?
Sander, if you just need it landed let me know, I can do it.
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?
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.
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?
Attachment #561520 - Attachment is obsolete: true
Attachment #589065 - Flags: review?(mrbkap)
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.
Attachment #589065 - Flags: review?(mrbkap) → review+
Hrm, |cc| seems unused in this patch?
(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.
(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.
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.
> has past.

Er, passed.
https://hg.mozilla.org/mozilla-central/rev/d9b704544273
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: