Last Comment Bug 769273 - CCW "Nuke" feature for sandboxes
: CCW "Nuke" feature for sandboxes
Status: RESOLVED FIXED
[MemShrink]
: dev-doc-needed
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla17
Assigned To: Gabor Krizsanits [:krizsa :gabor]
:
Mentors:
Depends on:
Blocks: 697775 771409 775067
  Show dependency treegraph
 
Reported: 2012-06-28 07:32 PDT by Alexandre Poirot [:ochameau]
Modified: 2012-08-08 04:29 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cu.nukeSandbox (3.66 KB, patch)
2012-07-03 05:10 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review-
Details | Diff | Splinter Review
Cu.nukeSandbox (7.33 KB, patch)
2012-07-03 08:29 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: feedback+
Details | Diff | Splinter Review
Cu.nukeSandbox (7.46 KB, patch)
2012-07-05 03:40 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review-
Details | Diff | Splinter Review
part1: Prep for refactoring NukeChromeCrossCompartmentWrappersForGlobal (1.21 KB, patch)
2012-07-10 03:08 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review+
Details | Diff | Splinter Review
part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal (6.18 KB, patch)
2012-07-10 03:08 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review-
Details | Diff | Splinter Review
Bug 769273 - part3: Cu.nukeSandbox (1.66 KB, patch)
2012-07-10 03:09 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review-
Details | Diff | Splinter Review
part4: avoiding innerization in NukeCrossCompartmentWrappers (3.61 KB, patch)
2012-07-10 03:10 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review-
Details | Diff | Splinter Review
part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal (6.34 KB, patch)
2012-07-11 03:09 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review+
Details | Diff | Splinter Review
part3: Cu.nukeSandbox (3.02 KB, patch)
2012-07-12 08:16 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review+
Details | Diff | Splinter Review
part4: avoiding innerization in NukeCrossCompartmentWrappers (3.68 KB, patch)
2012-07-12 11:14 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review+
Details | Diff | Splinter Review

Description Alexandre Poirot [:ochameau] 2012-06-28 07:32:06 PDT
We are using tons of sandboxes in Jetpack and any leak tend to be immediatly very harmfull. Each CommonJS module and each content script are loaded in their own sandbox. And the bad thing is that each time we are leaking a sandbox object, we tend to leak a lot more than this sandbox. Especially (all?!) other sandboxes!
CommonJS module are used to do this:
  const windows = require("window-module");
  const tabs= require("tabs");
  ...
So that if we leak a module, we end up leaking all its dependencies ...

Whereas we know at some point, on addon disabling, that all these sandboxes should be completely disabled. And for content script, we know that we can disable related sandboxes on removal of the document from the bfcache.


Here is a leak example, let's say I have an extremely badly written module, hosted in a sandbox:

  // Import a module which itself require tons of dependencies
  const windows = require("windows"); 

  let watcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].
    getService(Components.interfaces.nsIWindowWatcher);
  watcher.activeWindow = this;

  // or ...
  Components.utils.import("resource://gre/modules/Services.jsm");
  Services.foo = this;

  // or more subtle ...
  let content = windows.getCurrentContentWindow();
  content.addEventListener("load", function () {}, false);


I was thinking that we could apply same gentle treatment khuey has done in bug 695480, but for sandboxes? So that, in case of leak, we would only leak the wrapper and not all sandboxes.

Your thoughts?
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-06-28 07:34:52 PDT
SGTM. Components.utils.nukeSandbox or something of that sort. Should be trivial.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-28 07:37:39 PDT
Yep, although I wouldn't call that a gentle treatment :-P
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-28 07:51:41 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #0)

> So that, in case of leak, we would only leak
> the wrapper and not all sandboxes.

