Bug 1341061 - Manually unwrap WithEnvironmentObjects in GETBOUNDNAME. (r=arai)
Also refactor some stuff in this area.
Documenting why I think it's correct to use GETBOUNDNAME only for
dynamic (vs global) lookups, without the normal NAME checks:
1. GETBOUNDNAME doesn't need to check TDZ because BINDNAME does it
already, and GETBOUNDNAME is always preceded by BINDNAME.
2. '.this' doesn't need to be checked, because '.this' can't be assigned
to in a compound assignment or inc/dec.
3. For a global name there is never @@unscopables on the global
environments, so GNAME ops, while doing repeated lookups, remain valid
optimizations because the repetition is unobservable.
4. We *will* do double @@unscopable lookup in the case of a script
compiled for a syntactic scope then run under a non-syntactic
WithEnvironmentObject (e.g. like frame scripts). But this is
Gecko-specific and outside the spec, so we can do whatever we want
here.
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -4042,42 +4042,65 @@ BytecodeEmitter::emitPropIncDec(ParseNod
return false;
if (post && !emit1(JSOP_POP)) // RESULT
return false;
return true;
}
bool
+BytecodeEmitter::emitGetNameAtLocationForCompoundAssignment(JSAtom* name, const NameLocation& loc)
+{
+ if (loc.kind() == NameLocation::Kind::Dynamic) {
+ // For dynamic accesses we need to emit GETBOUNDNAME instead of
+ // GETNAME for correctness: looking up @@unscopables on the
+ // environment chain (due to 'with' environments) must only happen
+ // once.
+ //
+ // GETBOUNDNAME uses the environment already pushed on the stack from
+ // the earlier BINDNAME.
+ if (!emit1(JSOP_DUP)) // ENV ENV
+ return false;
+ if (!emitAtomOp(name, JSOP_GETBOUNDNAME)) // ENV V
+ return false;
+ } else {
+ if (!emitGetNameAtLocation(name, loc)) // ENV? V
+ return false;
+ }
+
+ return true;
+}
+
+bool
BytecodeEmitter::emitNameIncDec(ParseNode* pn)
{
MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME));
bool post;
JSOp binop = GetIncDecInfo(pn->getKind(), &post);
auto emitRhs = [pn, post, binop](BytecodeEmitter* bce, const NameLocation& loc,
bool emittedBindOp)
{
JSAtom* name = pn->pn_kid->name();
- if (!bce->emitGetNameAtLocation(name, loc, false)) // SCOPE? V
- return false;
- if (!bce->emit1(JSOP_POS)) // SCOPE? N
- return false;
- if (post && !bce->emit1(JSOP_DUP)) // SCOPE? N? N
- return false;
- if (!bce->emit1(JSOP_ONE)) // SCOPE? N? N 1
- return false;
- if (!bce->emit1(binop)) // SCOPE? N? N+1
+ if (!bce->emitGetNameAtLocationForCompoundAssignment(name, loc)) // ENV? V
+ return false;
+ if (!bce->emit1(JSOP_POS)) // ENV? N
+ return false;
+ if (post && !bce->emit1(JSOP_DUP)) // ENV? N? N
+ return false;
+ if (!bce->emit1(JSOP_ONE)) // ENV? N? N 1
+ return false;
+ if (!bce->emit1(binop)) // ENV? N? N+1
return false;
if (post && emittedBindOp) {
- if (!bce->emit2(JSOP_PICK, 2)) // N? N+1 SCOPE?
- return false;
- if (!bce->emit1(JSOP_SWAP)) // N? SCOPE? N+1
+ if (!bce->emit2(JSOP_PICK, 2)) // N? N+1 ENV?
+ return false;
+ if (!bce->emit1(JSOP_SWAP)) // N? ENV? N+1
return false;
}
return true;
};
if (!emitSetName(pn->pn_kid, emitRhs))
return false;
@@ -5860,29 +5883,18 @@ BytecodeEmitter::emitAssignment(ParseNod
// to emit BINDNAME is involved and should avoid duplication.
if (lhs->isKind(PNK_NAME)) {
auto emitRhs = [op, lhs, rhs](BytecodeEmitter* bce, const NameLocation& lhsLoc,
bool emittedBindOp)
{
// For compound assignments, first get the LHS value, then emit
// the RHS and the op.
if (op != JSOP_NOP) {
- if (lhsLoc.kind() == NameLocation::Kind::Dynamic) {
- // For dynamic accesses we can do better than a GETNAME
- // since the assignment already emitted a BINDNAME on the
- // top of the stack. As an optimization, use that to get
- // the name.
- if (!bce->emit1(JSOP_DUP))
- return false;
- if (!bce->emitAtomOp(lhs, JSOP_GETBOUNDNAME))
- return false;
- } else {
- if (!bce->emitGetNameAtLocation(lhs->name(), lhsLoc))
- return false;
- }
+ if (!bce->emitGetNameAtLocationForCompoundAssignment(lhs->name(), lhsLoc))
+ return false;
}
// Emit the RHS. If we emitted a BIND[G]NAME, then the scope is on
// the top of the stack and we need to pick the right RHS value.
if (!EmitAssignmentRhs(bce, rhs, emittedBindOp ? 2 : 1))
return false;
if (!lhs->isInParens() && op == JSOP_NOP && rhs && rhs->isDirectRHSAnonFunction()) {
--- a/js/src/frontend/BytecodeEmitter.h
+++ b/js/src/frontend/BytecodeEmitter.h
@@ -540,16 +540,18 @@ struct MOZ_STACK_CLASS BytecodeEmitter
// used as a non-asserting version of emitUint16Operand.
MOZ_MUST_USE bool emitLocalOp(JSOp op, uint32_t slot);
MOZ_MUST_USE bool emitArgOp(JSOp op, uint16_t slot);
MOZ_MUST_USE bool emitEnvCoordOp(JSOp op, EnvironmentCoordinate ec);
MOZ_MUST_USE bool emitGetNameAtLocation(JSAtom* name, const NameLocation& loc,
bool callContext = false);
+ MOZ_MUST_USE bool emitGetNameAtLocationForCompoundAssignment(JSAtom* name,
+ const NameLocation& loc);
MOZ_MUST_USE bool emitGetName(JSAtom* name, bool callContext = false) {
return emitGetNameAtLocation(name, lookupName(name), callContext);
}
MOZ_MUST_USE bool emitGetName(ParseNode* pn, bool callContext = false);
template <typename RHSEmitter>
MOZ_MUST_USE bool emitSetOrInitializeNameAtLocation(HandleAtom name, const NameLocation& loc,
RHSEmitter emitRhs, bool initialize);
--- a/js/src/jit/SharedIC.cpp
+++ b/js/src/jit/SharedIC.cpp
@@ -1969,19 +1969,19 @@ ComputeGetPropResult(JSContext* cx, Base
res.setInt32(frame->numActualArgs());
} else {
MOZ_ASSERT(name == cx->names().callee);
MOZ_ASSERT(frame->script()->hasMappedArgsObj());
res.setObject(*frame->callee());
}
} else {
if (op == JSOP_GETBOUNDNAME) {
- RootedObject obj(cx, &val.toObject());
+ RootedObject env(cx, &val.toObject());
RootedId id(cx, NameToId(name));
- if (!GetPropertyForNameLookup(cx, obj, id, res))
+ if (!GetNameBoundInEnvironment(cx, env, id, res))
return false;
} else {
MOZ_ASSERT(op == JSOP_GETPROP || op == JSOP_CALLPROP || op == JSOP_LENGTH);
if (!GetProperty(cx, val, name, res))
return false;
}
}
--- a/js/src/vm/EnvironmentObject.h
+++ b/js/src/vm/EnvironmentObject.h
@@ -1049,16 +1049,24 @@ IsExtensibleLexicalEnvironment(JSObject*
inline bool
IsGlobalLexicalEnvironment(JSObject* env)
{
return env->is<LexicalEnvironmentObject>() &&
env->as<LexicalEnvironmentObject>().isGlobal();
}
+inline JSObject*
+MaybeUnwrapSyntacticWithEnvironment(JSObject* env)
+{
+ if (env->is<WithEnvironmentObject>() && env->as<WithEnvironmentObject>().isSyntactic())
+ return &env->as<WithEnvironmentObject>().object();
+ return env;
+}
+
template <typename SpecificEnvironment>
inline bool
IsFrameInitialEnvironment(AbstractFramePtr frame, SpecificEnvironment& env)
{
// A frame's initial environment is the innermost environment
// corresponding to the scope chain from frame.script()->bodyScope() to
// frame.script()->outermostScope(). This environment must be on the chain
// for the frame to be considered initialized. That is, it must be on the
--- a/js/src/vm/Interpreter-inl.h
+++ b/js/src/vm/Interpreter-inl.h
@@ -169,21 +169,17 @@ GetLengthProperty(const Value& lval, Mut
return true;
}
}
}
return false;
}
-enum class GetNameMode
-{
- Normal,
- TypeOf
-};
+enum class GetNameMode { Normal, TypeOf };
template <GetNameMode mode>
inline bool
FetchName(JSContext* cx, HandleObject receiver, HandleObject holder, HandlePropertyName name,
Handle<PropertyResult> prop, MutableHandleValue vp)
{
if (!prop) {
switch (mode) {
@@ -197,19 +193,17 @@ FetchName(JSContext* cx, HandleObject re
/* Take the slow path if shape was not found in a native object. */
if (!receiver->isNative() || !holder->isNative()) {
Rooted<jsid> id(cx, NameToId(name));
if (!GetProperty(cx, receiver, receiver, id, vp))
return false;
} else {
RootedShape shape(cx, prop.shape());
- RootedObject normalized(cx, receiver);
- if (normalized->is<WithEnvironmentObject>() && !shape->hasDefaultGetter())
- normalized = &normalized->as<WithEnvironmentObject>().object();
+ RootedObject normalized(cx, MaybeUnwrapSyntacticWithEnvironment(receiver));
if (shape->isDataDescriptor() && shape->hasDefaultGetter()) {
/* Fast path for Object instance properties. */
MOZ_ASSERT(shape->hasSlot());
vp.set(holder->as<NativeObject>().getSlot(shape->slot()));
} else {
if (!NativeGetExistingProperty(cx, normalized, holder.as<NativeObject>(), shape, vp))
return false;
}
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -2677,20 +2677,20 @@ CASE(JSOP_GETPROP_SUPER)
goto error;
REGS.sp--;
}
END_CASE(JSOP_GETPROP_SUPER)
CASE(JSOP_GETBOUNDNAME)
{
- ReservedRooted<JSObject*> obj(&rootObject0, ®S.sp[-1].toObject());
+ ReservedRooted<JSObject*> env(&rootObject0, ®S.sp[-1].toObject());
ReservedRooted<jsid> id(&rootId0, NameToId(script->getName(REGS.pc)));
MutableHandleValue rval = REGS.stackHandleAt(-1);
- if (!GetPropertyForNameLookup(cx, obj, id, rval))
+ if (!GetNameBoundInEnvironment(cx, env, id, rval))
goto error;
TypeScript::Monitor(cx, script, REGS.pc, rval);
assertSameCompartmentDebugOnly(cx, rval);
}
END_CASE(JSOP_GETBOUNDNAME)
CASE(JSOP_SETINTRINSIC)
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -2120,22 +2120,34 @@ js::NativeGetProperty(JSContext* cx, Han
bool
js::NativeGetPropertyNoGC(JSContext* cx, NativeObject* obj, const Value& receiver, jsid id, Value* vp)
{
AutoAssertNoException noexc(cx);
return NativeGetPropertyInline<NoGC>(cx, obj, receiver, id, NotNameLookup, vp);
}
bool
-js::GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue vp)
+js::GetNameBoundInEnvironment(JSContext* cx, HandleObject envArg, HandleId id, MutableHandleValue vp)
{
- RootedValue receiver(cx, ObjectValue(*obj));
- if (obj->getOpsGetProperty())
- return GeneralizedGetProperty(cx, obj, id, receiver, NameLookup, vp);
- return NativeGetPropertyInline<CanGC>(cx, obj.as<NativeObject>(), receiver, id, NameLookup, vp);
+ // Manually unwrap 'with' environments to prevent looking up @@unscopables
+ // twice.
+ //
+ // This is unfortunate because internally, the engine does not distinguish
+ // HasProperty from HasBinding: both are implemented as a HasPropertyOp
+ // hook on a WithEnvironmentObject.
+ //
+ // In the case of attempting to get the value of a binding already looked
+ // up via BINDNAME, calling HasProperty on the WithEnvironmentObject is
+ // equivalent to calling HasBinding a second time. This results in the
+ // incorrect behavior of performing the @@unscopables check again.
+ RootedObject env(cx, MaybeUnwrapSyntacticWithEnvironment(envArg));
+ RootedValue receiver(cx, ObjectValue(*env));
+ if (env->getOpsGetProperty())
+ return GeneralizedGetProperty(cx, env, id, receiver, NameLookup, vp);
+ return NativeGetPropertyInline<CanGC>(cx, env.as<NativeObject>(), receiver, id, NameLookup, vp);
}
/*** [[Set]] *************************************************************************************/
static bool
MaybeReportUndeclaredVarAssignment(JSContext* cx, HandleString propname)
{
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -1388,17 +1388,17 @@ NativeLookupOwnProperty(JSContext* cx,
*/
extern bool
NativeGetExistingProperty(JSContext* cx, HandleObject receiver, HandleNativeObject obj,
HandleShape shape, MutableHandleValue vp);
/* * */
extern bool
-GetPropertyForNameLookup(JSContext* cx, HandleObject obj, HandleId id, MutableHandleValue vp);
+GetNameBoundInEnvironment(JSContext* cx, HandleObject env, HandleId id, MutableHandleValue vp);
} /* namespace js */
template <>
inline bool
JSObject::is<js::NativeObject>() const { return isNative(); }
namespace js {