1095660 - Fix XBL field scope chains to include the "node scope chain" again

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We used to do this until bug 1088228.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

[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>().

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the pointless JS_ExecuteScriptVersion API

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add overloads of JS_ExecuteScript and JS::Evaluate that take an explicit scope chain argument

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change nsJSUtils::EvaluateString to take an explicit scope chain

Comment 6

[Bobby Holley (:bholley)]

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 7

[Bobby Holley (:bholley)]

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?

Comment 8

[Luke Wagner [:luke]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 10

[Bobby Holley (:bholley)]

(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.

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I meant nsJSUtils::EvaluateOptions

Ah.  I can totally do that.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 14

[Bobby Holley (:bholley)]

(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 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change nsJSUtils::EvaluateString to take an explicit scope chain

Comment 16

[Bobby Holley (:bholley)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 18

[Shu-yu Guo [:shu]]

Discussed with bz on IRC.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Luke Wagner [:luke]]

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).

Comment 22

[Bobby Holley (:bholley)]

(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));

Comment 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[Bobby Holley (:bholley)]

(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().

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Give With objects defineProperty/Element/Generic class hooks

Comment 29

[Luke Wagner [:luke]]

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.

Comment 30

[Luke Wagner [:luke]]

(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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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 32

[Jeff Walden [:Waldo]]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

   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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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