1083648 - JIT inspector is broken by compartment-per-addon

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

Steps to reproduce:

1)  Install https://addons.mozilla.org/en-US/firefox/addon/jit-inspector/
2)  Load https://bugzilla.mozilla.org/attachment.cgi?id=8505953
3)  Tools > Web Developer > JIT Inspector
4)  Click the "Start profiling" button.
5)  Reload the page loaded in step 2.
6)  Click the "Stop profiling" button in JIT Inspector.
7)  Click one of the links under "Scripts" in its output (which should be to
    something like
    "https://bug1081274.bugzilla.mozilla.org/attachment.cgi?id=8505953 (line 3)"
8)  Click one of the two red numbers (#3, #4) next to the one-line loop.

EXPECTED RESULTS: Shows a dump of the assembly for those instructions.

ACTUAL RESULTS:  JavaScript error: chrome://jitinspector/content/JITInspectorChrome.xul#, line 1: ReferenceError: toggleOpcode is not defined

The source for this addon is at <https://github.com/bhackett1024/CodeInspector>.  Looking at it, it looks like toggleOpcode is supposed to work like so:

  this._document.toggleOpcode = this.toggleOpcode.bind(this);

and then:

 text = "<a href='#' onclick='toggleOpcode(" + scriptIndex + "," + id + ")' class='opcodeDropdown'>";

and then the text is set as innerHTML on an element in this._document.

Note that the very similar toggleScript works, maybe because it's explicitly done as document.toggleScript() in the relevant onclick handlers.

So why does the bareword toggleOpcode not work?  The document should certainly be on the scope chain for that event handler, no?

I considered the possibility that this._document ends up being an Xray now for some reason, but then I expect toggleScript() would not work either...

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I've verified that changing those onclick handlers to use document.toggleOpcode works correctly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ah, I bet I know what's going on here.

In mozilla::EventListenerManager::CompileEventHandlerInternal we initially wrap mTarget into "v", which gives us a JSObject whose parent is the JSObject for the document.

Then we discover addonId is not null, and call GetAddonScope.  This part is a bit weird to me; should this scope really differ from the Window that we have in this case?  In any case, we end wrapping "v" into this scope, which gives us a cross-compartment wrapper.  Importantly, this totally changes the parenting: instead of having getParent() return the wrapper for the document, the getParent() on the cross-compartment wrapper we get returns a Sandbox (!).

This of course totally messes up our bareword lookup in the event handler that we then compile with "v" on the scope chain.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, talked to Bobby.

The behavior described in comment 2 is expected, but its result in this case is very much not desirable.  Unfortunately, changing the parent of CCWs is not trivial.

What would _really_ fix this bug is if we just specified the scope chain for event handlers explicitly instead of this getParent() hackery.  We keep talking about this, but could we actually do it?

Specifically, can we add JSAPI that does the equivalent of NewObjectEnvironment?  Or JSAPI that lets me create a "with" object?  I can punch through a hole for DynamicWithObject::create, which is what I think I want here, but that wants a StaticWithObject, and it's not clear to me what the right way to allocate StaticWithObjects is.  Can I just StaticWithObject::create those, or do I need to initEnclosingNestedScope() in the process or something else?

Comment 4

[Luke Wagner [:luke]]

Just to make sure I understand the goal:
 - we want a JSAPI that takes in the arguments to a function constructor as well as a scope chain (that possibly a parent-linked chain of DOM elements terminating at the global object) and returns a new function object
 - the environment of this new function object contains a with object for every non-global object in the original parent-linked chain (linked in that order), terminated by the global object; there are never any enclosing functions.

Is all that right?

Just to be clear: I forget exact semantic details of 'with', but isn't there technically a difference between having an object on the scope chain directly (as we have now) and having it proxied via with object?  IIUC, other engines already do this, so probably it's fine (or maybe I'm wrong and there is no difference), but perhaps they have hacky bits set on their 'with' objects that tweak behavior?

To answer your question: yes, you do need to create a StaticWithObject for each DynamicWithObject and yes you need to initEnclosingNestedScope to link up the StaticWithObjects in parallel to the DynamicWithObjects (passing nullptr for the outermost).

Shu might have some more relevant insights, since he's been hacking on this recently.  In particular; shu: is there a problem that these StaticWithObjects are have no corresponding JSScript?  That is, these StaticWithObjects enclose the JSScript of the constructed function and yet are not part of any global JSScript.  I can't find a problem.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is all that right?

Not quite.  My problem is precisely that the parent links are not what I want for the scope chain.

What I want is a way to conceptually start with a list of objects (or an iterator that can produce objects or whatever) and produce a function that has a with object for every non-global in that list, linked in that order, terminated by the global object, on its scope chain.  I very explicitly don't want us to follow the parent links.

> I forget exact semantic details of 'with'

Per ES6 spec, "with" does NewObjectEnvironment.  Per HTML spec, inline event handlers do NewObjectEnvironment.  So at least per spec they should match up.  ;)

I'm not aware offhand of obvious differences between our scope chain setup and using a chain of with objects....

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #4)

> Shu might have some more relevant insights, since he's been hacking on this
> recently.  In particular; shu: is there a problem that these
> StaticWithObjects are have no corresponding JSScript?  That is, these
> StaticWithObjects enclose the JSScript of the constructed function and yet
> are not part of any global JSScript.  I can't find a problem.

Should be no problem to have StaticWithObjects independent of any JSScripts, since the static scope objects don't keep back pointers to what they enclose. Even if the StaticWithObject isn't part of any global scripts, the innermost StaticWithObject created via this new API should be set as the enclosingStaticScope of the new function's JSScript.

(In reply to Boris Zbarsky [:bz] from comment #5)
> > Is all that right?
> 
> Not quite.  My problem is precisely that the parent links are not what I
> want for the scope chain.
> 
> What I want is a way to conceptually start with a list of objects (or an
> iterator that can produce objects or whatever) and produce a function that
> has a with object for every non-global in that list, linked in that order,
> terminated by the global object, on its scope chain.  I very explicitly
> don't want us to follow the parent links.

I'm not understanding what linking in "linked in that order" above refers to, if not the "enclosing scope" parent link.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It means the enclosing scope parent link.  The point is that I want to control this explicitly, not implicitly based on other parent links.

I'll see if I can come up with an API proposal here.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> It means the enclosing scope parent link.  The point is that I want to
> control this explicitly, not implicitly based on other parent links.
> 

Do you mean for something like the snippet below, you want it to print "b" instead of "a.proto"?

function A() { }
A.prototype.foo = "a.proto";

var AScope = new A;
var BScope = { foo: "b" };

with (BScope) {
  with (AScope) {
    print(foo);
  }
}

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, that should printt "a.proto".

The situation I'm dealing with can't be produced from script.  It's described in comment 2.

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> No, that should printt "a.proto".
> 
> The situation I'm dealing with can't be produced from script.  It's
> described in comment 2.

Ah okay. You should be fine, as DynamicWithObject is a ScopeObject, which stores its enclosing scope in a separate slot than the parent. IIRC, all ScopeObjects have their parent as their global.

Comment 11

[Shu-yu Guo [:shu]]

s/their parent as their global/their global as their parent

Comment 12

[Luke Wagner [:luke]]

bz: with this proposed change, could we make any JSAPI simplifications such as:
 1. take out the 'obj' parameter of (Compile|Evaluate|Execute)Script et al (effectively, taking 'obj' as cx->global())?
 2. use this to replace the JS_CloneFunctionObject calls in XBL and remove JS_CloneFunctionObject?

What I'd really love is if the scope chain contained only zero or more ScopeObjects followed by a GlobalObject.

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #12)
> bz: with this proposed change, could we make any JSAPI simplifications such
> as:
>  1. take out the 'obj' parameter of (Compile|Evaluate|Execute)Script et al
> (effectively, taking 'obj' as cx->global())?
>  2. use this to replace the JS_CloneFunctionObject calls in XBL and remove
> JS_CloneFunctionObject?
> 
> What I'd really love is if the scope chain contained only zero or more
> ScopeObjects followed by a GlobalObject.

In other words, so even things like GSP would be ScopeObjects?

Comment 14

[Luke Wagner [:luke]]

Oh darn, I forgot about GSP; I was under the naive recollection that DOM elements (and XBL whatevers) were the only source of non-global/scope objects on the scope chain.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 1. take out the 'obj' parameter of (Compile|Evaluate|Execute)Script et al (effectively,
> taking 'obj' as cx->global())?

As long as we have a way to specify a scope chain when compiling a function....  That part we still need, right?

> What I'd really love is if the scope chain contained only zero or more ScopeObjects
> followed by a GlobalObject.

That's what we all want, and I think we can get there.  That's the plan, at least.

> Oh darn, I forgot about GSP

The GSP is not on the scope chain.  It's on the proto chain of the global, as are various other objects.

Comment 16

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> As long as we have a way to specify a scope chain when compiling a
> function....  That part we still need, right?

Well, I was hoping that the "normal" JS_CompileFunction would not take a scope (it'd use cx->global()).  Only the as-yet-unwriten CompileEventHandler function which does all the magic with-wrapping would take an object.  Or did you have another use case in mind?

Comment 17

[Boris Zbarsky [:bzbarsky]]

We could do that, sure.

So the with-wrapping would happen under the hood in CompileEventHandler?  I was expecting to just expose API to pass in an object and a parent scope and get a "with" for the object, then build up my own chain of with-wrapped things to pass to whatever compilation API takes a scope.

Comment 18

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #17)
If that is simpler, sure, but I was thinking it'd be kindof nice if it all happened in one place inside JS so that JS engine people could see the whole story and know what was happening.  Also, there is a non-trivial amount of churn around scopes.  (E.g.,, one idea I like is to eventually make scope objects not be real JSObjects but their own type of GC thing.)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> but I was thinking it'd be kindof nice if it all happened in one place inside JS

That's fine by me as long as it actually happens... we keep talking about it but not making it happen.  ;)

So let's say we do this.  Then it seems like the fundamental thing you need passed in is a list of objects representing the scope chain plus the other arguments to CompileFunction.  How about we call the new method CompileScopedFunction?

What form would you like the "list of objects" bit in?  A value vector or something?

Comment 20

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> That's fine by me as long as it actually happens... we keep talking about it
> but not making it happen.  ;)

Oh, it sounded above like you were going to write it :)

> So let's say we do this.  Then it seems like the fundamental thing you need
> passed in is a list of objects representing the scope chain plus the other
> arguments to CompileFunction.  How about we call the new method
> CompileScopedFunction?

I feel like "Scoped" tends to mean "lexically scoped" which this is definitely not.  CompileDynamicallyScopedFunction?

> What form would you like the "list of objects" bit in?  A value vector or
> something?

IIUC, the objects are already linked up (via .parent, terminating in a global object) right?  In that case, can you just pass a single HandleObject to the innermost object?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Oh, it sounded above like you were going to write it :)

