Fix subscript loader behavior inside a shared JSM

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

(Depends on: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
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 8

a year ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
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
(Assignee)

Comment 19

a year ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8906401 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8906399 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 25

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

Updated

a year ago
Priority: -- → P1

Comment 26

a year ago
mozreview-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.
Attachment #8906399 - Flags: review?(kmaglione+bmo) → review+

Comment 27

a year ago
mozreview-review
Comment on attachment 8906401 [details]
Bug 1398601 - Add testcase for loading subscripts within a JSM

https://reviewboard.mozilla.org/r/178108/#review183428
Attachment #8906401 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8906397 - Flags: review?(jorendorff)
Attachment #8906398 - Flags: review?(jorendorff)
Attachment #8906400 - Flags: review?(jorendorff)
(Assignee)

Comment 35

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

Comment 36

a year ago
mozreview-review
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.
Attachment #8906397 - Flags: review?(jorendorff) → review+

Comment 37

a year ago
mozreview-review
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.
Attachment #8906398 - Flags: review?(jorendorff) → review+

Comment 38

a year ago
mozreview-review
Comment on attachment 8906400 [details]
Bug 1398601 - Don't allow NSVO in js::CreateObjectsForEnvironmentChain

https://reviewboard.mozilla.org/r/178106/#review183482
Attachment #8906400 - Flags: review?(jorendorff) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8906397 - Attachment is obsolete: true
(Assignee)

Comment 43

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

Comment 44

a year ago
mozreview-review
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"
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 49

a year ago
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
(Assignee)

Comment 51

a year ago
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.
(Assignee)

Updated

a year ago
See Also: → bug 1398685
(Assignee)

Comment 52

a year ago
Oops.. those tests are supposed to throw the TypeErrors.
(Assignee)

Comment 53

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8907311 - Flags: review?(kmaglione+bmo)

Comment 58

a year ago
mozreview-review
Comment on attachment 8907311 [details]
Bug 1398601 - Add global/non-syntactic prefix to subscript loader cache

https://reviewboard.mozilla.org/r/178978/#review184098
Attachment #8907311 - Flags: review?(kmaglione+bmo) → review+

Comment 59

a year ago
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
Urgh. I probably should have looked more closely. The cache path logic is duplicated in DoLoadSubScriptWithOptions :(
(Assignee)

Comment 62

a year ago
Yeah, just found that. Putting up a patch..
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 67

a year ago
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.
Attachment #8907311 - Flags: review+ → review?(kmaglione+bmo)
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
(Assignee)

Comment 71

a year ago
That push broke the startup script cache, so talos regression makes sense.
(Assignee)

Comment 72

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1399343
(Assignee)

Comment 73

a year ago
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 74

a year ago
mozreview-review
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!
Attachment #8907311 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 80

a year ago
mozreview-review
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.
Attachment #8907733 - Flags: review?(ttromey) → review+

Comment 81

a year ago
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
(Assignee)

Comment 83

a year ago
Created attachment 8908106 [details] [diff] [review]
Fix wunused-variables error on beta merge

This fixes failures on non-DIAGNOSTIC_ASSERT builds.
Flags: needinfo?(ryanvm)

Comment 84

a year ago
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
You need to log in before you can comment on or make changes to this bug.