We used to do this until bug 1088228.

OK, so the same setup as we have for functions fails because we land in js::ExecuteKernel with type == EXECUTE_GLOBAL but scopeChainArg is a With object, which fails an assert. Does this assert just need to be adjusted, or is there a deeper invariant I'm violating here?

[Tracking Requested - why for this release]: And if I just comment out that assert, I hit another one in InterpreterFrame::epilogue because isGlobalFrame() but scopeChain()->is<ScopeObject>().

Not requesting review on part 2 yet, since this is the part that involves the assertions

Comment on attachment 8519366 [details] [diff] [review] part 1. Remove the pointless JS_ExecuteScriptVersion API Review of attachment 8519366 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +373,5 @@ > if (function) { > ok = JS_CallFunction(cx, targetObj, function, JS::HandleValueArray::empty(), > retval); > } else { > + ok = JS_ExecuteScript(cx, targetObj, script, retval); Can you remove the JS_GetVersion stuff above too? Looks like it's otherwise unused.

Comment on attachment 8519368 [details] [diff] [review] part 3. Change nsJSUtils::EvaluateString to take an explicit scope chain Review of attachment 8519368 [details] [diff] [review]: ----------------------------------------------------------------- I would much rather we push this into JS::EvaluateOptions, which I introduced exactly to counteract argument bloat on this method. We'd need to make the constructor take a cx, but that should be fine. Is there any reason we can't do that?

I think that assert was mostly just added "because it was true". But that did get me worried that maybe we were doing things like: while (!obj.is<ScopeObject>()) obj = obj->enclosingScope(); expecting to only skip the lexical scope objects. I did a quick grep for "is<ScopeObject>": - There is such a loop in SkipScopeParent, called by CloneFunctionObjectIfNotSingleton; I'm assuming CanReuseFunctionForClone would be false for any function that would have these non-lexical with objects on the scope chain (something would cause fun->hasSingletonType() to be false) but I don't really know; you'd probably want to confirm that with bhackett or shu. - InterpreterFrame::epilogue has an assert analogous to the one in ExecuteKernel. Are these non-lexical with objects detectable as such? If so, you could add an additional disjunct to the assert instead of removing it.

> Can you remove the JS_GetVersion stuff above too? Looks like it's otherwise unused. Looks used to me: 351 cachePath.AppendPrintf("jssubloader/%d", version); > I would much rather we push this into JS::EvaluateOptions If the JS folks are ok with that API. Are they? > Are these non-lexical with objects detectable as such? No idea, but we could probably make them so if they're not...

(In reply to Boris Zbarsky [:bz] from comment #9) > > I would much rather we push this into JS::EvaluateOptions > If the JS folks are ok with that API. Are they? Er, sorry. I meant nsJSUtils::EvaluateOptions, for which there's no need to ask the JS folks.

(Also, for the record, I am a JS folk).

> I meant nsJSUtils::EvaluateOptions Ah. I can totally do that. ;)

> Is there any reason we can't do that? So I tried it. The problem is that EvaluateOptions is const, while we need to wrap the scope chain into our compartment. We could make a copy of the scope chain, of course. Is that what you'd rather we do?

(In reply to Boris Zbarsky [:bz] from comment #13) > We could make a copy of the scope chain, of course. Is that what you'd > rather we do? Sure, or just make it non-const. It seems like this issue exists either way, and is orthogonal to whether we put this in EvaluateOptions. Making a copy is probably better IMO.

Comment on attachment 8520875 [details] [diff] [review] part 3. Change nsJSUtils::EvaluateString to take an explicit scope chain Review of attachment 8520875 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSUtils.cpp @@ +215,5 @@ > + > + // Do the junk Gecko is supposed to do before calling into JSAPI. > + for (size_t i = 0; i < scopeChain.length(); ++i) { > + JS::ExposeObjectToActiveJS(scopeChain[i]); > + } Hm, I'd rather not mess with all this stuff twice. Can't we just wait to do the appendAll until the part below where we wrap below, and merge it all into a single for loop? ::: dom/base/nsJSUtils.h @@ +64,5 @@ > const char** aArgArray, > const nsAString& aBody, > JSObject** aFunctionObject); > > struct EvaluateOptions { Can you make this MOZ_STACK_CLASS? ::: dom/plugins/base/nsNPAPIPlugin.cpp @@ +1554,5 @@ > .setVersion(JSVERSION_DEFAULT); > JS::Rooted<JS::Value> rval(cx); > + nsJSUtils::EvaluateOptions evalOptions(cx); > + if (obj != js::GetGlobalForObjectCrossCompartment(obj) && > + !evalOptions.scopeChain.append(obj)) { This theoretically changes behavior with where there's more than one non-global object on the parent chain right? I don't know if we care, but I just wanted to confirm.

> Can't we just wait to do the appendAll until the part below where we wrap below See comment 13. We can't wrap const things, so either EvaluateOptions needs to become non-const or we need to appendAll before we wrap. Or append one at a time, I guess.... In practice, the only time the scopechain here is nonempty is for XBL fields, of course. > This theoretically changes behavior with where there's more than one non-global object on > the parent chain right? Correct (though the point is there are no such cases on trunk anymore). I could preserve the old thing by unwrapping to Element like I do in XBL, but I figured this is less likely to be a compat issue...

Discussed with bz on IRC.

Bobby, see comment 17. I'm not sure what exactly you want in terms of multiple loops or not.

Comment on attachment 8520930 [details] [diff] [review] part 2. Change API-provided scope chains to flag their DynamicWithObjects as being special and adjust some asserts to allow those With objects as the scope chain for toplevel scripts Review of attachment 8520930 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense. ::: js/src/vm/ScopeObject.h @@ +174,5 @@ > * \ \ StaticWithObject Template for "with" object in static scope chain > * \ \ > * \ DynamicWithObject Run-time "with" object on scope chain > + * \ \ > + * \ FakeDynamicWithObject Used for API-provided scope chains I think this bit is stale and should be removed. @@ +400,5 @@ > class DynamicWithObject : public NestedScopeObject > { > static const unsigned OBJECT_SLOT = 1; > static const unsigned THIS_SLOT = 2; > + static const unsigned IS_API_PROVIDED_SCOPE_SLOT = 3; I'm wondering if we can't do better than "API-provided". How about isSyntactic() (where the API-provided ones return false)? isLexical is another obvious option, but "lexical" has been claimed for let things. @@ +410,5 @@ > static const Class class_; > > static DynamicWithObject * > + create(JSContext *cx, HandleObject object, HandleObject enclosing, HandleObject staticWith, > + bool isAPIProvidedScope = false); We generally try to avoid bool parameters (b/c they don't read particularly well at callsites). How about an enum WithKind? @@ +427,5 @@ > return getReservedSlot(THIS_SLOT).toObject(); > } > > + /* Return whether this object is a scope provided via JSAPI, not > + an actual lexical With object */ /* * */ for SM comments. Also, could you expand a bit for the non-initiated to, e.g., that these non-syntactic WithObjects are inserted between the outermost syntactic scope and the global object and wrap non-scope objects (such as DOM elements) passed to $(name of JSAPI functions).

(In reply to Boris Zbarsky [:bz] from comment #20) > Bobby, see comment 17. I'm not sure what exactly you want in terms of > multiple loops or not. I'm basically talking about the following - Not sure if the gray unmarking is still necessary in context of the wrap call, but I've left it in for completeness. diff --git a/dom/base/nsJSUtils.cpp b/dom/base/nsJSUtils.cpp index ddfb5bb..2ffd4af 100644 --- a/dom/base/nsJSUtils.cpp +++ b/dom/base/nsJSUtils.cpp @@ -166,7 +166,7 @@ nsJSUtils::CompileFunction(AutoJSAPI& jsapi, nsresult nsJSUtils::EvaluateString(JSContext* aCx, const nsAString& aScript, - JS::Handle<JSObject*> aScopeObject, + JS::Handle<JSObject*> aEvaluationGlobal, JS::CompileOptions& aCompileOptions, const EvaluateOptions& aEvaluateOptions, JS::MutableHandle<JS::Value> aRetValue, @@ -175,14 +175,14 @@ nsJSUtils::EvaluateString(JSContext* aCx, const nsPromiseFlatString& flatScript = PromiseFlatString(aScript); JS::SourceBufferHolder srcBuf(flatScript.get(), aScript.Length(), JS::SourceBufferHolder::NoOwnership); - return EvaluateString(aCx, srcBuf, aScopeObject, aCompileOptions, + return EvaluateString(aCx, srcBuf, aEvaluationGlobal, aCompileOptions, aEvaluateOptions, aRetValue, aOffThreadToken); } nsresult nsJSUtils::EvaluateString(JSContext* aCx, JS::SourceBufferHolder& aSrcBuf, - JS::Handle<JSObject*> aScopeObject, + JS::Handle<JSObject*> aEvaluationGlobal, JS::CompileOptions& aCompileOptions, const EvaluateOptions& aEvaluateOptions, JS::MutableHandle<JS::Value> aRetValue, @@ -197,6 +197,8 @@ nsJSUtils::EvaluateString(JSContext* aCx, MOZ_ASSERT_IF(!aEvaluateOptions.reportUncaught, aEvaluateOptions.needResult); MOZ_ASSERT(aCx == nsContentUtils::GetCurrentJSContext()); MOZ_ASSERT(aSrcBuf.get()); + MOZ_ASSERT(js::GetGlobalForObjectCrossCompartment(aEvaluationGlobal) == + aEvaluationGlobal); // Unfortunately, the JS engine actually compiles scripts with a return value // in a different, less efficient way. Furthermore, it can't JIT them in many @@ -205,14 +207,11 @@ nsJSUtils::EvaluateString(JSContext* aCx, // EvaluateString() which calls this function with aEvaluateOptions.needResult // set to false. aRetValue.setUndefined(); - - JS::ExposeObjectToActiveJS(aScopeObject); nsAutoMicroTask mt; nsresult rv = NS_OK; - bool ok = false; nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); - NS_ENSURE_TRUE(ssm->ScriptAllowed(js::GetGlobalForObjectCrossCompartment(aScopeObject)), NS_OK); + NS_ENSURE_TRUE(ssm->ScriptAllowed(aEvaluationGlobal), NS_OK); mozilla::Maybe<AutoDontReportUncaught> dontReport; if (!aEvaluateOptions.reportUncaught) { @@ -221,32 +220,42 @@ nsJSUtils::EvaluateString(JSContext* aCx, dontReport.emplace(aCx); } + bool ok = true; // Scope the JSAutoCompartment so that we can later wrap the return value // into the caller's cx. + JS::AutoObjectVector scopeChain(aCx); { - JSAutoCompartment ac(aCx, aScopeObject); + JSAutoCompartment ac(aCx, aEvaluationGlobal); + + // Now copy and wrap the scope chain. + for (size_t i = 0; i < aEvaluateOptions.scopeChain.length(); ++i) { + JS::ExposeObjectToActiveJS(aEvaluateOptions.scopeChain[i]); + if (!scopeChain.append(aEvaluateOptions.scopeChain[i]) || + !JS_WrapObject(aCx, scopeChain[i])) + { + ok = false; + break; + } + } - JS::Rooted<JSObject*> rootedScope(aCx, aScopeObject); - if (aOffThreadToken) { + if (ok && aOffThreadToken) { JS::Rooted<JSScript*> script(aCx, JS::FinishOffThreadScript(aCx, JS_GetRuntime(aCx), *aOffThreadToken));

OK, so you're like to append the items one by one. Wouldn't that have worse (re-)allocation behavior, that appending them all at once, if there several items?

(In reply to Boris Zbarsky [:bz] from comment #23) > OK, so you're like to append the items one by one. Wouldn't that have worse > (re-)allocation behavior, that appending them all at once, if there several > items? You can just .reserve() and then use infallibleAppend().

OK, sold. So a try run (before the above fixes, but pretty equivalent, I suspect) shows two issue: 1) We have some fields that do stuff like this: 2440 <field name="_panel" readonly="true"><![CDATA[ 2441 let node = this.parentNode; 2442 while(node && node.localName != "panel") { 2443 node = node.parentNode; 2444 } 2445 node; 2446 ]]></field> That "let" binding used to end up on ... I'm guessing the bound element? Now it's ending up on the global, somehow, or at least the test harness is seeing this property on the global. 2) I'm getting asserts in ScopeIter::settle after all, but only on some platforms. I guess I'll just adjust those.

So for problem #1, here's what happens. We end up with a JSOP_DEFVAR. That calls DefVarOrConstOperation, which looks up the prop name, doesn't find it, and does JSObject::defineProperty to define it on the With object. This lands us in JSObject::defineGeneric, we look up the defineGeneric object op, there is none on With objects, so we call baseops::DefineGeneric. This goes ahead and adds the property to the With object itself (via js::NativeObject::putProperty, etc). Then we get tp the JSOP_BINDNAME. We do LookupNameUnqualified. We do a JSObject::lookupGeneric on the With. But With _does_ have a lookupGeneric op: with_LookupGeneric. This forwards the lookup to the underlying object, where it fails, since the property was never defined on there. So we keep walking up the scope chain until we get to the global and bind the name there, since that's the isUnqualifiedVarObj. Should With objects have a defineGeneric op that forwards to the underlying object?

Part 2 updated to comments. Let me know if you want to see an interdiff too

Comment on attachment 8521263 [details] [diff] [review] part 2. Change API-provided scope chains to flag their DynamicWithObjects as being special and adjust some asserts to allow those With objects as the scope chain for toplevel scripts Perfect, thanks.

(In reply to Boris Zbarsky [:bz] from comment #26) What you say makes sense, but I'm not really an expert on the intersection between proxy ops and name lookup. I'd be curious why it doesn't bite in normal with use.

https://tbpl.mozilla.org/?tree=Try&rev=faaa76933cd6 Luke and I talked though maybe only doing the define thing for non-syntactic with and decided we'll go with what I have for now.

Comment on attachment 8519366 [details] [diff] [review] part 1. Remove the pointless JS_ExecuteScriptVersion API Review of attachment 8519366 [details] [diff] [review]: ----------------------------------------------------------------- I am mildly confused as to how this ever made any sense, as I thought script versions were always determined at script creation time, so this extra argument would always have been useless. Meh.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fae2321c2676 https://hg.mozilla.org/integration/mozilla-inbound/rev/57efd5c311bd https://hg.mozilla.org/integration/mozilla-inbound/rev/59e54843c6b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/8897fd934074 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b063da7562

https://hg.mozilla.org/mozilla-central/rev/fae2321c2676 https://hg.mozilla.org/mozilla-central/rev/57efd5c311bd https://hg.mozilla.org/mozilla-central/rev/59e54843c6b6 https://hg.mozilla.org/mozilla-central/rev/8897fd934074 https://hg.mozilla.org/mozilla-central/rev/c9b063da7562