Closed Bug 1083648 Opened 10 years ago Closed 10 years ago

JIT inspector is broken by compartment-per-addon

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m5+ ---
firefox35 + fixed
firefox36 + fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files, 3 obsolete files)

9.95 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.41 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.21 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.01 KB, patch
Details | Diff | Splinter Review
4.48 KB, patch
bholley
: review+
Details | Diff | Splinter Review
[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...
So I've verified that changing those onclick handlers to use document.toggleOpcode works correctly.
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.
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(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.
Flags: needinfo?(luke) → needinfo?(shu)
> 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....
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(shu)
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.
(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);
  }
}
No, that should printt "a.proto".

The situation I'm dealing with can't be produced from script.  It's described in comment 2.
(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.
s/their parent as their global/their global as their parent
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 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?
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.
> 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.
(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?
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.
(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.)
> 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?
(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?
> 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.
(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 :)
See also bug 1074645 comment 24. I think this is an e10s blocker.
tracking-e10s: --- → ?
Blocks: 1074645
Flags: needinfo?(bzbarsky)
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?
Attachment #8510516 - Flags: review?(shu)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
> (with a scope chai

Er, ignore this part.  ;)
Flags: needinfo?(bzbarsky)
Attachment #8510516 - Attachment is obsolete: true
Attachment #8510516 - Flags: review?(shu)
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.
(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.
OK.  I'll update to that later tonight.
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|.
Attachment #8510522 - Flags: review?(shu)
Blocks: 1088228
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?
Flags: needinfo?(shu)
Attachment #8510517 - Attachment is obsolete: true
Attachment #8510517 - Flags: review?(bobbyholley)
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.
Attachment #8510818 - Flags: review?(bobbyholley) → review+
Attachment #8510522 - Attachment is obsolete: true
Flags: needinfo?(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 = */
Attachment #8511140 - Flags: review?(shu) → review+
Attachment #8511141 - Flags: review?(shu) → review+
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.
Attachment #8511140 - Flags: approval-mozilla-aurora?
Attachment #8511141 - Flags: approval-mozilla-aurora?
Attachment #8510818 - Flags: approval-mozilla-aurora?
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
Attachment #8511476 - Flags: review?(bobbyholley)
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 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.
Attachment #8511476 - Flags: review?(bobbyholley) → review+
Like I said, I expect to fix it in bug 1089026.
Attachment #8511140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8510818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8511141 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1105781
You need to log in before you can comment on or make changes to this bug.