See: https://bugzilla.mozilla.org/show_bug.cgi?id=1186409#c161 With the new JSM global sharing (Bug 1186409), we load JSMs into a NonSyntacticVariablesObject instead of a private global. To facilitate this, we added a new API in Bug 1395360. Using the subscript loader from within a JSM NSVO will currently try to put the NSVO inside a WithEnvironment. This is inconsistent with the non-shared case where a global would be at root of chain, not within a With. The expected chain should be: > [JSM Shared Global/BackstagePass] > [JSM Shared Global LexicalEnvironmentObject] > [NSVO] > [Extensible LexicalEnvironmentObject] ** > [WithEnvironment(target object)] > [Extensible LexicalEnvironmentObject] > > ** This environment holds the |this| pointer of things like |(function() { return this; })()| This should be achieved by updating the ExecuteInJSMEnvironment API.

TODO: - Update ExecuteInJSMEnvironment API - Assert in CreateObjectsForEnvironmentChain we don't wrap NSVO - Update mozJSSubscriptLoader to call these - Add xpcshell test cases similar to the other environment tests

I put together a crude prototype. Running a try run with global sharing enabled here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4782b4c35771c667420e8728ea9f26416e66dc37 This still depends on IndirectEval being fixed, so a couple xpcshell tests should fail.

Comment on attachment 8906399 [details] Bug 1398601 - Fix subscript loader when using JSM global sharing https://reviewboard.mozilla.org/r/178104/#review183042 ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:186 (Diff revision 1) > + } else if (js::IsJSMEnvironment(targetObj)) { > + MOZ_DIAGNOSTIC_ASSERT(loadScope == targetObj); > + if (!ExecuteInJSMEnvironment(cx, script, targetObj)) { > + return false; > + } > + // FIXME - Support return values Nah, let's not. Code compiled to return a value runs much slower than it would otherwise. Let's just throw if someone tries to execute a return value script in a JSM environment.

Oops, I didn't handle the cross-compartment subscript loading correctly. I also added a diagnostic the check if we are about to subscript load into the bare shared JSM global. One behavior difference once we turn on global sharing occurs when a JSM loads a subscript into a target of another JSM. Without sharing, this was a cross-compartment subscript load that would pollute the target. With sharing, it is no long a cross-compartment subscript load and will pollute the caller JSM instead. Another try attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faebdf60947c75a568ae5326b354a25bd28898db