Also, I don't understand what you mean here.
Comment 4 Alexandre Poirot [:ochameau] 2012-06-28 08:05:19 PDT
(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Alexandre Poirot (:ochameau) from comment #0)
> 
> > So that, in case of leak, we would only leak
> > the wrapper and not all sandboxes.
> 
> Also, I don't understand what you mean here.

From what I understood about wrappers, compartements and all ...

If we have such leaky code:

# SandboxModule.js:
  Components.utils.import("resource://gre/modules/Services.jsm");
  Services.foo = this;

And we call Cu.nukeSandbox on it, we will still leak a CCW? in Services.jsm compartment (Services.foo). This wrapper will throw the "dead object" exception while loosing its reference to the SandboxModule global object (this).
So that we won't leak anything from SandboxModule compatment anymore, but still some leftover wrappers in other compartments.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-06-28 08:13:57 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #4)

> And we call Cu.nukeSandbox on it, we will still leak a CCW? in Services.jsm
> compartment (Services.foo). This wrapper will throw the "dead object"
> exception while loosing its reference to the SandboxModule global object
> (this).
> So that we won't leak anything from SandboxModule compatment anymore, but
> still some leftover wrappers in other compartments.

Yes, in a sense. Nuking replaces the guts of the cross-compartment wrapper and turns it into a "dead object proxy", which is just a little stub thing that doesn't point to anything. And it's true that this stub object will remain on the Services object. But I don't think we'd really classify that as a leak.
Comment 6 Dave Townsend [:mossop] 2012-06-28 08:17:24 PDT
I'd like us to use this to nuke the bootstrap.js scope from restartless add-ons when necessary
Comment 7 Gabor Krizsanits [:krizsa :gabor] 2012-07-03 05:10:36 PDT
Created attachment 638659 [details] [diff] [review]
Cu.nukeSandbox

If I get it correctly this should be as simple as this. 

Note that I enforce the argument to be a (wrapped) sandbox object, that is the only thing we want to 'free' this way, right Alex?
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-07-03 05:42:09 PDT
Comment on attachment 638659 [details] [diff] [review]
Cu.nukeSandbox

This will only nuke the single wrapper for the global, which is not what you want. Look at NukeChromeCrossCompartmentWrappersForGlobal.
Comment 9 Gabor Krizsanits [:krizsa :gabor] 2012-07-03 08:29:23 PDT
Created attachment 638739 [details] [diff] [review]
Cu.nukeSandbox

Right. So in our case we need not only the chrome-content case but the other two cases as well. Basically all cross compartment wrappers imo. so I added a new option for nuking for sandboxes, and I renamed a few things according to the functionality changes. Does this look ok?
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-07-03 09:00:42 PDT
Comment on attachment 638739 [details] [diff] [review]
Cu.nukeSandbox

I think I'd prefer to have two separate arguments, one for do/don't nuke global, and one to specify the source compartment.

The ideal thing would actually be to use the compartment filtering machinery I added in bug 655649 patch 4. If you want to use it, I can get those patches landed right away (otherwise I was going to wait until the additional patches were all r+ed). You don't have to, though. Your choice.
Comment 11 Gabor Krizsanits [:krizsa :gabor] 2012-07-04 01:28:23 PDT
(In reply to Bobby Holley (:bholley) from comment #10)
> Comment on attachment 638739 [details] [diff] [review]
> Cu.nukeSandbox
> 
> I think I'd prefer to have two separate arguments, one for do/don't nuke
> global, and one to specify the source compartment.

I feel neutral about this. So if you prefer that, sure.

> 
> The ideal thing would actually be to use the compartment filtering machinery
> I added in bug 655649 patch 4. If you want to use it, I can get those
> patches landed right away (otherwise I was going to wait until the
> additional patches were all r+ed). You don't have to, though. Your choice.

Seems like the same logic indeed. So you mean we should use this filter for NukeChromeCrossCompartmentWrappersForGlobal too? By making RemapWrapper a function pointer argument in RecomputeWrappers? I can do that, but there is a line in NukeCCCWFP I totally don't understand: e.removeFront();
This line seems to be the difference between the two logic and I don't really see why we need it on the first place. If we do e.popFront() in each loop cycle, doesn't this line mean we leave out a wrapper? Is that intentional?
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-07-04 01:32:39 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)

> Seems like the same logic indeed. So you mean we should use this filter for
> NukeChromeCrossCompartmentWrappersForGlobal too? By making RemapWrapper a
> function pointer argument in RecomputeWrappers?

Nonono! I just mean having NukeCrossCompartmentWrappersForGlobal take a CompartmentFilter argument, rather than a forChrome boolean. That's the infrastructure I was referring to.
Comment 13 Gabor Krizsanits [:krizsa :gabor] 2012-07-04 01:47:23 PDT
(In reply to Bobby Holley (:bholley) from comment #12)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Nonono! I just mean having NukeCrossCompartmentWrappersForGlobal take a
> CompartmentFilter argument, rather than a forChrome boolean. That's the
> infrastructure I was referring to.

Yupp, that makes sense. I'd prefer that over the boolean flag. So if it's not a big work to push that patch to central, that would be great. (Assuming the recent version on hg does compile on my machine... sigh)
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-07-04 03:17:36 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> Yupp, that makes sense. I'd prefer that over the boolean flag. So if it's
> not a big work to push that patch to central, that would be great. (Assuming
> the recent version on hg does compile on my machine... sigh)

Landed it to inbound. :-)
Comment 15 Gabor Krizsanits [:krizsa :gabor] 2012-07-05 03:40:15 PDT
Created attachment 639275 [details] [diff] [review]
Cu.nukeSandbox

I think this should be it. I've been struggling with tinderbox and having problem with local testing too right now... So in short, apart from some manual tests, testing is still to do. My plan is to push it to tinderbox and work with Alex together to try it with jetpack.

I had to move the CompartmentFilters to jsfriendapi.h to reach it from XPCComponents.cpp
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-07-05 10:58:25 PDT
Comment on attachment 639275 [details] [diff] [review]
Cu.nukeSandbox

So, looking at this patch, having both |obj| and |targetFilter| doesn't make sense to me. |obj| already implies SingleCompartment(obj) as a compartment filter, so checking each wrapper in obj's scope to see of it (and thus obj) is in a system compartment is silly.

Now, that's certainly what we do now, and I in fact r+ed the patch that did it. But in my defense, it was an emergency "disable this for now" patch to get CPG landed.

So, here's what I'd like to do, in 3 patches.

1 - Move the 'target compartment is chrome' check to the current callers. If they don't want to nuke wrappers to system compartments, they shouldn't be passing in an |obj| that's in a system compartment.

2 - Rename NukeChromeCrossCompartmentWrappersForGlobal to NukeCrossCompartmentWrappers, and have it take 3 arguments: cx, sourceFilter, and targetFilter. The previous callers can get the old behavior by passing SingleCompartment(GetObjectCompartment(obj)) as the targetFilter.

3 - Implement Cu.NukeSandbox.

Make sense? Any thoughts or feedback welcome. :-)
Comment 17 Alexandre Poirot [:ochameau] 2012-07-05 11:04:18 PDT
FYI, here is a patch to use this new method in the SDK:
  https://github.com/mozilla/addon-sdk/pull/479
