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

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36+ fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

4.50 KB, patch
Waldo
: review+
bholley
: review+
Details | Diff | Splinter Review
7.94 KB, patch
Details | Diff | Splinter Review
18.32 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.54 KB, patch
luke
: feedback+
Details | Diff | Splinter Review
2.67 KB, patch
luke
: review+
Details | Diff | Splinter Review
We used to do this until bug 1088228.
(Assignee)

Comment 1

4 years ago
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?
(Assignee)

Updated

4 years ago
Flags: needinfo?(luke)
(Assignee)

Comment 2

4 years ago
[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>().
tracking-firefox36: --- → ?
(Assignee)

Updated

4 years ago
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 3

4 years ago
Created attachment 8519366 [details] [diff] [review]
part 1.  Remove the pointless JS_ExecuteScriptVersion API
Attachment #8519366 - Flags: review?(jwalden+bmo)
Attachment #8519366 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8519367 [details] [diff] [review]
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
(Assignee)

Comment 5

4 years ago
Created attachment 8519368 [details] [diff] [review]
part 3.  Change nsJSUtils::EvaluateString to take an explicit scope chain
Attachment #8519368 - Flags: review?(bobbyholley)
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.
Attachment #8519366 - Flags: review?(bobbyholley) → review+
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?
Attachment #8519368 - Flags: review?(bobbyholley)

Comment 8

4 years ago
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.

Updated

4 years ago
Flags: needinfo?(luke)
(Assignee)

Comment 9

4 years ago
> 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...
Flags: needinfo?(shu)
Flags: needinfo?(jorendorff)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(jorendorff)
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Created attachment 8520875 [details] [diff] [review]
part 3.  Change nsJSUtils::EvaluateString to take an explicit scope chain
Attachment #8520875 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Attachment #8519368 - Attachment is obsolete: true
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.
Attachment #8520875 - Flags: review?(bobbyholley) → review+
> 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

4 years ago
Discussed with bz on IRC.
Flags: needinfo?(shu)
Created 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
Attachment #8520930 - Flags: review?(luke)
Bobby, see comment 17.  I'm not sure what exactly you want in terms of multiple loops or not.
Flags: needinfo?(bobbyholley)

Comment 21

4 years ago
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).
Attachment #8520930 - Flags: review?(luke) → review+
(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));
Flags: needinfo?(bobbyholley)
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?
(Assignee)

Updated

4 years ago
Flags: needinfo?(bobbyholley)
(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().
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(luke)
Created 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

Part 2 updated to comments.  Let me know if you want to see an interdiff too
Attachment #8521263 - Flags: feedback?(luke)
(Assignee)

Updated

4 years ago
Attachment #8520930 - Attachment is obsolete: true
Created attachment 8521266 [details] [diff] [review]
part 3.  Give With objects defineProperty/Element/Generic class hooks
Attachment #8521266 - Flags: review?(luke)

Comment 29

4 years ago
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.
Flags: needinfo?(luke)
Attachment #8521263 - Flags: feedback?(luke) → feedback+

Updated

4 years ago
Attachment #8521266 - Flags: review?(luke) → review+

Comment 30

4 years ago
(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.
Attachment #8519366 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

4 years ago
Flags: needinfo?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Blocks: 1097987
(Assignee)

Updated

4 years ago
Target Milestone: --- → mozilla36
Depends on: 1098295
tracking-firefox36: ? → -
tracking-firefox36: - → +
status-firefox36: --- → fixed
You need to log in before you can comment on or make changes to this bug.