When writing frontend code, it's frequently necessary to trade off modular design (using small, self-contained JSMs) with low memory usage (each JSM uses a separate compartment). This is very frustrating. I just measured the number of JSMs we load to start up a browser and load a random web page. It's ~250 in the main process and ~90 in the content process. We've previously measured that each new compartment consumes ~75K of overhead [1]. So the memory isn't a huge issue, but it's significant. And it's going to become more important as we try to increase the number of content processes. I think it would be great if we could go back to using a single compartment for all JSMs (or at least for all non-addon JSMs). I think the goal should be to have these boundaries: zones are lifetime boundaries compartments are security boundaries JSMs are scope boundaries One way to implement this is similar to what we do for content scripts and ExecuteInGlobalAndReturnScope. We would have a single global for all JSMs. Each JSM would have its own NonSyntacticVariablesObject below that on the scope chain. We would always run JSM code with |this| set to the NonSyntacticVariablesObject (this is different than how content scripts work, but it seems doable). As I understand it, the main outcomes of this change would be: - JSMs would still be completely isolated from each other in terms of what variables they see. - Builtin properties on the global like Array would be shared. But since we don't do any monkeypatching in the frontend that I know of, this doesn't seem like it would be an issue. - WrappedNatives would be shared across JSMs, so a QI in one JSM would be visible to other JSMs. That's sorta weird but it seems tolerable. - Expandos would be shared across JSMs. (I don't actually remember if this is already true?) Similar to above, this seems like something we could deal with. - Memory reporters would no longer be able to distinguish JSMs. Not sure how big a deal that is. - We'd probably get worse JIT performance. Luke, can you advise about whether this idea makes sense? The JIT issue seems like the biggest concern. The use of NonSyntacticVariablesObject is a little strange since, unlike with ExecuteInGlobalAndReturnScope, the NonSyntacticVariablesObject will always be parented to the same global. So it's basically a normal scope except that we stick unqualified variable references there. Maybe we could use a different kind of object that still has some of the same properties but isn't deoptimized so much? [1] https://mail.mozilla.org/pipermail/firefox-dev/2015-April/002878.html

> Memory reporters would no longer be able to distinguish JSMs. Not sure how big a deal that is. I think the only context in which we end up discussing JSMs when looking at about:memory reports is marveling at how many there are.

Shouldn't we just wait for shu or whoever was working on it to finish ES6 modules and then just make JSMs (and indeed the current caller ExecuteInGlobalAndReturnScope) into modules in the ES6 sense? Unless the timeframe on that is too long, of course.

Jon, when do you thing ES6 modules will be implemented to the point where we can start using them in the browser?