I'd like to.  But I'd also like to avoid doing major spidermonkey surgery in the process, honestly.

> CompileDynamicallyScopedFunction?

Worksforme.

> IIUC, the objects are already linked up

No, they are not.  That's the point.  The objects I want here, which are the cross-compartment wrappers for the things that are linked up, are not linked up.  That's precisely why we need a different mechanism!  See comment 2.

Comment 22

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> No, they are not.  That's the point.  The objects I want here, which are the
> cross-compartment wrappers for the things that are linked up, are not linked
> up.  That's precisely why we need a different mechanism!  See comment 2.

Ah, I see; I missed that.  Well then a const AutoVectorRooter& or perhaps HandleObjectVector (which doesn't exist, but I see a HandleValueVector) would seem reasonable.  I think the surgery should be fairly topical :)

Comment 23

[Bobby Holley (:bholley)]

See also bug 1074645 comment 24. I think this is an e10s blocker.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add JSAPI for compiling a function with a given scope chain (represented as a vector of JSObjects)

Let me know if it would be equivalent, and preferable, to leave the global off scopeChain entirely and just imply it via the current compartment of cx (with a scope chai.  For example, do we actually get the same behavior if scopeChain contains only a global vs if empty scopeChain is passed, with the patch as-is?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change nsJSUtils::CompileFunction to take an explicit scope chain vector, not just a single scope object, and pass in the right objects in CompileEventHandlerInternal

Comment 26

[Boris Zbarsky [:bzbarsky]]

> (with a scope chai

Er, ignore this part.  ;)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add JSAPI for compiling a function with a given scope chain (represented as a vector of JSObjects)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Those two patches seem to fix things for me.  I filed bug 1088228 on the followup work of never having random things on scope chains.

Comment 30

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #24)
> Created attachment 8510516 [details] [diff] [review]
> part 1.  Add JSAPI for compiling a function with a given scope chain
> (represented as a vector of JSObjects)
> 
> Let me know if it would be equivalent, and preferable, to leave the global
> off scopeChain entirely and just imply it via the current compartment of cx
> (with a scope chai.  For example, do we actually get the same behavior if
> scopeChain contains only a global vs if empty scopeChain is passed, with the
> patch as-is?

I think it would be. With CNG, it is more robust to just leave out the terminating GlobalObject and use the one associated with cx.

Comment 31

[Boris Zbarsky [:bzbarsky]]

OK.  I'll update to that later tonight.

Comment 32

[Shu-yu Guo [:shu]]

Comment on attachment 8510522 [details] [diff] [review]
part 1.  Add JSAPI for compiling a function with a given scope chain (represented as a vector of JSObjects)

Review of attachment 8510522 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'll trust you to change the API to not need to take in the terminating global.

Because of the way you are injecting 'with' scopes, the static scope chain will not show up via StaticScopeIter. That is, the newly compiled function's JSScript's enclosingScope_ will be nullptr.

This may be exactly what you want, since these aren't real static scopes anyways. But this does have two wrinkle once bug 963879 lands. The minor one is that Debugger will be unable to fully reflect the scope chain of functions compiled via this new API. Again, that might even be what you want. The major wrinkle is that in the interpreter, we assert that the static scope chain matches up with the dynamic scope chain in DEBUG builds (see the new AssertDynamicScopeMatchesStaticScope in the patch in bug 963879).

Usually, JSScript::enclosingScope_ is set by the BytecodeEmitter for nested functions. Punching a hole through the BCE for your API is going to hurt, because there'll be a static with scope without an actual with statement. You probably need to add a static scope argument to CompileFunctionBody to pass as the 2nd argument to JSScript::Create.

::: js/src/jsapi.cpp
@@ +4645,5 @@
> +                    const char *name, unsigned nargs, const char *const *argnames,
> +                    const char16_t *chars, size_t length, MutableHandleFunction fun)
> +{
> +    if (scopeChain.length() == 0) {
> +        // Not much neds to be done here.

Typo: needs.

::: js/src/jsapi.h
@@ +3949,5 @@
> +/**
> + * Compile a function with scopeChain as its scope chain.  scopeChain must
> + * contain objects in the current compartment of cx, with a global object (the
> + * global of cx) as the last element.  An empty scopeChain is allowed, and will
> + * behave just like passing a null "obj" to one of the overloads above.

I would add a explanation here that the scope chain objects are in fact proxied as if via |with|.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm.  I'm not sure what I want the debugger to do here.  What I want this to look like is as if I did some NewObjectEnvironment calls to create my function environment.  As long as the debugger doesn't claim actual "with" keywords were used, it might be fine...

I could try punching through the CompileFunctionBody bit.

But there's a bigger problem here too.  Doing what you suggest will work fine for the CompileFunction end of things, but we need custom scope chains for JS_CloneFunctionObject too.  I was going to do that the same way, but stamping stuff on the JSScript there isn't really reasonable, because it's shared across multiple functions, at least as long as you don't clone across compartments.  So what do we want to do in that situation?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change nsJSUtils::CompileFunction to take an explicit scope chain vector, not just a single scope object, and pass in the right objects in CompileEventHandlerInternal

Comment 35

[Boris Zbarsky [:bzbarsky]]

So per IRC discussion:

1) Punching through for the compile case probably makes sense.  No need to do it if our
   scope is just the global.
2) For the XBL case we'd want to leave the enclosingScope_ null (since we initially compile
   in global scope) and just not change it on clone.
3) We want to loosen up the assert in bug 963879 so the world doesn't explode.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to pass an enclosing static scope to CompileFunction

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add JSAPI for compiling a function with a given scope chain (represented as a vector of JSObjects)

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from the last thing shu reviewed

Comment 39

[Shu-yu Guo [:shu]]

Comment on attachment 8511140 [details] [diff] [review]
part 1.  Add a way to pass an enclosing static scope to CompileFunction

Review of attachment 8511140 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSLink.cpp
@@ +820,5 @@
>      SourceBufferHolder::Ownership ownership = stableChars.maybeGiveOwnershipToCaller()
>                                                ? SourceBufferHolder::GiveOwnership
>                                                : SourceBufferHolder::NoOwnership;
>      SourceBufferHolder srcBuf(chars, end - begin, ownership);
> +    if (!frontend::CompileFunctionBody(cx, &fun, options, formals, srcBuf, NullPtr()))

Could use a comment, like /* enclosingScope = */ NullPtr()

::: js/src/frontend/BytecodeCompiler.cpp
@@ +514,5 @@
>  // handler attribute in an HTML <INPUT> tag, or in a Function() constructor.
>  static bool
>  CompileFunctionBody(JSContext *cx, MutableHandleFunction fun, const ReadOnlyCompileOptions &options,
>                      const AutoNameVector &formals, SourceBufferHolder &srcBuf,
> +                    HandleObject enclosingScope,

Nit: join this line and the next please.

::: js/src/jsfun.cpp
@@ +1907,5 @@
>      SourceBufferHolder srcBuf(chars.start().get(), chars.length(), ownership);
>      if (isStarGenerator)
>          ok = frontend::CompileStarGeneratorBody(cx, &fun, options, formals, srcBuf);
>      else
> +        ok = frontend::CompileFunctionBody(cx, &fun, options, formals, srcBuf, NullPtr());

Could also do with the /* enclosingScope = */

Comment 40

[Boris Zbarsky [:bzbarsky]]

Updated to those comments and:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/9dae351f1d7a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f84c2b9b2a9c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a80bffd16ed

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511140 [details] [diff] [review]
part 1.  Add a way to pass an enclosing static scope to CompileFunction

Approval Request Comment
[Feature/regressing bug #]: Bug 1030420
[User impact if declined]: Some extensions are broken
[Describe test coverage new/current, TBPL]: Fairly well covered for the non-addon
   case.  Apparently none for the addon case...
[Risks and why]: Medium risk; this is pretty core code and we're changing it in a
   pretty fundamental way.  That said, I think this is ok to take on Aurora,
   because we do have good enough test coverage in the non-addon case.
[String/UUID change made/needed]:  None.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: followup to fix orange on CLOSED TREE due to random XBL stuff getting defined as props on the global

This side-effect is annoying.  I wish we could just explicitly say to not define on scope... in any case, this will get better in bug 1089026

Comment 43

[Boris Zbarsky [:bzbarsky]]

I pushed that fixup as https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a2d28c50c6

What I meant by "explicitly" was "inside the API that takes a scope vector", not its callers, because frankly the fact that compiling defines the function at all is nuts.

Comment 44

[Bobby Holley (:bholley)]

Comment on attachment 8511476 [details] [diff] [review]
followup to fix orange on CLOSED TREE due to random XBL stuff getting defined as props on the global

Review of attachment 8511476 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah yikes - let's find some way to fix the API here.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Like I said, I expect to fix it in bug 1089026.

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9dae351f1d7a
https://hg.mozilla.org/mozilla-central/rev/f84c2b9b2a9c
https://hg.mozilla.org/mozilla-central/rev/5a80bffd16ed
https://hg.mozilla.org/mozilla-central/rev/f5a2d28c50c6

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2cb21f30fa3d
https://hg.mozilla.org/releases/mozilla-aurora/rev/860511ae5a45
https://hg.mozilla.org/releases/mozilla-aurora/rev/8aa88cc51b86

I rolled the bustage fix into Part 3.