Using the patch from bug 671159 I found that when I start Firefox with Bugzilla Tweaks installed, there are 40(!) system compartments (ie. compartments with a codebase of "[System Principal]". This is just with about:memory?verbose loaded. If I reload about:memory?verbose repeatedly, I see the number cycle between 40, 42, 44, 46, but not in any particular order. I also tried Browse By Name, it only seems to create a single extra system compartment. When viewing bugzilla pages with Bugzilla Tweaks enabled, the number of compartments would typically increase by up to 10 or 15 for each bug report opened in a new tab, but I couldn't see an obvious pattern predicting the number. Is this an inherent problem with jetpack-based add-ons, or something specific to Bugzilla Tweaks? khuey suspected the former. Just in case your reaction is "lots of compartments, so what?" -- I saw that most of these compartments are method JITting a tiny-but-non-zero amount of code. This adds up, because the minimum code chunk size for the jmit is 64KB. (Doing less is difficult because VirtualAlloc doesn't allow sub-64KB aligned blocks.)

Jetpack creates one Sandbox per module so if there is one compartment for each Sandbox, we can easily end up with a lot of compartments! Most of these sandboxes use system principal. i.e. sandboxes for common js modules. Then, when you use the page-mod API in order to have some content script applied on web pages, we are creating another set of sandboxes. This time, they are using the same principal than the web page they are targeting. I'm currently working on bug 636145 in order to improve our sandboxing of common js module, as avoid giving system principal to all modules. But it appears to be quite challenging. So if you think there is some platform capabilities we should try using in jetpack, do not hesitate to mention them.