(In reply to Bill McCloskey (:billm) from comment #3) There are some issues to work out first so I'd say not for a couple of months at least.

OK. This seems like something we could do sooner than that, so I think we should still look into it.

Yeah, using modules seems like the real goal state here. Since modules also share the same global scope, I would guess that moving to ExecuteInGlobalAndReturnScope is a step in the right direction in flushing out all the other issues you listed.

So how is this different than the compartment sharing stuff that we're doing on b2g today?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (UTC+8 July 17-25, expect delays) from comment #7) > So how is this different than the compartment sharing stuff that we're doing > on b2g today? I think the only difference is that unqualified vars (i.e., variables that are never declared) would go on the special scope object rather than the global. I guess we would also need to do some special stuff for Cu.import and loadSubScript to make this work properly. The b2g thing uses GetOutermostEnclosingFunctionOfScriptedCaller. I guess we could do a similar thing to find the enclosing JSScript and then map that to its scope. I'd like to avoid doing the trick of compiling these as functions since seems kind of weird.

In general I am all for something similar but less hacky :D

Shu, it looks like we now have the invariant that a StaticNonSyntacticScopeObjects represents either 0+ non-syntactic DynamicWithObjects or a single NonSyntacticVariablesObject. Would it be okay to change this so that a StaticNonSyntacticScopeObjects can also represent 0+ DynamicWithObjects followed by a single NonSyntacticVariablesObject (where the variables object is closer to the global)? I don't see any obvious places where we rely on it being "either but not both". I don't understand the debugger stuff though.

(In reply to Bill McCloskey (:billm) from comment #0) > I just measured the number of JSMs we load to start up a browser and load a > random web page. It's ~250 in the main process and ~90 in the content > process. We've previously measured that each new compartment consumes ~75K > of overhead [1]. So the memory isn't a huge issue, but it's significant. And > it's going to become more important as we try to increase the number of > content processes. Have we made any progress in figuring out where that overhead is coming from? I'm not necessarily opposed to this, but I'd much rather reduce the memory footprint of a compartment to something more negligible, since that's a win that would translate to the web as well. See bug 989373 comment 23. Nick, have we made any progress in our understanding here?

(In reply to Bill McCloskey (:billm) from comment #10) > Shu, it looks like we now have the invariant that a > StaticNonSyntacticScopeObjects represents either 0+ non-syntactic > DynamicWithObjects or a single NonSyntacticVariablesObject. > > Would it be okay to change this so that a StaticNonSyntacticScopeObjects can > also represent 0+ DynamicWithObjects followed by a single > NonSyntacticVariablesObject (where the variables object is closer to the > global)? > > I don't see any obvious places where we rely on it being "either but not > both". I don't understand the debugger stuff though. The Debugger doesn't depend on this invariant AFAIK. I think it's fine to change the invariant to 0+ non-syntactic DynamicWiths followed by 1? NonSyntacticVariablesObject.

> See bug 989373 comment 23. Nick, have we made any progress in our > understanding here? No. Right now my level of understanding is basically "lots of small things that add up", and I don't even understand what all of those small things are.

Here's a WIP patch. It seems to work pretty well, but I need to fix a few things and I'm not sure when I'll get a chance.

I talked to billm about this, and he said in his local measurements it saved 15-20MB (but not too much in content processes).

This is a more recent version of the patch. It's based on an m-c revision from July 22 in case it helps in rebasing. I'll comment with some explanations.

Comment on attachment 8660182 [details] [diff] [review] patch v2 Review of attachment 8660182 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/sdk/base64.js @@ +14,5 @@ > // (devtools loader injects these symbols as global and prevent using > // const here) > +let scope = Cu.import("resource://gre/modules/Services.jsm", {}); > +let global = Cu.getGlobalForObject(scope); > +let { atob, btoa } = global; atob/btoa are not defined by Services.jsm. They're included on every BackstagePass, though. Even though they're not part of EXPORTED_SYMBOLS, Cu.import returns the full JSM global and so we can access them here. ::: addon-sdk/source/lib/toolkit/loader.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > ;((factory) => { // Module boilerplate :( > + let Cu = Components ? Components["utils"] : null; This isn't needed now (see later about loader.js). ::: browser/components/sessionstore/content/content-sessionStore.js @@ +10,5 @@ > let Cr = Components.results; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm", this); > Cu.import("resource://gre/modules/Timer.jsm", this); > +Cu.import("resource://gre/modules/Services.jsm", this); Lots of frame scripts assume that stuff imported by Cu.import goes on the shared frame script global, so they rely on other scripts to Cu.import stuff for them. This patch makes Cu.import add stuff to the module scope, so the sharing no longer works. ::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm @@ +613,5 @@ > } > > RangedChromeActions.prototype = Object.create(ChromeActions.prototype); > var proto = RangedChromeActions.prototype; > + Object.defineProperty(proto, "constructor", {value: RangedChromeActions}); The changes in this file probably are no longer needed. ::: js/xpconnect/loader/mozJSComponentLoader.cpp @@ +492,5 @@ > + bool reuseGlobal = !aAddonId; > + > + nsCString spec; > + aURI->GetSpec(spec); > + if (spec.EqualsASCII("resource://gre/modules/commonjs/toolkit/loader.js")) These cutouts are the worst part of this patch. We might be able to add some kind of API so we can do this from JS instead of C++, but it's still bad. The loader.js is tricky because it calls Object.freeze on things like Object and Object.prototype: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#92 With this patch, all JSMs share the same copies of Object, Array, etc. We don't do any monkeypatching in Firefox as far as I know, so this isn't bad in itself. The problem happens for this reason. Let's say you want to set foo.constructor = bar where foo is some prototype object. Normally that works fine and adds a property directly on foo. However, foo's proto is Object.prototype, which also has a constructor property. This property is now frozen. For some reason, ES semantics say that if foo's proto has a frozen property, you can't set that property on foo, even if foo itself is not frozen. I started to fix this by converting assignments to such properties to Object.defineProperty instead, since this has its own rules. However, I eventually decided that this was really gross and loader.js should not be freezing shared stuff. So I decided to move it out of the shared global. @@ +494,5 @@ > + nsCString spec; > + aURI->GetSpec(spec); > + if (spec.EqualsASCII("resource://gre/modules/commonjs/toolkit/loader.js")) > + reuseGlobal = false; > + else if (spec.EqualsASCII("resource://gre/modules/jsdebugger.jsm")) This is necessary because of the following common pattern: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js#22 We have a function that adds the builtin Debugger property to a global. We load a JSM and then try to add Debugger to that global. With this patch, the JSM is no longer a global, so we can't add Debugger to it. This is all for test case, so I just got frustrated and made jsdebugger.jsm be its own global. Probably a better thing to do here is to convert such tests to call a function that makes a Sandbox global and adds Debugger to that instead. ::: toolkit/components/search/nsSearchService.js @@ -220,5 @@ > function isUsefulLine(aLine) { > return !(/^\s*($|#)/i.test(aLine)); > } > > -this.__defineGetter__("FileUtils", function() { BackstagePass has __defineGetter__ on it but the NonSyntacticVariablesObject doesn't. It's deprecated anyway, so I replaced them all with Object.defineProperty.

Bill, what's the status of this? It sounds like it might be a good step along the way to replacing JSM modules with ES modules.

This was mostly working at the time, but it was pretty gross. There are just some horrible hacks that some code does and it was difficult to work around them. I think a more incremental approach, like the one proposed in bug 1305669, would be better. Some of the code in the patch here might be useful as a starting point, but landing it as-is wouldn't necessarily help much with bug 1305669.

I've started rebasing this patch. I'm mostly just trying to figure out how the scope and environment stuff needs to be changed in light of the rewrite Shu did recently.

The current problem I need to fix is that if |this| is not explicitly defined for a function, it should use the NonSyntacticVariablesObject instead of the global. This seems to happen in js::BoxNonStrictThis().

I managed to get this at least sort of working (I'm able to start up the browser without any error messages). The memory win is less than I was seeing before with the b2g merging: around 720kb saved in the content process, and 16mb saved in the parent process. The content process savings isn't too impressive, but the parent process amount is pretty good. I suspect I may have been seeing more with the b2g merging because scripts weren't fixed up properly, so things throw a lot of errors, and so we simply don't load as many scripts. I hacked up some patches to fix up the b2g merging a little in the content process, and the savings were about what I'm seeing for this approach.

I figured out part of why global merging does not improve memory usage by much in the content process. Bug 1303754 changed setSourceIsLazy to false in the content process, which enables lazy parsing (when we can do that is determined by BytecodeCompiler::canLazilyParse()), which Jan found saved 1mb+ in the child process. This patch adds a non-syntactic variables object onto the scope chain, which causes canLazilyParse() to return false, so we lose out on the 1mb. Presumably the actual memory gain is like 1.3MB or so, but much of that is cancelled out. The parent process doesn't use lazy parsing, so it is all win. The parent process has 6 times as many compartments, so if it is saving 16mb, we might expect to save 2.6mb in the child process. I'm not sure if there's another 1mb we should be saving somewhere.

On advice of Shu, I just removed the line from canLazilyParse() for non syntactic scopes, and it at least works well enough to load a simple page without any noticable errors. With that, content process memory usage was reduced by 2.75mb, which is in line with what I'd expect.

I split out the removal of the old B2G merging code into bug 989373. This is rebased on top of that. I've also split out some of the frontend fixes that are needed, but they aren't all here yet.

Just to add another data point for this: Since bug 1364974, we pre-compile all module scripts into xpc::CompilationScope, and then clone them into global of new module when it's created. This winds up being much faster than XDR decoding, but still much slower than not having to clone at all. If we're loading all modules into the same global, we can compile for the shared module global instead, and avoid the clone altogether (as long as we either don't need to compile for non-syntactic scopes, or initially compile the scripts for non-syntactic scopes rather than global scopes). Also, the script preloader patches disabled lazy parsing and enabled lazy source in both parent and child processes, so if we were previously saving any memory from that, it should already no longer be the case. But my medium/long-term thoughts were that, because we share the original mmapped XDR data for pre-loaded module scripts across all processes, we should be able to reuse a slice of that original data whenever we re-lazify module functions, which means that there wouldn't be any significant per-process overhead for functions which aren't used very often.

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #29) > Also, the script preloader patches disabled lazy parsing and enabled lazy > source in both parent and child processes, so if we were previously saving > any memory from that, it should already no longer be the case. Hmm that seems bad. If that was in the patches I reviewed, I guess I didn't noice it. It was saving a few megs per content process (it was already disabled in the parent process). Kind of odd it wouldn't show up on AWSY.

(In reply to Andrew McCreight [:mccr8] from comment #30) > (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment > #29) > > Also, the script preloader patches disabled lazy parsing and enabled lazy > > source in both parent and child processes, so if we were previously saving > > any memory from that, it should already no longer be the case. > > Hmm that seems bad. If that was in the patches I reviewed, I guess I didn't > noice it. It was saving a few megs per content process (it was already > disabled in the parent process). Kind of odd it wouldn't show up on AWSY. It was disabled as a side-effect of caching being enabled, since we enable lazy source only when we're not using the cache. Given that we didn't see any AWSY regressions, something may have changed in the mean time, or we may see smaller memory wins from lazy parsing when we're using the bytecode cache.

I've been fixing test failures over the last week or two. I think it should be green now, at least on Linux. I haven't tried other platforms. Most of the changes were tweaks to how JSMs were being imported and the like, which mostly required running the test and looking for JS error messages, but two issues were more complex to investigate. My code for evaluating JSOP_FUNCTIONTHIS |this| in a non-syntactic scope was returning the |this| from the innermost dynamic lexical environment, rather than from the NSVO. This was breaking an XPConnect test, test_bug993423.html, which runs an inner function inside an XBL event handler to get the value of |this|. I should write some more explicit tests for this issue, as it is mostly luck that a test existed that caught this issue. My current fix also feels a little hacky. test_exceptionSanitization.html (and another similar DOM bindings test) was failing, but only with e10s. This test checks that an exception thrown by chrome code to content gets caught and replaced with a clean new exception. It turns out that there's a method Cu.forcePermissiveCOWs() that testing code can use to basically disable security wrappers. This works by setting a flag on the compartment, and gets loaded in a couple of testing JSMs. If those testing JSMs are merged in with the rest, then these wrappers are disabled everywhere, and the test fails. This only happens with e10s because I think these JSMs are only loaded for e10s. This is easily fixed by blacklisting those JSMs from being merged, but I should also add an assertion that we never set this testing flag on the shared JSM global. Bill suggested that I add some similar asserts for other things that should never get merged into the shared global. I did an AWSY run, which showed a memory improvement of about 10MB. This is all without lazy parsing. I need to investigate what happens with that next, both in terms of test failures and memory usage. The caching stuff kmag added recently may affect what happens in terms of memory.

Andrew, this is great to see! I was curious, have you tried profiling these builds? One of the things that I'm hoping this change to achieve is eliminating cross-compartment wrappers between the objects imported from JSMs. It would be great to try to profile some chrome-JS heavy test with this change to see if it has any measurable impact on performance too. mconley and florian can probably help think of a good test case. I'm thinking maybe startup on the reference hardware?

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #33) > One of the things > that I'm hoping this change to achieve is eliminating cross-compartment > wrappers between the objects imported from JSMs. Yes, it should do that. I'm not sure how much of an impact this will have. Chrome JS doesn't even get Ion compiled most of the time so I'm not sure how much time is spent in CCWs. This patch may actually worsen perf by preventing Ion compilation entirely. But yes, I should see what affect there is.

I know it's anecdotal but while profiling the new l10n API we're preparing for post Quantum landing, :smaug pointed out today that we are doing some cross compartment back and forth since we introduce 4 jsms. I can bundle them into one but that would reduce the code maintainability so I hope for this bug to remove the issue for me :) On a related note - is there a way to check if my jsm jits?

I don't know how much I believe it, but according to a try run I did, this makes tp5o responsiveness opt e10s more than 30% better. ;) I don't know if that is meaningful. tp5o opt e10s was flat. AWSY was about 10mb better, as before, though this time I had lazy parsing enabled for nonsyntactic scopes. https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a49112c7a576&newProject=try&newRevision=9f93aef69dfe5f7ceb52d92ffad0595cffc98507&framework=1&showOnlyImportant=0

(In reply to Andrew McCreight [:mccr8] from comment #36) > I don't know how much I believe it, but according to a try run I did, this > makes tp5o responsiveness opt e10s more than 30% better. ;) I don't know if > that is meaningful. Given the number of stacks I've seen with multiple JIT fallbacks triggered by cross-compartment wrapper access, it wouldn't entirely surprise me. Assuming that the use of non-syntactic scopes doesn't completely destroy the JITtability of most of our chrome code, anyway.

In some local testing, main process memory went down by a little over 8mb, and a single child process went down by about 4.7mb, so for 4 child processes I'd expect a bit more of a reduction, but I'm not sure what the discrepancy is.

Try looks fairly green. I think the oranges are known failures. With pref disabled (as it is in this bug): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c29a7bade3ae92c291b2f3bc3962cbe9a6e892&filter-tier=1 With pref enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab19acfcc69fe659a976ec2e0c0d4ed93c2fd464&filter-tier=1

Jason, I put you as an additional reviewer because tcampbell is not a peer, and you are looking at compartment sharing, which is at least somewhat related. Thanks. In case anybody is looking at all of these patches, "Add the ability to use NSVOs on the scope chain in place of globals" contain the JS engine changes needed, while "Use a single global for all JSMs" implements a pref (disabled in these patches) to load JSMs into a shared global. The rest of the patches are test fixes, or adding assertions.

Kris, is it easy to test for add-ons manager startup time regressions with this patch? I'm worried that NSVOs will block Ion compile and you observed measurable impact when we added |super| support to Ion in https://bugzilla.mozilla.org/show_bug.cgi?id=1169745#c30

(In reply to Ted Campbell [:tcampbell] from comment #53) > Kris, is it easy to test for add-ons manager startup time regressions with > this patch? I'm worried that NSVOs will block Ion compile and you observed > measurable impact when we added |super| support to Ion in > https://bugzilla.mozilla.org/show_bug.cgi?id=1169745#c30 Not Ion, just baseline. But regressions there should show up in talos ts_paint tests. I'm a bit worried about that too, but it's possible that worse JIT performance will be offset by fewer cross-compartment wrappers. In most of the profiles I've looked at, we spend huge amounts of time creating cross-compartment wrappers, and going through proxies rather than directly accessing objects. But in one quick test I did to merge all WebExtension JSMs into a single global using the subscript loader and plain context objects, it caused pretty serious performance regressions, so I abandoned it. Maybe these patches will handle it better, though.

Andres, can you do a complete talos run with 5 rebuilds on the latest versions of these patches? The tp5o numbers in comment 36 looked good, but I'm still worried about the possibility of regressing ts_paint, given what I've seen when using the subscript loader with object scopes in the past.

Sure thing. Yeah, that's something I'm concerned about, too, I just didn't know what tests to run. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5d4ddb0697078e794f2a741bccbfb6dd1fe37fc

Comment on attachment 8885493 [details] Bug 1186409 - Fix Addon SDK JSM context detection. https://reviewboard.mozilla.org/r/156356/#review176490

(In reply to Andrew McCreight [:mccr8] from comment #56) > Sure thing. Yeah, that's something I'm concerned about, too, I just didn't > know what tests to run. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=d5d4ddb0697078e794f2a741bccbfb6dd1fe37fc This run is with the sharedGlobal pref set to false :) Can you do a run with it set to true? Also, it's usually worth doing linux64 runs, since they're cheaper and we do more of them. And adding `--rebuild-talos 5` so we get enough data points.

(In reply to Kris Maglione [:kmag] from comment #58) > (In reply to Andrew McCreight [:mccr8] from comment #56) > > Sure thing. Yeah, that's something I'm concerned about, too, I just didn't > > know what tests to run. > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=d5d4ddb0697078e794f2a741bccbfb6dd1fe37fc > > This run is with the sharedGlobal pref set to false :) Can you do a run with > it set to true? > > Also, it's usually worth doing linux64 runs, since they're cheaper and we do > more of them. And adding `--rebuild-talos 5` so we get enough data points. Yeah, oops, I realized both of those things after I did the push. Here's a newer push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aec30e0a79ef2f58fa419e4ef9819804a3995bb&selectedJob=125216028 You should double check me because I'm not super familiar with Talos stuff, but it looks like it is 9.53% better. ts_paint_webext is 17.6% better.

(In reply to Andrew McCreight [:mccr8] from comment #59) > You should double check me because I'm not super familiar with Talos stuff, > but it looks like it is 9.53% better. ts_paint_webext is 17.6% better. Excellent. I agree, the results so far look extremely promising. I'll get to the reviews today.

Comment on attachment 8885492 [details] Bug 1186409 - Fix code that tries to get the global by using |this|. https://reviewboard.mozilla.org/r/156354/#review177010

Comment on attachment 8885494 [details] Bug 1186409 - Use Cu.getGlobalForObject when importing properties off a JSM global. https://reviewboard.mozilla.org/r/156358/#review177012 ::: addon-sdk/source/lib/sdk/base64.js:16 (Diff revision 2) > const { Cu } = require("chrome"); > > // Passing an empty object as second argument to avoid scope's pollution > // (devtools loader injects these symbols as global and prevent using > // const here) > -var { atob, btoa } = Cu.import("resource://gre/modules/Services.jsm", {}); > +var { atob, btoa } = Cu.getGlobalForObject(Cu.import("resource://gre/modules/Services.jsm", {})); Can we use `Cu.importGlobalProperties` for this? ::: addon-sdk/source/test/addons/places/lib/favicon-helpers.js:12 (Diff revision 2) > const loader = Loader(module); > const httpd = loader.require('./httpd'); > const { pathFor } = require('sdk/system'); > const { startServerAsync } = httpd; > const basePath = pathFor('ProfD'); > -const { atob } = Cu.import("resource://gre/modules/Services.jsm", {}); > +const { atob } = Cu.getGlobalForObject(Cu.import("resource://gre/modules/Services.jsm", {})); And this (even though it's dead code)

(In reply to Kris Maglione [:kmag] from comment #60) > Excellent. I agree, the results so far look extremely promising. Heck yeah they do: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=4aec30e0a79e&framework=1&selectedTimeRange=172800

Comment on attachment 8885496 [details] Bug 1186409 - Check to make sure we don't set some weird XPConnect flags on the shared global. https://reviewboard.mozilla.org/r/156362/#review177014 ::: js/xpconnect/src/XPCComponents.cpp:2885 (Diff revision 2) > { > if (!obj.isObject()) > return NS_ERROR_INVALID_ARG; > > JSObject* scopeObj = js::UncheckedUnwrap(&obj.toObject()); > + MOZ_ASSERT(!mozJSComponentLoader::Get()->IsLoaderGlobal(scopeObj), Can we make this a diagnostic assert? ::: js/xpconnect/src/XPCComponents.cpp:2931 (Diff revision 2) > + MOZ_ASSERT(!mozJSComponentLoader::Get()->IsLoaderGlobal(currentGlobal), > + "Don't call Cu.forcePermissiveCOWs() in a JSM that shares its global"); Diagnostic assert here, too. And probably also for SetWantXrays?

Comment on attachment 8899618 [details] Bug 1186409 - Use a single global for all JSMs. https://reviewboard.mozilla.org/r/170920/#review177016 ::: js/xpconnect/loader/mozJSComponentLoader.cpp:523 (Diff revision 1) > aGlobal.set(global); > } > > +bool > +mozJSComponentLoader::ReuseGlobal(bool aIsAddon, > + nsIURI* aURI) Nit: Fits on previous line. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:529 (Diff revision 1) > +{ > + if (aIsAddon || !mShareLoaderGlobal) > + return false; > + > + nsCString spec; > + aURI->GetSpec(spec); This can theoretically fail. Probably best to return false if it does rather than true. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:820 (Diff revision 1) > + if (!JS_IsGlobalObject(obj)) { > + if (!scopeChain.append(obj)) > + return NS_ERROR_FAILURE; > + } > JS::RootedValue rval(cx); > - if (!JS::CloneAndExecuteScript(aescx, script, &rval)) { > + if (!JS::CloneAndExecuteScript(aescx, scopeChain, script, &rval)) { We should only call the `scopeChain` version of `CloneAndExecuteScript` when we have a NSVO, or we're going to wind up non-syntactic compilation even when it's not necessary. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:854 (Diff revision 1) > + dom::AutoJSAPI jsapi; > + jsapi.Init(); > + JSContext* cx = jsapi.cx(); > + RootedObject global(cx, mLoaderGlobal); > + JSAutoCompartment ac(cx, global); > + if (JS_HasExtensibleLexicalEnvironment(global)) { Probably may as well make this an assert. This should be guaranteed for global objects. ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:194 (Diff revision 1) > if (!envChain.append(targetObj)) { > return false; > } > + if (loadScope != targetObj && > + !JS_IsGlobalObject(loadScope) && > + js::GetObjectCompartment(loadScope) == js::GetObjectCompartment(targetObj)) I'd be a lot more comfortable if we did the same-compartment check in `LoadSubScriptWithOptions` and nulled it out if the compartments aren't the same. It's too easy to imagine someone misunderstanding its purpose down the road, and us ending up with a cross-compartment scope chain. Also, some comments about the purpose of the parameter and the member variables would be helpful. What we're doing here is pretty subtle... ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:589 (Diff revision 1) > + targetObj = options.target; > + else > + targetObj = loadScope; > > targetObj = JS_FindCompilationScope(cx, targetObj); > - if (!targetObj) > + loadScope = JS_FindCompilationScope(cx, loadScope); The `JS_FindCompilationScope` shouldn't be necessary for `loadScope`. It's only really useful when we may have a cross-compartment object, and we want to unwrap it to the appropriate global or non-wrapper object for compilation.

(In reply to Kris Maglione [:kmag] from comment #62) > Can we use `Cu.importGlobalProperties` for this? Hmm I think there was some reason for doing it this way, but maybe not. I can try it and see if anything breaks. For some code, you can't use it because there's a lint rule that blocks the use of importGlobalProperties but that doesn't seem to apply to this code ( http://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-importGlobalProperties.js ).

(In reply to Andrew McCreight [:mccr8] from comment #66) > (In reply to Kris Maglione [:kmag] from comment #62) > > Can we use `Cu.importGlobalProperties` for this? > > Hmm I think there was some reason for doing it this way, but maybe not. I > can try it and see if anything breaks. For some code, you can't use it > because there's a lint rule that blocks the use of importGlobalProperties > but that doesn't seem to apply to this code ( > http://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin- > mozilla/lib/rules/reject-importGlobalProperties.js ). Hm. Interesting. It looks like it's mostly used by devtools, but I wonder why they consider it dangerous. I guess maybe because they share a single global for all of their modules, and it adds properties to the global rather than the current non-syntactic scope?

(In reply to Kris Maglione [:kmag] from comment #67) > (In reply to Andrew McCreight [:mccr8] from comment #66) > > (In reply to Kris Maglione [:kmag] from comment #62) > > > Can we use `Cu.importGlobalProperties` for this? > > > > Hmm I think there was some reason for doing it this way, but maybe not. I > > can try it and see if anything breaks. For some code, you can't use it > > because there's a lint rule that blocks the use of importGlobalProperties > > but that doesn't seem to apply to this code ( > > http://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin- > > mozilla/lib/rules/reject-importGlobalProperties.js ). > > Hm. Interesting. It looks like it's mostly used by devtools, but I wonder > why they consider it dangerous. I guess maybe because they share a single > global for all of their modules, and it adds properties to the global rather > than the current non-syntactic scope? Right, it led to confusing behavior for DevTools since `importGlobalProperties` would affect all modules after it runs. Modules in DevTools generally behave like they are independent, but this would break through the independence causing surprising behavior. See bug 1223452 for more.

jorendorff and tcampbell were asking why I needed the FUNCTIONTHIS changes in Interpreter.cpp. I backed out those changes, and looked for failures. Here's one of them, when running the XPCShell test services/common/tests/unit/test_blocklist_certificates.js. This is a deminified version of some code from kinto-offline-client.js (with irrelevant code elided with ...): (function(f){ if (...) { ... } else { var g; if (...) { ... } else { // a) Without global sharing, |this| is the global. OK! // b) With global sharing and my Interpreter.cpp changes, this is the NSVO (I think). OK! // c) With global sharing, but without my Interpreter.cpp changes, |this| is the global. FAILS! // So, in case c, we set a property on the global, but later try to get the Kinto field // of the NSVO and it fails. dump(this); g = this; } g.Kinto = f() } })(function(){});

tcampbell noticed a bug in the loader patch. He said the __URI__ for the first JSM that is loaded is "shared JSM global". I think the problem is that globalLocation is a reference, so when we set it to "shared JSM global", it overwrites nativePath.

Comment on attachment 8885495 [details] Bug 1186409 - Add the ability to use NSVOs on the scope chain in place of globals. https://reviewboard.mozilla.org/r/156360/#review177446 Clearing review for now since we need to iterate on this a bit. Overall, it looks like problems you are bumping in to are more JS side and should be factored out to different bugs. Thanks for tracking down all these edge cases :) I've opened bug 1394490 to handle the bug you noticed with JSOP_FUNCTIONTHIS to ensure we cover JIT updates as well. We hopefully don't need the ISTHIS_SLOT changes, and I'm looking into where we get off-track in js engine. The global |this| should Just Work(TM) with NVSO as they are, so we should track that down as a seperate bug. ::: js/src/vm/EnvironmentObject.h:588 (Diff revision 2) > +// > +// If |isThis| is true, then the NSVO object is also used as the |this| object, > +// instead of the global. > class NonSyntacticVariablesObject : public EnvironmentObject > { > + static const unsigned IS_THIS_SLOT = 1; This seems like we are going down the wrong path.

(In reply to Andrew McCreight [:mccr8] from comment #70) > tcampbell noticed a bug in the loader patch. He said the __URI__ for the > first JSM that is loaded is "shared JSM global". I think the problem is that > globalLocation is a reference, so when we set it to "shared JSM global", it > overwrites nativePath. That should be fixed by the patches for bug 1381976.

(In reply to Ted Campbell [:tcampbell] from comment #71) > Clearing review for now since we need to iterate on this a bit. Overall, it > looks like problems you are bumping in to are more JS side and should be > factored out to different bugs. Thanks for tracking down all these edge > cases :) > > I've opened bug 1394490 to handle the bug you noticed with JSOP_FUNCTIONTHIS > to ensure we cover JIT updates as well. > > We hopefully don't need the ISTHIS_SLOT changes, and I'm looking into where > we get off-track in js engine. The global |this| should Just Work(TM) with > NVSO as they are, so we should track that down as a seperate bug. I got a mostly green try run with a quick patch to force JSM/component scripts into strict mode (and some of the issues it found are actual bugs), so I'll try to have a patch for that later today. If that sticks, we won't need to block this on non-strict `this` boxing issues.

(In reply to Kris Maglione [:kmag] from comment #62) > Can we use `Cu.importGlobalProperties` for this? I checked and things are still green with importGlobalProperties, so I've updated both places to use it.

(In reply to Kris Maglione [:kmag] from comment #65) > Also, some comments about the purpose of the parameter and the member > variables would be helpful. What we're doing here is pretty subtle... I'm afraid I don't really know what this code is doing. The subscript loader code is all from Bill's initial patch, and it all just worked so I never had to investigate how it works. I think I can make the compartment check change you suggest, but you'll have to be the one who writes up the comments on what exactly this is doing. I'll try to read over it more to see if I can figure it out.

(In reply to Kris Maglione [:kmag] from comment #65) > I'd be a lot more comfortable if we did the same-compartment check in > `LoadSubScriptWithOptions` and nulled it out if the compartments aren't the > same. Do you mean that it should just get checked at the start of DoLoadSubScriptWithOptions before we null check targetObj and loadScope, so we'd just return NS_ERROR_FAILURE?

Here is the overview of the Spidermonkey side of things. I summarize the current state of our loaders interaction with environments as follows: > | FrameScript | JSM** | Subscript | XBL > -------------+---------------|---------|-------------|-------- > a = 1; | NSVO | NSVO | global | global > var b = 2; | NSVO | NSVO | WEO | WEO > let c = 3; | NSLEO | NSLEO | NSLEO* | NSLEO* > this.d = 4; | global*** | NSVO | WEO | WEO > > NSVO NonSyntacticVariablesObject > WEO WithEnvironmentObject > NSLEO LexicalEnvironmentObject[NonSyntactic] > > * Shared between subscripts with same target > ** Proposed JSM changes in this bug. Currently is private global. > *** https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Frame_script_environment As Andrew noticed in his work above, the behavior of JSOP_FUNCTIONTHIS in non-strict mode is inconsistent when using NSVOs. I have opened Bug 1394490 against JS to fix. Additionally, Kris has opened Bug 1394556 to fix our JSMs to not-rely on this non-strict behavior to begin with. Either of these needs to land for JSMs to share globals. Looking at the chart suggests we should strongly consider FrameScript and JSM loading to take same approach. For JSMs, I would like to use |ExecuteInGlobalAndReturnScope| instead of the current CloneAndExecuteScript. This also removes the need to for patching around WithEnvironment issues that :mccr8 needed to do in these patches. The convention difference between FrameScript and JSM handling of |this| needs to be addressed in that case. We can either change JSAPI so that there is a flag to select between the two mode, or as Kris proposed we can change the few FrameScripts that use global |this| to use another name to access the shared global state. It is probably preferred to change the FrameScript conventions such that |this.a| and |var a| are consistent and be explicit about shared context. I will put together a try run to see if using ExecuteInGlobalAndReturnScope can pass tests.

(In reply to Andrew McCreight [:mccr8] from comment #75) > (In reply to Kris Maglione [:kmag] from comment #65) > > Also, some comments about the purpose of the parameter and the member > > variables would be helpful. What we're doing here is pretty subtle... > > I'm afraid I don't really know what this code is doing. The subscript loader > code is all from Bill's initial patch, and it all just worked so I never had > to investigate how it works. I think I can make the compartment check change > you suggest, but you'll have to be the one who writes up the comments on > what exactly this is doing. I'll try to read over it more to see if I can > figure it out. So, essentially, when we call loadSubScript from a Cu.import module, we want it to behave the same way now as it did before. Previously, the loaded script got access to any non-lexical variables defined on the JSM by default, since they were defined on the global, and scripts executed in non-lexical scope chains get access to the global by default. When the subscript loader is used in a JSM, the scripts still need access to top-level, non-lexically-scoped variables from that JSM, so we need to include its NSVO in the scope chain when we load a script in a scope object. The trick, though, is that it's possible to pass a scope object to loadSubScript that's in a different compartment, and in that case, we *do not* want to include the NSVO for the module in the scope chain. Aside from producing unexpected behavior, it would almost certainly cause crashes. Granted... previously, it would be possible to pass a scope object that belonged to a different JSM, and have the subscript get access to both that scope object and the globals for the other JSM, which won't be possible now. But I don't think we have any code that relies on that. The issue I have with the current code is that we pass the NSVO around a lot, and use it in a place far removed from all of the scoping logic, even in the case where we *don't* want to use it in the scope chain, because it belongs to a different compartment than the target object. This seems like asking for trouble. (In reply to Andrew McCreight [:mccr8] from comment #76) > Do you mean that it should just get checked at the start of > DoLoadSubScriptWithOptions before we null check targetObj and loadScope, Yes. Or something along those lines. > so we'd just return NS_ERROR_FAILURE? No, we shouldn't return NS_ERROR_FAILURE, but we probably should do something like null out the variable, and pass around a nullptr rather than a potential cross-compartment footgun. (In reply to Ted Campbell [:tcampbell] from comment #77) > Looking at the chart suggests we should strongly consider FrameScript and > JSM loading to take same approach. For JSMs, I would like to use > |ExecuteInGlobalAndReturnScope| instead of the current > CloneAndExecuteScript. This also removes the need to for patching around > WithEnvironment issues that :mccr8 needed to do in these patches. I don't think that will work. The reason (I think) we went with a different approach here is that we need access to the NSVO before we run scripts in it, so that we can set certain properties (__URI__, __LOCATION__), and store it in the module import map for any Cu.import calls that happen while the module script is being executed. If we used ExecuteInGlobalAndReturnScope, we wouldn't be able to do either of those things. We also need |loadSubScript(url, this)| from a JSM to load the subscript into the NSVO, and behave the same way as the main module script. Without the scope chain changes for handling of NSVOs, that won't happen. That said, I really would like to see consistent APIs and behaviors for both JSMs and frame scripts, so if we keep one API, I think it needs to be the CloneAndExecuteScript version.

Comment on attachment 8885495 [details] Bug 1186409 - Add the ability to use NSVOs on the scope chain in place of globals. https://reviewboard.mozilla.org/r/156360/#review179534

Comment on attachment 8885495 [details] Bug 1186409 - Add the ability to use NSVOs on the scope chain in place of globals. Oops, I'm not sure why these got set again.

I addressed kmag's review comments, and removed the FUNCTIONTHIS changes, but the IS_THIS_SLOT stuff is still needed, so I did not re-request review. I also need to fix the __URI__ issue from comment 70. (I know kmag's other patch fixes it, but I might as well land a non-broken patch.)

Oh, and the non_syntactic.js test changes likely need to be altered, as I didn't actually fix non-strict function this.

Ouch, that did not work out like I'd hoped. Anyways, the __URI__ thing is fixed now.

(In reply to Andrew McCreight [:mccr8] from comment #86) > I addressed kmag's review comments, and removed the FUNCTIONTHIS changes, > but the IS_THIS_SLOT stuff is still needed, so I did not re-request review. Can we go with a short-term solution for now, until bug 1394490 lands? I'd like to get this landed as soon as possible so it has a bit of time to stabilize before beta. And so much of the performance work I have left to do is heavily tied to module initialization and cross-compartment wrappers, so it's hard to know what to focus on while this patch is still pending.

(In reply to Bill McCloskey (:billm) from comment #0) > As I understand it, the main outcomes of this change would be: > - (...) > - Builtin properties on the global like Array would be shared. But since we > don't do any monkeypatching in the frontend that I know of, this doesn't > seem like it would be an issue. One potential issue that can arise with shared mutable builtins is that if an attacker gets to evaluate code in one JSM, they get to change builtins used in all JSMs at once (and escalate). One solution would be to freeze the shared builtins at startup so no one can mutate them. The only known practical issue with this solution is that it's not possible to assign `toString` to an object dynamically any longer. It's possible to work around by not freezing but making the builtins "tamper-proof" [1] (turning data properties into accessor that act the same but don't have the toString limitation mentioned above) [1] https://github.com/google/caja/blob/71697dd9593f0ead26fac0781e1055b6e8313709/src/com/google/caja/ses/repairES5.js#L389-L452

(In reply to David Bruant from comment #96) > One potential issue that can arise with shared mutable builtins is that if > an attacker gets to evaluate code in one JSM, they get to change builtins > used in all JSMs at once (and escalate). I don't understand how this is an issue. JSMs all run with chrome privilege, so if you can run arbitrary code in a JSM, it is already game over.

(In reply to Andrew McCreight [:mccr8] from comment #97) > JSMs all run with chrome privilege, I guess that's a problem of its own. Why would the Color.jsm module [1] have chrome privilege (access to file system and network, etc.) given it's only doing math computation? Ok, I'm going off-topic, sorry for the noise. Maybe I'll open a different bug on the topic of loading modules with different privileges. [1] https://hg.mozilla.org/mozilla-central/file/tip/toolkit/modules/Color.jsm

(In reply to David Bruant from comment #98) > (In reply to Andrew McCreight [:mccr8] from comment #97) > > JSMs all run with chrome privilege, > > I guess that's a problem of its own. Why would the Color.jsm module [1] have > chrome privilege (access to file system and network, etc.) given it's only > doing math computation? > > Ok, I'm going off-topic, sorry for the noise. > Maybe I'll open a different bug on the topic of loading modules with > different privileges. Well, for one thing, for efficiency reasons: Cross-compartment (and especially cross-zone) JS communication is expensive. One of the biggest benefits of these patches is that they remove the need to create and call through cross-compartment wrappers every time code from one JSM touches an object from another (which is expensive both in terms of memory and performance). If some of the JSMs had a lower privilege level, things get even worse, since the privileged side would generally access the unprivileged side through X-ray wrappers (which are orders of magnitude more expensive than ordinary wrappers), there would be additional access checks in a lot of places, we generally wouldn't be able to pass objects to those helpers without cloning them into the unprivileged scope, ...

Comment on attachment 8902841 [details] Bug 1186409 - Fix Addon SDK JSM context detection. I don't know why MozReview keeps clearing these flags. Oh well.

I updated these patches on top of the patches in bug 1394490 and bug 1395360. It still seems to work with merging disabled, but with merging enabled there are errors almost immediately and it doesn't load, so something must be pretty wrong. I'm not sure what. The first error is: JavaScript error: [...]/browser/components/nsBrowserContentHandler.js, line 533: TypeError: LaterRun is undefined LaterRun is defined like this: XPCOMUtils.defineLazyModuleGetter(this, "LaterRun", "resource:///modules/LaterRun.jsm"); I'm not going to have time to investigate the cause of this today.

If I had to guess, I'd say this is because LaterRun is being defined with `let`: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/browser/modules/LaterRun.jsm#51 and we're somehow failing to map the NSVO to the right non-syntactic scope when we try to retrieve exported symbols from the module.

Oops. Good catch Kris. The issue is due to varEnv vs lexEnv lookup. There is a |ResolveModuleObjectProperty| function that needs rewriting. Basically can't use |JS_ExtensibleLexicalEnvironment| with NSVOs. The ExtensibleLexicalEnvironment of an NSVO is the lexEnv value you stashed. We can improve the APIs, but relying on the Compartment to cache the lexical env for you seems not great. I'd rather add a LEXICAL_SLOT link in the NSVO class to point to the corresponding lexical (similar to GlobalObject does).

Ah, mid-aired my comment in bug 1395360. Doesn't make a difference to me whether it's a compartment map or a slot, so long as we get top-level lexical symbol resolution :)

Kris, I'm thinking best course of action is to use the compartment map for this case to minimize changes. We will make js::CreateNonSyntacticGlobal only for the JSM loader. We can avoid :mccr8 needing to push around the lexEnv as much and keep using JS_ExtensibleLexicalEnvironment. Reducing risk vs trying to do code cleanup.

Sounds good to me

I rewrote Bug 1395360 to be similar to the original API on this bug with only a single environment to pass around, and using JS_ExtensibleLexicalEnvironment to get the other. Hopefully this is much easier.

It all seems to be working again. \o/ My local debug build seems to work properly with sharing enabled, so I launched a try run with sharing enable here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4147186b278af0c69cfb10a855cd492f62296164 The patch queue is: - An m-c revision from today (including Bug 1394490) - The update patches included on Bug 1395360 - All patches from this bug minus the WIP one - A minor patch for the new API of 1395360, replacing the WIP patch from this bug - Enabling jsloader.sharedGlobal by default for the testing

With the new-new API, the changes over my old patch are trivial, so I'll just fold it into the main patch. My fixup patch ended up being the same as Ted's, which is this, if you are curious: https://hg.mozilla.org/try/rev/cc0fc31bce97

Kris, Andrew, what's your take about landing this super late in the 57 cycle vs. waiting for 58? I know that this will probably give us a lot of performance benefits but its risk profile isn't quite clear to me, and we're getting to a point when we should think hard about reducing the risk in the 57 release and potentially defer even big perf wins that are late to the game for 58. I'm curious to know what you think about this one...

One relevant angle is that this is a significant memory win, which would counterbalance some amount of inevitable memory regression we're going to take with Stylo. I don't have a good sense of how much we care about memory metrics from release to release, and whether that should play a role in this decision.

I think it's worth landing in 57. The memory and performance improvements of not having to create thousands of extra cross-compartment wrappers at runtime, and dozens of extra BackstagePass globals at startup are huge. The actual behavior changes from these patches are behind a preference, so if it winds up causing problems, we can always turn it off with a hotfix add-on. It might be a good idea to add another preference to blacklist additional URLs from global sharing rather than just having a hard-coded list, so we can partially disable with a hotfix if necessary rather than turning it off entirely. That said, I don't think the risks are all that large. Most of our toolkit JS has already had to deal with global sharing for years, for the sake of b2g. Some issues may crop up shortly after landing, but I suspect they'll be fairly isolated and easy to fix.

From SpiderMonkey perspective, we are migrating the JSM loader from it's existing approach to using codepaths shared with FrameScript loading. There are some subtle differences between those two though. It is my understanding that JSM loading will encompass a larger and more varied set of JS code than FrameScripts do. We have been hacking away and identifying and fixing small little bugs that occur even in FrameScripts. I haven't run into concerns about JIT crashes or sandbox escapes, but have noticed little issues where JSMs may interfere or null-ptr derefs may occur. Overall it looks promising, but I expect there will be (minor) uplifts required. I'm am continuing to test and fix these edges. My preference would be landed, but pref'd off. Enabling it is quite possible, but there will be almost certainly be minor uplifts in 57beta. We should make sure everyone is on same page if we go forward. We have a try build that is quite functional up. Perhaps we can get more concrete memory/perf numbers.

On the XPConnect side, I don't think this is too risky. The risk is hidden behind a pref, so we can just turn it off again. I don't really have much of a sense of how risky the JS engine changes are, or how risky they are specifically in the case where global sharing is disabled. I also don't really know what our perf goals are, so I don't know how worthwhile it would be to take on this additional risk. (In reply to Ted Campbell [:tcampbell] from comment #134) > We have a try build that is quite functional up. Perhaps we can get more > concrete memory/perf numbers. I'd expect that they would be about the same as they were with my jankier old version. But I guess it is worth knowing that for sure.

(In reply to Andrew McCreight [:mccr8] from comment #135) > (In reply to Ted Campbell [:tcampbell] from comment #134) > > We have a try build that is quite functional up. Perhaps we can get more > > concrete memory/perf numbers. > > I'd expect that they would be about the same as they were with my jankier > old version. But I guess it is worth knowing that for sure. The perf numbers of your "jankier old version" being (conservatively) 8-10% improvement on ts_paint, 18% improvement on ts_paint_webext, 8% improvement on sessionrestore, 36-40% improvement on tp5o responsiveness, 4% improvement on tp5o, 8-9% improvement on RSS: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=80fafbaaf848&newProject=try&newRevision=4aec30e0a79ef2f58fa419e4ef9819804a3995bb&framework=1 If Ted's version for some reason doesn't show the same improvements, I think that would just be an argument that we should find out why and fix it, not that we shouldn't try to get it in for 57.

Please be careful with prefs related to this stuff. I remember that some of the JSM loader code runs before the pref manager has been initialized, so you always get the default value for prefs.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #136) > (In reply to Andrew McCreight [:mccr8] from comment #135) > > (In reply to Ted Campbell [:tcampbell] from comment #134) > > > We have a try build that is quite functional up. Perhaps we can get more > > > concrete memory/perf numbers. > > > > I'd expect that they would be about the same as they were with my jankier > > old version. But I guess it is worth knowing that for sure. > > The perf numbers of your "jankier old version" being (conservatively) 8-10% > improvement on ts_paint, 18% improvement on ts_paint_webext, 8% improvement > on sessionrestore, 36-40% improvement on tp5o responsiveness, 4% improvement > on tp5o, 8-9% improvement on RSS: Also, notably, these results are without bug 1381976, which adds a significant startup perf improvement even on top of these changes.

(In reply to Bill McCloskey (:billm) from comment #137) > Please be careful with prefs related to this stuff. I remember that some of > the JSM loader code runs before the pref manager has been initialized, so > you always get the default value for prefs. The shareGlobal pref is checked from ReallyInit(), which only happens just before we're about to load a module for the first time. The preference service should already be fully initialized by that point.

Andrew, I've push Bug 1395360 to tree to add the new API. The names changed slightly while going through review. We still need to address IndirectEval by fixing either Bug 1396050 or Bug 1396145 or disabling sharing on problematic scripts. We did discuss for example disabling JSM sharing for the devtools code. Hopefully you can land this pref'd off now.

(In reply to Ted Campbell [:tcampbell] from comment #140) > We still need to address IndirectEval by fixing either Bug 1396050 or Bug > 1396145 or disabling sharing on problematic scripts. We did discuss for > example disabling JSM sharing for the devtools code. Hi from devtools, may we help? I can look into things on Monday.

I don't think this is an issue for any devtools code (they don't generally use JSMs, anyway). There are a couple of JSM-ified third-party libraries that use eval or Function to get the global `this`, but we should be able to fix those by adding a couple of variable declarations to the heads of the files.

(In reply to Bill McCloskey (:billm) from comment #137) > Please be careful with prefs related to this stuff. I remember that some of > the JSM loader code runs before the pref manager has been initialized, so > you always get the default value for prefs. I've confirmed that the loader gets the right value, whether the pref is set to true or false.

Comment on attachment 8902841 [details] Bug 1186409 - Fix Addon SDK JSM context detection. MozReview really doesn't believe in these last three patches.

Comment on attachment 8903829 [details] Bug 1186409 - Fix code that tries to get the global by using |this|. https://reviewboard.mozilla.org/r/175590/#review182862

Comment on attachment 8902844 [details] Bug 1186409 - Check to make sure we don't set some weird XPConnect flags on the shared global. https://reviewboard.mozilla.org/r/174538/#review182864

Comment on attachment 8902842 [details] Bug 1186409 - Use Cu.getGlobalForObject when importing properties off a JSM global. https://reviewboard.mozilla.org/r/174534/#review182866

Comment on attachment 8902841 [details] Bug 1186409 - Fix Addon SDK JSM context detection. https://reviewboard.mozilla.org/r/174532/#review182868

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/244de99e2b94 Fix code that tries to get the global by using |this|. r=kmag https://hg.mozilla.org/integration/autoland/rev/ba636d93c4d5 Fix Addon SDK JSM context detection. r=kmag https://hg.mozilla.org/integration/autoland/rev/9109b1f49d23 Use Cu.getGlobalForObject when importing properties off a JSM global. r=kmag https://hg.mozilla.org/integration/autoland/rev/37e7829372eb Use a single global for all JSMs. r=kmag https://hg.mozilla.org/integration/autoland/rev/811d09140ee0 Check to make sure we don't set some weird XPConnect flags on the shared global. r=kmag