Bug 759585 is adding a sameZoneAs option to the sandbox constructor, looks like roughly the same semantics as sharing compartments used to have. We should make all our sandboxes share zones to save memory. Alex, sounds like your bag, but this is lower priority than any remaining work on the API landings and any help you can bring to the private browsing stuff in 841823.

Before the patch: 63,674,552 B (100.0%) -- explicit ... │ │ │ ├─────239,636 B (00.38%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmpyfmuix.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180)) │ │ │ │ ├──172,032 B (00.27%) -- gc-heap │ │ │ │ │ ├──106,224 B (00.17%) ── unused-gc-things │ │ │ │ │ ├───34,480 B (00.05%) -- objects │ │ │ │ │ │ ├──25,760 B (00.04%) ── cross-compartment-wrapper │ │ │ │ │ │ └───8,720 B (00.01%) ── function │ │ │ │ │ ├───20,384 B (00.03%) ── sundries │ │ │ │ │ └───10,944 B (00.02%) ── shapes/tree/global-parented │ │ │ │ ├───32,768 B (00.05%) ── cross-compartment-wrapper-table │ │ │ │ ├───21,972 B (00.03%) ── other-sundries │ │ │ │ └───12,864 B (00.02%) ── objects-extra/slots After: 54,513,142 B (100.0%) -- explicit ... │ │ │ ├─────130,684 B (00.24%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmpdsdxel.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180)) │ │ │ │ ├───65,192 B (00.12%) -- gc-heap │ │ │ │ │ ├──34,480 B (00.06%) -- objects │ │ │ │ │ │ ├──25,760 B (00.05%) ── cross-compartment-wrapper │ │ │ │ │ │ └───8,720 B (00.02%) ── function │ │ │ │ │ ├──19,768 B (00.04%) ── sundries │ │ │ │ │ └──10,944 B (00.02%) ── shapes/tree/global-parented │ │ │ │ ├───32,768 B (00.06%) ── cross-compartment-wrapper-table │ │ │ │ ├───19,860 B (00.04%) ── other-sundries │ │ │ │ └───12,864 B (00.02%) ── objects-extra/slots Looks like a big win!

Could you explain what this patch does? I didn't do a good job of explaining what's needed in bug 759585, and I don't know enough about how things worked before CPG to understand comment 0. Here's my understanding of the situation: - Now that the zones patch has landed, all sandboxes will be created in the same "system zone" by default, so they'll share a lot of GC resources. - If a sandbox is not tied to any particular page, then that's the behavior we want. - However, if a sandbox is expected to die when a page dies, then we want to stick it in the same zone as the page. So we would want to pass some object from the page as the sameZoneAs parameter. Bobby, does that sound right? Is that what the patch does? Also, did you take the "before" measurement using a build where the zones patch was applied?

Ah ok, I thought your patch was only doing something when you pass sameZoneAs. So if I follow you correctly, as SDK modules are using system principal, we shouldn't have to do anything to share the same zone... The "before" was using a nightly without platform zone patch. Here is the with the platform patch but without the sdk patch I just submitted: │ │ │ ├─────130,684 B (00.22%) -- compartment([System Principal], resource://gre/modules/commonjs/sdk/deprecated/traits.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///c:/users/alex/appdata/local/temp/tmp2_mxof.mozrunner/extensions/anonid0-reddit-panel@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:180)) │ │ │ │ ├───65,192 B (00.11%) -- gc-heap │ │ │ │ │ ├──34,480 B (00.06%) -- objects │ │ │ │ │ │ ├──25,760 B (00.04%) ── cross-compartment-wrapper │ │ │ │ │ │ └───8,720 B (00.01%) ── function │ │ │ │ │ ├──19,768 B (00.03%) ── sundries │ │ │ │ │ └──10,944 B (00.02%) ── shapes/tree/global-parented │ │ │ │ ├───32,768 B (00.06%) ── cross-compartment-wrapper-table │ │ │ │ ├───19,860 B (00.03%) ── other-sundries │ │ │ │ └───12,864 B (00.02%) ── objects-extra/slots It confirms what you just said. So we don't have to do anything for module sandboxes, but we will have to pass sameZoneAs for content script sandboxes, that aren't using system principal but page principal instead.

(In reply to Alexandre Poirot (:ochameau) from comment #3) > It confirms what you just said. So we don't have to do anything for module > sandboxes, but we will have to pass sameZoneAs for content script sandboxes, > that aren't using system principal but page principal instead. Correct, unless the GC characteristics of the addon SDK would warrant a separate zone. For example, if they accumulated a lot of garbage, it would be advantageous to be able to GC them separately from the rest of the system zone (all the JSMs and such).

(In reply to Bobby Holley (:bholley) from comment #4) > Correct, unless the GC characteristics of the addon SDK would warrant a > separate zone. For example, if they accumulated a lot of garbage, it would > be advantageous to be able to GC them separately from the rest of the system > zone (all the JSMs and such). I can easily think about addon disabling/uninstall, where we are going to trash all module sandboxes. But is it really worth? Is it going to introduce additional cost in code being called between system global zone and addons zones? Or some more memory cost due to wrappers? Addon disabling isn't something that happen often...

(In reply to Alexandre Poirot (:ochameau) from comment #5) > (In reply to Bobby Holley (:bholley) from comment #4) > > Correct, unless the GC characteristics of the addon SDK would warrant a > > separate zone. For example, if they accumulated a lot of garbage, it would > > be advantageous to be able to GC them separately from the rest of the system > > zone (all the JSMs and such). > > I can easily think about addon disabling/uninstall, where we are going to > trash all module sandboxes. But is it really worth? Is it going to introduce > additional cost in code being called between system global zone and addons > zones? Or some more memory cost due to wrappers? Addon disabling isn't > something that happen often... Yeah, they should probably go in the system zone. Content-scripts should definitely go in the associated content zone, though.

Here is a patch to force using page zone for content script.

As for previous patch, I'm not sure this patch is mandatory? Aren't we sharing zone compartment of the given principal? We do something similar to this, with contentWindow being a reference to the page window object: Cu.Sandbox(contentWindow {sandboxPrototype: contentWindow, ...}) ?

(In reply to Alexandre Poirot (:ochameau) from comment #8) > As for previous patch, I'm not sure this patch is mandatory? > Aren't we sharing zone compartment of the given principal? > > We do something similar to this, with contentWindow being a reference to the > page window object: > Cu.Sandbox(contentWindow {sandboxPrototype: contentWindow, ...}) ? All sandboxes go into the system zone, regardless of their principal, unless you pass sameZoneAs.

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/e7d0e6171f9f9be8cf29106ff182a1547f702e99 Bug 844180: Share content script and page zones. https://github.com/mozilla/addon-sdk/commit/ab0413ef0f12c063a592cd1c2552cfdca77ad7d1 Merge pull request #942 from ochameau/contentScriptZone Bug 844180: Share content script and page zones. r=@mossop

Commits pushed to integration at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/e7d0e6171f9f9be8cf29106ff182a1547f702e99 Bug 844180: Share content script and page zones. https://github.com/mozilla/addon-sdk/commit/ab0413ef0f12c063a592cd1c2552cfdca77ad7d1 Merge pull request #942 from ochameau/contentScriptZone

This looks fixed to me.