Comment 18 Gabor Krizsanits [:krizsa :gabor] 2012-07-05 11:25:22 PDT
I will look into this tomorrow, but Alex had one more interesting example.

 <ochameau> for example if I have a sandbox A with an object a={}, and a sandbox B with an object b={}. Then I do A.a.foo=B.b. If I do Cu.nukeSandbox(A), B.b will be collected, right?

So this version only breaks references to the sandbox itself, while the feature maybe should be instead break the references to the entire compartment... If this is the case, and we need this heavy weight version, probably I will do that in a new function... Anyway, I will discuss this tomorrow with Alex and with a clear head will think it over and ping you on irc probably before the next round.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-07-05 12:46:39 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> I will look into this tomorrow, but Alex had one more interesting example.
> 
>  <ochameau> for example if I have a sandbox A with an object a={}, and a
> sandbox B with an object b={}. Then I do A.a.foo=B.b. If I do
> Cu.nukeSandbox(A), B.b will be collected, right?

That is the correct behavior, yes. Nuking references to the global only is pretty pointless.


> So this version only breaks references to the sandbox itself

Are you sure about that? I'm pretty sure that's not the case, see the line:

> if (&wrapped->global() == global)

Which nukes all CC references to |wrapped| where |wrapped| is in the scope of |global|.
Comment 20 Gabor Krizsanits [:krizsa :gabor] 2012-07-06 04:58:04 PDT
(In reply to Bobby Holley (:bholley) from comment #19)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> Are you sure about that? I'm pretty sure that's not the case, see the line:

I was making that comment without having the code in front of me... the name of the function tricked me.

I cannot add much to your suggestion it makes perfectly sense. Only thing I would change is that I would even remove cx from the arguments. It is only used for fetching a reference to the runtime, and it is grabbed from the stack... I will add that change to patch 2 if you are ok with it.
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 04:58:59 PDT
SGTM.
Comment 22 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:08:05 PDT
Created attachment 640548 [details] [diff] [review]
part1: Prep for refactoring NukeChromeCrossCompartmentWrappersForGlobal
Comment 23 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:08:58 PDT
Created attachment 640549 [details] [diff] [review]
part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal
Comment 24 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:09:49 PDT
Created attachment 640550 [details] [diff] [review]
Bug 769273 - part3: Cu.nukeSandbox
Comment 25 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:10:56 PDT
Created attachment 640551 [details] [diff] [review]
part4: avoiding innerization in NukeCrossCompartmentWrappers
Comment 26 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:15:00 PDT
I'm not so sure about this try result. https://tbpl.mozilla.org/?tree=Try&rev=7a06079ccd1f

I might messed something up while refactoring this nuke wrappers function, trying to figure out if that is the case, or is this just an intermittent failure I should not take that seriously. I think it would be great to come up with a good test for this feature.

By the way, I could not get rid of the ctx arg, because reaching for the ctx stack from wrappers.cpp is not that trivial as I thought it is.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 03:47:52 PDT
Comment on attachment 640549 [details] [diff] [review]
part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal

># HG changeset patch
># Parent c3a3530d3409ab6802ba782c10c5598dc20685c7
># User Gabor Krizsanits <gkrizsanits@mozilla.com>
>Bug 769273 - part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal
>
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -6842,16 +6842,17 @@
>       NS_ENSURE_TRUE(currentInner, NS_OK);
> 
>       JSObject* obj = currentInner->FastGetGlobalJSObject();
>-      // We only want to nuke wrappers for chrome->content case
>+      // We only want to nuke wrappers for the chrome->content case
>       if (obj && !js::IsSystemCompartment(js::GetObjectCompartment(obj))) {
>         JSContext* cx =
>           nsContentUtils::ThreadJSContextStack()->GetSafeJSContext();
> 
>         JSAutoRequest ar(cx);
>-
>-        js::NukeChromeCrossCompartmentWrappersForGlobal(cx, obj,
>-                                                        window->IsInnerWindow() ? js::DontNukeForGlobalObject :
>-                                                                                  js::NukeForGlobalObject);
>+        js::NukeCrossCompartmentWrappers(cx, 
>+                                         js::ContentCompartmentsOnly(),
>+                                         js::SingleCompartment(js::GetObjectCompartment(obj)),
>+                                         window->IsInnerWindow() ? js::DontNukeForGlobalObject :
>+                                                                   js::NukeForGlobalObject);

There's your problem! The old code was nuking chrome->content wrappers, and you switched it to nuking content->content wrappers. ;-)

Change this to CompartmentWithPrincipals(systemPrincipal) and it should work. Or you can add a ChromeCompartmentsOnly filter - that might be nicer.
Comment 28 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 03:53:16 PDT
(In reply to Bobby Holley (:bholley) from comment #27)
> Comment on attachment 640549 [details] [diff] [review]
 
> There's your problem! The old code was nuking chrome->content wrappers, and
> you switched it to nuking content->content wrappers. ;-)
> 
> Change this to CompartmentWithPrincipals(systemPrincipal) and it should
> work. Or you can add a ChromeCompartmentsOnly filter - that might be nicer.

Uh... thanks, I've even added ChromeCompartmentsOnly at some point...
Comment 29 Gabor Krizsanits [:krizsa :gabor] 2012-07-10 06:09:51 PDT
Actually... It was not entirely bad. Because I did 
if (sourceFilter.match(c))
  continue;

So technically it did the right thing, except it was extremely confusing. I will fix this. On the other hand I could finally check the result of my first patch on try (try is working again yaaay!), and it looked quite similar to this one https://tbpl.mozilla.org/?tree=Try&rev=27acd3fcdee0. 

So this might not be a leak this patch introduced. (there is an intermittent bug related to it: Bug 754804 but that reports a lot less leaking windows)
Comment 30 Gabor Krizsanits [:krizsa :gabor] 2012-07-11 03:09:59 PDT
Created attachment 640990 [details] [diff] [review]
part2: Refactoring NukeChromeCrossCompartmentWrappersForGlobal

this patch is functionally identical to the previous version, just I fixed the previously mentioned confusing usage of the sourceFilter.
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2012-07-12 02:20:59 PDT
Comment on attachment 640550 [details] [diff] [review]
Bug 769273 - part3: Cu.nukeSandbox


>+    NukeCrossCompartmentWrappers(cx, AllCompartments(), 
>+                                 AllCompartments(), NukeForGlobalObject);

This will nuke all cross-compartment wrappers in existence and destroy the world as we know it: http://i49.tinypic.com/28iysll.jpg


r-
Comment 32 Gabor Krizsanits [:krizsa :gabor] 2012-07-12 03:39:15 PDT
(In reply to Bobby Holley (:bholley) from comment #31)
> Comment on attachment 640550 [details] [diff] [review]
> This will nuke all cross-compartment wrappers in existence and destroy the
> world as we know it: http://i49.tinypic.com/28iysll.jpg

How I hate to work with multiple patches... Anyway, I will add some tests while stop nuking the entire world, and save the picture for the future.
Comment 33 Gabor Krizsanits [:krizsa :gabor] 2012-07-12 08:16:04 PDT
Created attachment 641478 [details] [diff] [review]
part3: Cu.nukeSandbox

Should not nuke the world this time. I've added a very simple test as well, but will test it soon against jetpack anyway.
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2012-07-12 08:48:28 PDT
Comment on attachment 641478 [details] [diff] [review]
part3: Cu.nukeSandbox

Tactical nuclear weapons are better. r=bholley
Comment 35 Gabor Krizsanits [:krizsa :gabor] 2012-07-12 09:07:02 PDT
(In reply to Bobby Holley (:bholley) from comment #34)
> Tactical nuclear weapons are better. r=bholley

Alright it might not be a 'gentle treatment' for cleaning up sandbox, as the original bug report requested but it did the job thoroughly and the provided extra functionality matched the name perfectly :) And yeah, I admit it's meme worthy, sorry about it...
Comment 36 Bobby Holley (:bholley) (busy with Stylo) 2012-07-12 09:10:01 PDT
Comment on attachment 640551 [details] [diff] [review]
part4: avoiding innerization in NukeCrossCompartmentWrappers

>diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h
>--- a/js/src/jsfriendapi.h
>+++ b/js/src/jsfriendapi.h
>@@ -788,10 +788,10 @@
> GetErrorTypeNameFromNumber(JSContext* cx, const unsigned errorNumber);
> 
> /* Implemented in jswrapper.cpp. */
>-typedef enum NukedGlobalHandling {
>-    NukeForGlobalObject,
>-    DontNukeForGlobalObject
>-} NukedGlobalHandling;
>+typedef enum NukeReferencesToWindow {
>+    NukeOuters,
>+    DontNukeOuters
>+} NukeReferencesToWindow;

{,Dont}NukeOuters is not a great name IMO. I'd prefer NukeWindowReferences and DontNukeWindowReferences.


>             JSObject *wobj = &e.front().value.get().toObject();
>-            JSObject *wrapped = UnwrapObject(wobj, false);
>+            BaseProxyHandler *handler = GetProxyHandler(wobj);
> 
>-            if (nukeGlobal == DontNukeForGlobalObject && wrapped->isGlobal())
>+            if (nukeReferencesToWindow == DontNukeOuters && handler->isOuterWindow())


This doesn't work right. wobj is the cross-compartment wrapper (not the wrappee), so you still need the UnwrapObject call. And once you do that, GetProxyHandler isn't guaranteed to succeed, because the object may not be a proxy. So Unwrap (passing stopAtOuter=true), and the check obj->getClass()->ext.outerObject.
Comment 37 Bobby Holley (:bholley) (busy with Stylo) 2012-07-12 10:47:13 PDT
(In reply to Bobby Holley (:bholley) from comment #36)
> and the check obj->getClass()->ext.outerObject.

err, ->ext.innerObject.
Comment 38 Gabor Krizsanits [:krizsa :gabor] 2012-07-12 11:14:20 PDT
Created attachment 641539 [details] [diff] [review]
part4: avoiding innerization in NukeCrossCompartmentWrappers

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