Closed
Bug 1095660
Opened 10 years ago
Closed 10 years ago
Fix XBL field scope chains to include the "node scope chain" again
Categories
(Core :: XBL, defect)
Core
XBL
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(5 files, 2 obsolete files)
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•10 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•10 years ago
|
Flags: needinfo?(luke)
Assignee | ||
Comment 2•10 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•10 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8519366 -
Flags: review?(jwalden+bmo)
Attachment #8519366 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
Not requesting review on part 2 yet, since this is the part that involves the assertions
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8519368 -
Flags: review?(bobbyholley)
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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•10 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•10 years ago
|
Flags: needinfo?(luke)
Assignee | ||
Comment 9•10 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)
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
(Also, for the record, I am a JS folk).
Assignee | ||
Comment 12•10 years ago
|
||
> I meant nsJSUtils::EvaluateOptions
Ah. I can totally do that. ;)
Assignee | ||
Comment 13•10 years ago
|
||
> 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)
Comment 14•10 years ago
|
||
(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)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8520875 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8519368 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
> 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...
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8520930 -
Flags: review?(luke)
Assignee | ||
Comment 20•10 years ago
|
||
Bobby, see comment 17. I'm not sure what exactly you want in terms of multiple loops or not.
Flags: needinfo?(bobbyholley)
Comment 21•10 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+
Comment 22•10 years ago
|
||
(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)
Assignee | ||
Comment 23•10 years ago
|
||
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•10 years ago
|
Flags: needinfo?(bobbyholley)
Comment 24•10 years ago
|
||
(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)
Assignee | ||
Comment 25•10 years ago
|
||
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.
Assignee | ||
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
Part 2 updated to comments. Let me know if you want to see an interdiff too
Attachment #8521263 -
Flags: feedback?(luke)
Assignee | ||
Updated•10 years ago
|
Attachment #8520930 -
Attachment is obsolete: true
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8521266 -
Flags: review?(luke)
Comment 29•10 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•10 years ago
|
Attachment #8521266 -
Flags: review?(luke) → review+
Comment 30•10 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.
Assignee | ||
Comment 31•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 33•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla36
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
status-firefox36:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•