(In reply to comment #0) > Just in case your reaction is "lots of compartments, so what?" -- I saw that > most of these compartments are method JITting a tiny-but-non-zero amount of > code. This adds up, because the minimum code chunk size for the jmit is > 64KB. (Doing less is difficult because VirtualAlloc doesn't allow sub-64KB > aligned blocks.) This is probably something to keep in mind with compartment-per-global too ...

(In reply to comment #2) > This is probably something to keep in mind with compartment-per-global too Compartment-per-global is blocked on making compartments lightweight enough to do compartment-per-global to avoid this very problem. In practice, that should mean moving the JIT caches out of the compartments and into the runtime.

I've filed bug 672636 and bug 672637 for the two major issues on the Jetpack side here. I'll drop the MemShrink tag on this bug for now, and we'll reevaluate when at least one of the dependencies gets fixed.

I see two ways to fix this problem: (a) create fewer compartments; (b) make compartments cheaper. Comment 3 is about (b); it'll have to happen to get compartment-per-global. The spin-off bugs (bug 672636 and bug 672637) are about (a). If we achieve (b), (a) won't hurt but will it be truly necessary? (Having said that, it'd be good to avoid bloating about:memory with lots of tiny uninteresting compartments.)

We probably could create compartment per add-on and use function wrappers for module instead of sandboxes. That's what for example nodejs does by default.

Another fun data point: if I open about:memory?verbose and techcrunch.com at start-up, I get 114 compartments. Yikes.

And if after opening techcrunch.com I open five articles from the front page in new tabs, I end up with 12 user compartments and 305 system compartments. 123(!) of these system compartments had identical stats: │ ├──────140,037 B (00.03%) -- compartment([System Principal], 0x7fa6fde68000) │ │ ├───69,632 B (00.02%) -- gc-heap │ │ │ ├──39,064 B (00.01%) -- arena-unused │ │ │ ├──17,600 B (00.00%) -- objects │ │ │ ├──11,392 B (00.00%) -- shapes │ │ │ ├───1,104 B (00.00%) -- arena-padding │ │ │ ├─────408 B (00.00%) -- arena-headers │ │ │ ├──────64 B (00.00%) -- strings │ │ │ └───────0 B (00.00%) -- xml │ │ ├───65,536 B (00.02%) -- mjit-code │ │ ├────3,328 B (00.00%) -- object-slots │ │ ├────1,381 B (00.00%) -- scripts │ │ ├──────160 B (00.00%) -- string-chars │ │ ├────────0 B (00.00%) -- mjit-data │ │ ├────────0 B (00.00%) -- tjit-code │ │ └────────0 B (00.00%) -- tjit-data │ │ ├──0 B (00.00%) -- allocators-main │ │ └──0 B (00.00%) -- allocators-reserve So clearly we're getting huge numbers of identical compartments.

Nick: the explosion of compartments number on techcrunch only appears when buzilla tweak is installed? Here is the main module of this addon: https://bitbucket.org/ehsan/bugzilla-tweaks/src/431a74ba2744/lib/main.js#cl-38 I don't think that's related to page-mod API as it will be triggered only on mozilla websites but I suspect context-menu API to add workers for each tab (or even worse, all content documents?) Drew: I think that you are the best one for such question about context-menu? ehsan: your bugs doesn't make much sense as there is no differences between internal and developer modules. We are currently working on simplifying our module loader: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/2da6241694dc32a1 So that the code would be wayyy shorter/simplier/faster. We had a long discussion about choosing the better platform API: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/f251a6810767aa7a/02b92fb28caf6829 I believe that we should first land a simplier module loader, then it will be easier to lower our compartments/sandbox uses. Finally, we have content scripts, used by page-mod, context-menu APIs. They are using a really different code path and they have different requirements. I think the issue your are seing on techcrunch is related to them. These scripts have to be executed with same principal than website and should not pollute web page context. So sandboxes appear as an obvious solution to use.

(In reply to comment #9) > I don't think that's related to page-mod API as it will be triggered only on > mozilla websites but I suspect context-menu API to add workers for each tab > (or even worse, all content documents?) > Drew: I think that you are the best one for such question about context-menu? context-menu creates workers for each pair of menu items and content windows.

(In reply to comment #9) > ehsan: your bugs doesn't make much sense as there is no differences between > internal and developer modules. Well, in that case feel free to dupe one against the other. :-)

(In reply to comment #9) > Nick: the explosion of compartments number on techcrunch only appears when > buzilla tweak is installed? Correct. Without Bugzilla Tweaks I get numbers like 10--20 compartments.

(In reply to comment #10) > context-menu creates workers for each pair of menu items and content windows. BTW, Bugzilla Tweaks should restrict the sites it adds context menu items to, and therefore the number of workers created, by using URLContext(), just as it does with its page mods by using `include`.

(In reply to comment #13) > (In reply to comment #10) > > context-menu creates workers for each pair of menu items and content windows. > > BTW, Bugzilla Tweaks should restrict the sites it adds context menu items to, > and therefore the number of workers created, by using URLContext(), just as it > does with its page mods by using `include`. IINM, it doesn't add context menu items to documents from other sites...

(In reply to comment #14) > IINM, it doesn't add context menu items to documents from other sites... It uses onBugzillaPage() in the urltest.js content script to test URLs in response to the "context" event, but because no URLContext() is specified, that test runs for any page when the context menu is invoked, regardless of its URL. If it used URLContext() instead, its content scripts would only run on pages that matched the given URL context, not all pages. So right now, when you invoke the context menu on TechCrunch with Bugzilla Tweaks installed, onBugzillaPage() runs, returns false, and so no Bugzilla Tweaks items are added to the menu. If it used URLContext() instead, no Bugzilla Tweaks code would run at all for TechCrunch, because no workers would be created for that page.

A tidbit from bug 672636 comment 1 (that bug has been marked as a dup of this bug): an empty (jetpack-based) add-on causes five extra system compartments to be created!

(In reply to comment #15) > (In reply to comment #14) > > IINM, it doesn't add context menu items to documents from other sites... > > It uses onBugzillaPage() in the urltest.js content script to test URLs in > response to the "context" event, but because no URLContext() is specified, > that test runs for any page when the context menu is invoked, regardless of > its URL. If it used URLContext() instead, its content scripts would only > run on pages that matched the given URL context, not all pages. So right > now, when you invoke the context menu on TechCrunch with Bugzilla Tweaks > installed, onBugzillaPage() runs, returns false, and so no Bugzilla Tweaks > items are added to the menu. If it used URLContext() instead, no Bugzilla > Tweaks code would run at all for TechCrunch, because no workers would be > created for that page. I tried to follow the docs and do that, but I couldn't make it work. Here's my patch: http://pastebin.mozilla.org/1282015

I haven't tested, but I don't think the match patterns in that patch are legal. This page describes them: https://addons.mozilla.org/en-US/developers/docs/sdk/1.0/packages/api-utils/docs/match-pattern.html It's a little tricky. Unfortunately URLContext doesn't currently support the regexp patterns described in that doc, but it should.

Alexandre, Irakli: This is an importatn bug. Bug 636145 sounds like it will help here, but also sounds difficult. The suggestion in comment 6 also sounds like a plausible fix. Can you comment on if one or both are necessary? And who would be a good person to work on the comment 6 suggestion, if it's necessary?

(In reply to comment #21) > Alexandre, Irakli: This is an importatn bug. Bug 636145 sounds like it > will help here, but also sounds difficult. The suggestion in comment 6 also > sounds like a plausible fix. Can you comment on if one or both are > necessary? And who would be a good person to work on the comment 6 > suggestion, if it's necessary? I don't think me or Alex are right persons to comment on whether or not suggestion in comment 6 it's necessary. It will for sure reduce amount of compartments that are created. Also please note that Bug 636145 is probably incompatible with comment 6 as it suggests creation of one compartment only in contrast to having per module compartments with different principals. Since comment 6 suggests a big architectural change I think it's best to discuss it with Myk Melez (Jetpack Tech lead) and Dave Mason (Jetpack project manager) to have a specific bug with high priority. Once we have a such a bug, I don't mind picking it up!

FWIW, what I care about is having modules protect their internal state (including their references to dependent modules that they've imported), so that when a reviewer sees main.js only do require("request"), they know that main.js won't be able to reach inside request.js and get full XPCOM authority. That's what will enable POLA and modular review. This includes preventing one module from changing the primordials (Object, Number, etc) of other modules. And it can't be too hard for module authors to do the right thing: what their usual style of expression is, we should adjust the platform/loader to let those modules be secure. We might accomplish that by getting module authors to use lexically-scoped state, and apply Caja-style restrictions (starting with ES5-Strict), including freezing the primordials. Or maybe we put each module in a separate compartment/sandbox/principal/etc and require authors to use other tools to indicate how they want sharing to happen. As I understand it, our current sandboxes give separate primordials to each module, so we can protect them from each other without freezing, and therefore switching to just-function-wrappers would also need to protect Object/Number somehow.

I've linked this bug to a module loader simplification work. My recent patch on that bug makes it possible to configure loader so that it either usese: - compartment per add-on modules. - compartment per module. Also this does not affects compartment creation by page-mods, context-menus etc..

Myk, you took assignment of this bug -- do you have plans for moving it forward?

njn: yes, my plan for moving it forward is to dig into the capabilities of the platform and figure out what we're able to do while waiting for compartments to become cheap. In particular, I'm going to investigate whether it's possible (or could be made possible) for sandboxes to share compartments, and I'm going to check if the function wrapper approach described in comment 6 achieves the degree of module isolation we want.

I took some time to measure sandbox memory usage with this xpcshell script that read resident memory (or malloc/allocated depending on Firefox version). Sandboxes memory cost is falling down on each new release: - Firefox 5.0 (20110705030204) 250k/sandbox - Firefox 6.0 (20110805030753) 171k/sandbox - Firefox 8.0 (20110808030804) Today's nightly 29k/sandbox If this script is right, a sandbox doesn't necessary cost 64KB at least. Then I imagine that we can continue working on lowering these numbers? I tried to use only one Sandbox for all modules by modifying Irakli's new module loader: https://github.com/mozilla/addon-sdk/pull/220 I end up saving around 2MB of resident memory on Firefox Nightly with Bugzilla tweak. This addon creates 76 additional compartments on firefox startup. 76*30 = 2280. This real world test match the sandbox memory measurement.

(In reply to Alexandre Poirot (:alex) from comment #27) > Created attachment 551489 [details] > xpcshell script to measure sandbox cost > > I took some time to measure sandbox memory usage with this xpcshell script > that read resident memory (or malloc/allocated depending on Firefox version). > > Sandboxes memory cost is falling down on each new release: > - Firefox 5.0 (20110705030204) > 250k/sandbox > - Firefox 6.0 (20110805030753) > 171k/sandbox > - Firefox 8.0 (20110808030804) Today's nightly > 29k/sandbox > > If this script is right, a sandbox doesn't necessary cost 64KB at least. > Then I imagine that we can continue working on lowering these numbers? A sandbox will cost 64KB once we JIT something inside it. > I tried to use only one Sandbox for all modules by modifying Irakli's new > module loader: https://github.com/mozilla/addon-sdk/pull/220 > I end up saving around 2MB of resident memory on Firefox Nightly with > Bugzilla tweak. This addon creates 76 additional compartments on firefox > startup. 76*30 = 2280. This real world test match the sandbox memory > measurement. How much memory does this save after using the browser for an hour or two?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28) > > A sandbox will cost 64KB once we JIT something inside it. Moving the JIT caches into the JSRuntime (as per comment 3) will avoid this cost. However, if the patch in bug 671702 lands that will make the JS heap quantum 64KB, up from the current 4KB. With Bugzilla Tweaks installed I see lots of compartment gc-heaps with sizes like 68KB, 76KB, 80KB, and some as small as 24KB.

(Pushing all open bugs to the --- milestone for the new triage system)

Note that right now we only jit code for a sandbox compartment if that code is in a function called from outside the sandbox. Code that runs during evalInSandbox is not jitted. See bug 695806.

Unassigning myself from bugs I am not actively working on.

This patch seems like it should work, but I don't see a reduction in the number of compartments after applying it. krizsa: am I using the API introduced by bug 677294 correctly?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #33) > krizsa: am I using the API introduced by bug 677294 correctly? I think there is a simpler way to use it here, but the problem is not on your side, I debugged this and turned out that there is a bug in my patch for bug 677294, I'll continue this comment there.

Myk: the fixed patch has been landed so now it should work. https://hg.mozilla.org/mozilla-central/rev/941801e7439c

Here's an updated patch that applies cleanly.

Here's the recent unhorked patch as an easily-mergeable pull request. Requesting review from ochameau instead, as gozala asked him to review the pull request for bug 680821, which this patch also addresses.

Comment on attachment 587197 [details] [diff] [review] patch v2 as a pull request It breaks tests using helpers.js and introduce many warning messages. Otherwise with this being fixed, it looks good. Is gabor's patch finally landed for sameGroupAs, because I wasn't able to see any memory usage difference ?

Comment on attachment 587197 [details] [diff] [review] patch v2 as a pull request (In reply to Alexandre Poirot (:ochameau) from comment #38) > It breaks tests using helpers.js and introduce many warning messages. I added two changes to the pull request that should resolve these issues. > Is gabor's patch finally landed for sameGroupAs, because I wasn't able to > see any memory usage difference ? Yes, it's landed. If you run the annotator example with the changes and load about:memory?verbose, you should see many fewer compartments being created for modules.

Also, if this patch gets your review, please merge it too! I'm in meetings this afternoon and might not get the chance before the merge to stabilization for the 1.5 release, and I'd like to get this in for 1.5 if possible.

Comment on attachment 587197 [details] [diff] [review] patch v2 as a pull request Works fine with very last nightly. If you test with annotator example, it goes from 156 compartments down to 8 :o Thanks gabor, thanks myk! Speaking about memory it looks like it allowed to dropped around 20MB for this particular example.

Commit pushed to https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/0b626f74512212955c6fef44a917f89c6b907000 Merge pull request #325 from mykmelez/bug-672443-reuse-compartments fix bug 672443 - reuse compartments between module sandboxes and set sandboxName to the module URI r=@ochameau

Nicolas, github robot automatically closed this bug. But feel free to reopen it or open more precise bugs if you think we should continue minimizing compartments number.