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?

SGTM. Components.utils.nukeSandbox or something of that sort. Should be trivial.

Yep, although I wouldn't call that a gentle treatment :-P

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

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

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

I'd like us to use this to nuke the bootstrap.js scope from restartless add-ons when necessary

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

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

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

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

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

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

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

FYI, here is a patch to use this new method in the SDK: https://github.com/mozilla/addon-sdk/pull/479

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.

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

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

SGTM.

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

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

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)

this patch is functionally identical to the previous version, just I fixed the previously mentioned confusing usage of the sourceFilter.

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-

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

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 on attachment 641478 [details] [diff] [review] part3: Cu.nukeSandbox Tactical nuclear weapons are better. r=bholley

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

(In reply to Bobby Holley (:bholley) from comment #36) > and the check obj->getClass()->ext.outerObject. err, ->ext.innerObject.

https://tbpl.mozilla.org/?tree=Try&rev=8387b4c1dafa http://hg.mozilla.org/integration/mozilla-inbound/rev/164bd49a19a1 http://hg.mozilla.org/integration/mozilla-inbound/rev/9a6fcbd8f96a http://hg.mozilla.org/integration/mozilla-inbound/rev/9b8e56ada40e http://hg.mozilla.org/integration/mozilla-inbound/rev/74b54ce37285

https://hg.mozilla.org/mozilla-central/rev/164bd49a19a1 https://hg.mozilla.org/mozilla-central/rev/9a6fcbd8f96a https://hg.mozilla.org/mozilla-central/rev/9b8e56ada40e https://hg.mozilla.org/mozilla-central/rev/74b54ce37285