(In reply to Ted Campbell [:tcampbell] from comment #12) > One behavior difference once we turn on global sharing occurs when a JSM > loads a subscript into a target of another JSM. Without sharing, this was a > cross-compartment subscript load that would pollute the target. With > sharing, it is no long a cross-compartment subscript load and will pollute > the caller JSM instead. Yup, that's expected (see bug 1186409 comment 78). There's not really anything we can do about it, so I don't think it's worth worrying about.

GetTargetObject shouldn't match the NSVO of a framescript or the subscript loader will be unhappy. https://treeherder.mozilla.org/#/jobs?repo=try&revision=81cb50bc9287f755b42ec4d961c92c1c1869c384

Ah, I see that :kmag pointed out this issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1186409#c163 . I proposed FindTargetObject only do the NSVO thing if it is from the loader global.

The patches are working. I still need to remove the copy-paste code in JS side, but the xpconnect side is ready for review.

Comment on attachment 8906399 [details] Bug 1398601 - Fix subscript loader when using JSM global sharing https://reviewboard.mozilla.org/r/178104/#review183420 ::: js/xpconnect/loader/mozJSComponentLoader.cpp:456 (Diff revision 4) > - if (!aTargetObject) { > + // > + // If the target object was not in the JSM shared global, return the global > + // instead. This is needed when calling the subscript loader within a frame > + // script, since it the FrameScript NSVO will have been found. > + if (!aTargetObject || > + !IsLoaderGlobal(js::GetGlobalForObjectCrossCompartment(aTargetObject))) { Hm. I'm on the fence about this. I think we really probably do want to use the NSVO as the default import target in frame scripts, especially since there's no way for the scripts to access it as an explicit target (`this` points to the global in frame scripts, unlike in JSMs). But I can also see an argument for fixing that as a follow-up. ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:191 (Diff revision 4) > - if (loadScope != targetObj && > - loadScope && > - !JS_IsGlobalObject(loadScope)) > - { > - MOZ_DIAGNOSTIC_ASSERT(js::GetObjectCompartment(loadScope) == > - js::GetObjectCompartment(targetObj)); > - > - if (!envChain.append(loadScope)) { > + if (js::GetObjectCompartment(loadScope) != js::GetObjectCompartment(targetObj)) { > + // If target is cross-compartment, check the target isn't in JSM > + // shared global compartment or we will contaiminate it. > + // NOTE: If loadScope is already a shared-global JSM, we can't > + // determine which JSM the target belongs too. > + JSObject* targetGlobal = js::GetGlobalForObjectCrossCompartment(targetObj); > + MOZ_DIAGNOSTIC_ASSERT(!mozJSComponentLoader::Get()->IsLoaderGlobal(targetGlobal), > + "Don't load subscript into target in a shared-global JSM"); I'd rather keep the compartment check in DoLoadSubScriptWithOptions, and stick to the null check here. And if we do the target global check there, we can just throw rather than asserting.

Comment on attachment 8906401 [details] Bug 1398601 - Add testcase for loading subscripts within a JSM https://reviewboard.mozilla.org/r/178108/#review183428

I moved the compartment check back to DoLoadSubScriptWithOptions. Deferring changing the FrameScript behaviour as a follow-up.

Comment on attachment 8906397 [details] Bug 1398601 - Add js::IsJSMEnvironment to jsfriendapi https://reviewboard.mozilla.org/r/178100/#review183446 I am the worst reviewer ::: js/src/builtin/Eval.cpp:543 (Diff revision 2) > } > + > +JS_FRIEND_API(bool) > +js::IsJSMEnvironment(JSObject* obj) > +{ > + return obj->is<NonSyntacticVariablesObject>(); This could use a comment explaining why the implementation is correct (or pointing to the comment that explains the general situation). ::: js/src/jsfriendapi.h:2893 (Diff revision 2) > ExecuteInGlobalAndReturnScope(JSContext* cx, JS::HandleObject obj, JS::HandleScript script, > JS::MutableHandleObject scope); > > // These functions are only for JSM component loader, please don't use this for anything else! > extern JS_FRIEND_API(JSObject*) > NewJSMEnvironment(JSContext* cx); Bad luck: I just realized "nobody should ever use this" is not an acceptable excuse for not documenting a public function. Please comment all these functions briefly.

Comment on attachment 8906398 [details] Bug 1398601 - Support target objects in js::ExecuteInJSMEnvironment https://reviewboard.mozilla.org/r/178102/#review183460 ::: js/src/builtin/Eval.cpp:448 (Diff revision 4) > { > return fun->maybeNative() == IndirectEval; > } > > static bool > -ExecuteInNonSyntacticGlobalInternal(JSContext* cx, HandleObject global, HandleScript scriptArg, > +ExecuteInNonSyntacticGlobalInternal(JSContext* cx, HandleScript scriptArg, HandleObject env) NonSyntacticGlobal isn't a good name, even for internal use only. If anything, the idea that these objects behave kind of like global objects will seem *less* sensible for people working inside the engine. ExecuteInExtensibleLexicalEnvironment perhaps? ::: js/src/builtin/Eval.cpp:520 (Diff revision 4) > +{ > assertSameCompartment(cx, varEnv); > MOZ_ASSERT(cx->compartment()->getNonSyntacticLexicalEnvironment(varEnv)); > + MOZ_DIAGNOSTIC_ASSERT(scriptArg->noScriptRval()); > > RootedObject global(cx, &varEnv->global()); `global` is now unused, I think. ::: js/src/jsfriendapi.h:2898 (Diff revision 4) > NewJSMEnvironment(JSContext* cx); > > extern JS_FRIEND_API(bool) > ExecuteInJSMEnvironment(JSContext* cx, JS::HandleScript script, JS::HandleObject nsvo); > > +extern JS_FRIEND_API(bool) Short comment, please, at least explaining the preconditions on nsvo and script.

Comment on attachment 8906400 [details] Bug 1398601 - Don't allow NSVO in js::CreateObjectsForEnvironmentChain https://reviewboard.mozilla.org/r/178106/#review183482

- Added comments to the APIs - Flattened the first two patches - s/ExecuteInNonSyntacticGlobalInternal/ExecuteInExtensibleLexicalEnvironment/ - Removed the dead 'global' local variable

Comment on attachment 8906398 [details] Bug 1398601 - Support target objects in js::ExecuteInJSMEnvironment https://reviewboard.mozilla.org/r/178102/#review183532 ::: js/src/builtin/Eval.cpp:528 (Diff revision 5) > + // them to the environment. These are added after the NSVO environment. > + if (!targetObj.empty()) { > + // The environment chain will be as follows: > + // GlobalObject / BackstagePass > + // LexicalEnvironmentObject[this=global] > + // NonSyntacticVariablesObject // NonSyntacticVariablesObject (the JSMEnvironment) ::: js/src/builtin/Eval.cpp:576 (Diff revision 5) > + > +JS_FRIEND_API(bool) > +js::IsJSMEnvironment(JSObject* obj) > +{ > + // NOTE: This also returns true if the NonSyntacticVariablesObject was > + // created for reasons other than the JSM loader. Heh. ::: js/src/jsfriendapi.h:2897 (Diff revision 5) > +// > +// A 'JSMEnvironment' refers to an environment chain constructed for JSM loading > +// in a shared global. Internally it is a NonSyntacticVariablesObject with a > +// corresponding extensible LexicalEnvironmentObject that is accessible by > +// JS_ExtensibleLexicalEnvironment. The |this| value of that lexical environment > +// is the NSVO itself. Feel free to use or discard this. (Of course, if you use it, you have to fix my mistakes. Nothing's free...) ``` // Normal global environment (ES6): JSM "global" environment: // // * - extensible lexical environment // | (code runs in this environment; // | `let/const` bindings go here) // | // * - JSMEnvironment (=== `this`) // | (`var` bindings go here) // | // * - extensible lexical environment * - extensible lexical environment // | (code runs in this environment; | (empty) // | `let/const` bindings go here) | // | | // * - actual global (=== `this`) * - shared JSM global // (var bindings go here; and (Object, Math, etc. live here) // Object, Math, etc. live here) ``` ::: js/src/jsfriendapi.h:2930 (Diff revision 5) > +// NOTE: This may find NonSyntacticVariablesObject generated by other embedding > +// such as a Gecko FrameScript. Caller can check the compartment if needed. > extern JS_FRIEND_API(JSObject*) > GetJSMEnvironmentOfScriptedCaller(JSContext* cx); > > +// Determine if current object is a JSMEnvironment "Determine if obj is a JSMEnvironment"

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e9816be61b49 Support target objects in js::ExecuteInJSMEnvironment r=jorendorff https://hg.mozilla.org/integration/autoland/rev/dd4af7998505 Fix subscript loader when using JSM global sharing r=kmag https://hg.mozilla.org/integration/autoland/rev/7a4bb5a1848a Add testcase for loading subscripts within a JSM r=kmag https://hg.mozilla.org/integration/autoland/rev/7aac2595bc17 Don't allow NSVO in js::CreateObjectsForEnvironmentChain r=jorendorff

Backed out in https://hg.mozilla.org/integration/autoland/rev/fe7465d53091 for Windows (7/8/10, opt/pgo/debug) Marionette (Mn and MnH) crashes/assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=130218034&repo=autoland

The failing test case is of course the pref-toggle test added in Bug 1398499. I'm going through the log now and see a number of |TypeError: ... is undefined| that need to be addressed. I'm not sure if these do to Bug 1396050, or if the marionette scripts are being sloppy. The assertion that fires is that we are trying to load a subscript into a JSM |this| without it being script-compiled for non-syntactic environment support.

Oops.. those tests are supposed to throw the TypeErrors.

When we start using JSMEnvironments from subscript loader, we trip assertions about non-syntactic environment scripts. In CloneAndExecuteScript, we can clone script to make it compatible. ExecuteInJSMEnvironment was based off ExecuteInGlobalAndReturnScope which does not implicitly clone. In the FrameScript case, the script cache will only allow one version to be stored and doesn't use cache if it does not match. To fix this patch, I'll have the startup-cache use the same prefixing we did for JSM loader. This test only failed on Windows due to divergent implementations of Subprocess.jsm.

Comment on attachment 8907311 [details] Bug 1398601 - Add global/non-syntactic prefix to subscript loader cache https://reviewboard.mozilla.org/r/178978/#review184098

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/776a65d43a5e Support target objects in js::ExecuteInJSMEnvironment r=jorendorff https://hg.mozilla.org/integration/autoland/rev/eddc38b0afd0 Add global/non-syntactic prefix to subscript loader cache r=kmag https://hg.mozilla.org/integration/autoland/rev/f5c26c3407c0 Fix subscript loader when using JSM global sharing r=kmag https://hg.mozilla.org/integration/autoland/rev/05957a92b1a5 Add testcase for loading subscripts within a JSM r=kmag https://hg.mozilla.org/integration/autoland/rev/b728872f4d9a Don't allow NSVO in js::CreateObjectsForEnvironmentChain r=jorendorff

Backed out in https://hg.mozilla.org/integration/autoland/rev/a45742d015d3 for unexpected crashtest assertions, https://treeherder.mozilla.org/logviewer.html#?job_id=130503890&repo=autoland

Urgh. I probably should have looked more closely. The cache path logic is duplicated in DoLoadSubScriptWithOptions :(

Yeah, just found that. Putting up a patch..

Comment on attachment 8907311 [details] Bug 1398601 - Add global/non-syntactic prefix to subscript loader cache Rewrote this patch to factor out the cache key generation to one place.

This let browser-chrome's browser_ext_devtools_network.js frequently failing. There are also assertions before, but only this test fails: See the Linux bc16 one in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=82f171552905ea7bdbc60688db5b4aad3a3f4b95&filter-searchStr=4e1b40c7bb84b37a00fd934a28d025a1dae2e2d0

Before the last push, these Talos regressions were observed: == Change summary for alert #9406 (as of September 13 2017 01:12 UTC) == Regressions: 16% tpaint summary windows10-64 pgo e10s 222.00 -> 258.22 10% sessionrestore windows10-64 pgo e10s 426.00 -> 467.42 9% sessionrestore_no_auto_restore windows10-64 pgo e10s515.33 -> 564.00 8% ts_paint windows10-64 pgo e10s 574.71 -> 620.50 4% sessionrestore_many_windows windows10-64 pgo e10s3,617.38 -> 3,769.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9406

After the backout, the perf baselines returned to normal: == Change summary for alert #9416 (as of September 13 2017 03:11 UTC) == Improvements: 15% tpaint summary windows10-64 pgo e10s 258.12 -> 220.31 9% sessionrestore windows10-64 pgo e10s 470.04 -> 427.17 9% sessionrestore_no_auto_restore windows10-64 pgo e10s564.42 -> 515.00 7% ts_paint windows10-64 pgo e10s 622.00 -> 577.08 4% sessionrestore_many_windows windows10-64 pgo e10s3,771.71 -> 3,626.58 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9416

That push broke the startup script cache, so talos regression makes sense.

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #68) > This let browser-chrome's browser_ext_devtools_network.js frequently > failing. There are also assertions before, but only this test fails: > > See the Linux bc16 one in > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=82f171552905ea7bdbc60688db5b4aad3a3f4b95&filter- > searchStr=4e1b40c7bb84b37a00fd934a28d025a1dae2e2d0 The startup cache errors are fixed in my staged copy now. The |panel is undefined| message needs further investigation.

As discussed on #developers. There were also failures in tc-M(dt7) of devtools/client/commandline/test/browser_gcli_async.js. This is a bug in the test where they load a subscript with wrong char encoding. Previously, a specific ordering of script cache behavior would ignore the (incorrect) default char encoding and the test would work.

Comment on attachment 8907311 [details] Bug 1398601 - Add global/non-syntactic prefix to subscript loader cache https://reviewboard.mozilla.org/r/178978/#review184548 ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:95 (Diff revision 2) > NS_IMPL_ISUPPORTS(mozJSSubScriptLoader, mozIJSSubScriptLoader) > > +#define JSSUB_CACHE_PREFIX(aType) "jssubloader/" aType > + > +static void > +SubscriptCachePath(JSContext* cx, nsIURI* uri, JS::HandleObject targetObj, nsACString& cachePath) Thank you!

Comment on attachment 8907733 [details] Bug 1398601 - Fix devtools browser_gcli_async test https://reviewboard.mozilla.org/r/179410/#review184582 Thank you for the patch. This looks good. I just wanted to note that we discussed this on irc and concluded that this was the best route forward, because there's no good spot to force the test encoding to be UTF-8 without impacting other things; and that we don't believe the encoding is relevant to the substance of the test.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1ef5858d537c Support target objects in js::ExecuteInJSMEnvironment r=jorendorff https://hg.mozilla.org/integration/autoland/rev/62d3e87c505c Fix devtools browser_gcli_async test r=tromey https://hg.mozilla.org/integration/autoland/rev/354b9d0b2101 Add global/non-syntactic prefix to subscript loader cache r=kmag https://hg.mozilla.org/integration/autoland/rev/675444347ccd Fix subscript loader when using JSM global sharing r=kmag https://hg.mozilla.org/integration/autoland/rev/51d0521b16aa Add testcase for loading subscripts within a JSM r=kmag https://hg.mozilla.org/integration/autoland/rev/f70469dfd87a Don't allow NSVO in js::CreateObjectsForEnvironmentChain r=jorendorff

https://hg.mozilla.org/mozilla-central/rev/1ef5858d537c https://hg.mozilla.org/mozilla-central/rev/62d3e87c505c https://hg.mozilla.org/mozilla-central/rev/354b9d0b2101 https://hg.mozilla.org/mozilla-central/rev/675444347ccd https://hg.mozilla.org/mozilla-central/rev/51d0521b16aa https://hg.mozilla.org/mozilla-central/rev/f70469dfd87a

This fixes failures on non-DIAGNOSTIC_ASSERT builds.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/mozilla-central/rev/ef0f1085d54e Fix wunused-variable errors when MOZ_DIAGNOSTIC_ASSERT isn't available. a=RyanVM