A let-binding at top-level should not be turned into a var-binding. Harmony calls for disallowing the same variable to be both let-declared and var-declared in the same scope. Dave

When I first started using 'let', I assumed that a let at the toplevel would be lexically scoped (to this script) and wouldn't be a property of the global object. Is that what this bug is about?

(In reply to comment #1) > When I first started using 'let', I assumed that a let at the toplevel would be > lexically scoped (to this script) and wouldn't be a property of the global > object. Is that what this bug is about? Yes, for let at top level in global code. For let at the top of a function body, it also means not aliasing formal parameters (or arguments[i] for the i'th formal with the same name as the let binding!), rather shadowing them. /be

Given all the various changes to scoping, block stuffs, etc. that have been done in the last six months to a year, I think this may be feasible to do without incredible pain at this point. Could be wrong, I haven't done too much in the front end lately. But I think this may be pretty doable now. Assigning to Nathan to see what he can get done on it. It'll be interesting to see how extensions and our own code react to this change. I expect the "let is the new var" people may end up surprised by this change in places, when a let-variable in one script isn't visible in others. Guess we'll see.

First patch, adds a new object, lexicalScope, to the scope chain right below the global and fixes related breakage.

Comment on attachment 8439622 [details] [diff] [review] addLexicalScope_r1.patch - Add lexicalScope to the scope chain Review of attachment 8439622 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty reasonable overall. (At least, assuming we've hit all the places needing changes, and I don't truly know that we have. But I'm cautiously optimistic. :-) ) Making the SHG scope chain include a lexical is probably the biggest requirement for this to land, at this point. Other than that it's semi-cosmetic things. ::: js/src/jscntxt.h @@ +375,5 @@ > > // Current global. This is only safe to use within the scope of the > // AutoCompartment from which it's called. > inline js::Handle<js::GlobalObject*> global() const; > + inline js::Handle<JSObject*> lexicalScope() const; Could use a separate comment on this, something like // Lexical environment object, to be used in the future for storing // globally-scoped |let| variables. Can this be made Handle<BlockObject*> or something to clarify by type that this probably isn't what people want, unless they're specifically looking for scope-ful stuff? Actually, maybe we should add a named typedef to vm/ScopeObject.h now for this -- LexicalStorageObject. (And propagate that typedef everywhere in this patch, to save me pointing it out everywhere I didn't before making this comment. :-) ) As this object's going to be a little unusual in terms of supporting no deletions yet not being static, I suspect we're going to want a new kind of scope object for this. A typedef will ease that transition -- and also support the previous concern a little, too. ::: js/src/jscompartment.cpp @@ +579,5 @@ > if (enterCompartmentDepth && global_) > MarkObjectRoot(trc, global_.unsafeGet(), "on-stack compartment global"); > + > + if (enterCompartmentDepth && lexicalScope_) > + MarkObjectRoot(trc, lexicalScope_.unsafeGet(), "on-stack compartment lexicalScope"); What with the global storing the lexical in a reserved slot now, it might be the case this (and various other special-casing like it) isn't needed any more. Heck, maybe we could even get rid of the field in the compartment entirely. Does that seem feasible to you? ::: js/src/jsobjinlines.h @@ +638,5 @@ > return &global() == this; > } > > +inline JSObject & > +JSObject::lexicalScope() const Yeah, looking at the full patch, this shouldn't exist, should go through the global to get the lexical from it. ::: js/src/vm/GlobalObject.cpp @@ +230,5 @@ > Rooted<GlobalObject *> global(cx, &obj->as<GlobalObject>()); > > cx->compartment()->initGlobal(*global); > > + Rooted<StaticBlockObject *> lexicalScope(cx, StaticBlockObject::create(cx)); StaticBlockObject::create needs to be checked for failure, returning nullptr on failure. @@ +239,5 @@ > return nullptr; > if (!global->setDelegate(cx)) > return nullptr; > + // Put the lexicalScope in a reserved slot in the global so it isn't GCed. > + global->setReservedSlot(LEXICAL_SCOPE, ObjectValue(*lexicalScope)); I believe setVarObj and setDelegate can GC, so this should move up next to the initLexicalScope call. In any case, even if they can't, that's conceptually the right place to put this. ::: js/src/vm/Interpreter-inl.h @@ +240,5 @@ > * In strict-mode, we need to trigger an error when trying to assign to an > * undeclared global variable. To do this, we call SetPropertyHelper > * directly and pass Unqualified. > */ > + if (scope->is<GlobalObject>() || isLexicalScope(scope)) { So is it the case that we should ever get a GlobalObject here any more after this patch? At the least, ultimately I think we always have to get a lexical here -- for if we didn't, a setname targeting a let-variable wouldn't actually modify that let-variable, which would be not-to-spec semantics. This sort of conversion from isGlobal -> isGlobal or isLexical appears a bunch of places in this patch. I think we want to make all of them isGlobal -> isLexical. Agree with me, or convince me I'm missing something here. :-) ::: js/src/vm/Interpreter.cpp @@ +273,5 @@ > * the actual behavior even if the id could be found on the scope chain > * before the global object. > */ > if (IsGlobalOp(JSOp(*pc))) > + obj = &obj->lexicalScope(); I think probably lexicalScope() should be a method on GlobalObject. Certainly there's no good reason for the global lexical scope of an object to be so easily accessible on any object at all. @@ +587,5 @@ > js::ExecuteKernel(JSContext *cx, HandleScript script, JSObject &scopeChainArg, const Value &thisv, > ExecuteType type, AbstractFramePtr evalInFrame, Value *result) > { > JS_ASSERT_IF(evalInFrame, type == EXECUTE_DEBUG); > + JS_ASSERT_IF(type == EXECUTE_GLOBAL, isLexicalScope(&scopeChainArg) || GetRuntime(cx)->isSelfHostingGlobal(&scopeChainArg)); Hmm. I expect self-hosting will have to have a lexical scope as well, here, when we start encoding number-of-scope-chain-hops into lookups. It's debatable whether we truly need consistency just quite yet. But I think I'm leery about having inconsistency here. So the SHG scope chain needs to have a lexical in it too, for this patch to land. @@ +623,5 @@ > return false; > > + /* Use the scope chain as 'this', modulo outerization. However, > + * if the scope chain is the global object, swap it out for the > + * lexicalScope. The global remains 'this'. */ Style nit: /* * multi... * line... * comment. */ That said, it appears the old comment works just fine here. @@ +631,5 @@ > + Value thisv = ObjectValue(*thisObj); > + > + /* The VAROBJFIX option makes varObj == globalObj in global code. */ > + if (!cx->options().varObjFix()) { > + if (!scopeChain->setVarObj(cx)) Fairly sure this can GC, in which case |thisv| could become invalid in a moving world. So you'll need a RootedValue for thisv. This would be obvious in the ggc (?) builds on tryserver, which I believe turn red when these sorts of mistakes are made. We should probably get you tryserver access -- especially if you're working on a bug like this, that requires a bunch of touching in a bunch of different places, and benefits from all the browser tests being exercised atop its patches. @@ +636,5 @@ > + return false; > + } > + > + if (scopeChain == cx->global() && !GetRuntime(cx)->isSelfHostingGlobal(scopeChain)) > + scopeChain = GetInnerObject(cx->compartment()->maybeLexicalScope()); Self-hosting code should see the same structure of scope chain as regular code -- there needs to be a lexical in the scope chain, even if as discussed previously it'll need to always be empty. So this should be only if (scopeChain->is<GlobalObject>()) scopeChain = ...; (The is<> thing is micro-optimization territory, but my guess is it's very slightly faster.) @@ +645,5 @@ > do { > assertSameCompartment(cx, s); > JS_ASSERT_IF(!s->enclosingScope(), s->is<GlobalObject>()); > } while ((s = s->enclosingScope())); > #endif Let's have this loop where it was before. Best to have asserts as early as possible, and all. ::: js/src/vm/ScopeObject.cpp @@ +1044,5 @@ > type_ = callobj.isForEval() ? StrictEvalScope : Call; > hasScopeObject_ = true; > JS_ASSERT_IF(type_ == Call, callobj.callee().nonLazyScript() == frame_.script()); > } else { > + JS_ASSERT(!cur_->is<ScopeObject>() || isLexicalScope(&*cur_)); Eugh, &*. Either this should be .get(), or (more likely) IsLexicalScope should take a handle type. ::: js/src/vm/ScopeObject.h @@ +919,5 @@ > } > > +template<typename T> > +inline bool > +isLexicalScope(T object) Standalone methods are named InterCaps-style, so IsLexicalScope. Can this be template<typename T> inline bool IsLexicalScope(Handle<T*> obj) for slightly more specificity? @@ +921,5 @@ > +template<typename T> > +inline bool > +isLexicalScope(T object) > +{ > + return object-> template is<js::StaticBlockObject>(); Nitpick, get rid of space after ->. ::: js/src/vm/Stack-inl.h @@ +56,5 @@ > > inline JSObject & > +InterpreterFrame::lexicalScope() const > +{ > + return scopeChain()->lexicalScope(); If this is only relevant for global frames, which it might be (potentially unlike global()), we should MOZ_ASSERT(isGlobalFrame()). ::: js/src/vm/Stack.h @@ +587,5 @@ > inline HandleObject scopeChain() const; > > inline ScopeObject &aliasedVarScope(ScopeCoordinate sc) const; > inline GlobalObject &global() const; > + inline JSObject &lexicalScope() const; Hmm. Not sure if only relevant to global frames, or also relevant to function frames. I don't know ES6 well enough to say how var/let interact/conflict at top level within functions.

Work in progress - adding another object to the scope chain. Added an assert causing lots of tests to fail because Eval needs to be fixed.

Comment on attachment 8454098 [details] [diff] [review] postCleanupForReview.patch - All tests passing Review of attachment 8454098 [details] [diff] [review]: ----------------------------------------------------------------- Is this just to add an extra scope object right before the global, with no code yet to actually put the let decls in there?

Yes. Just tack in an extra scope object before the global, containing nothing, and ensure all code can deal with the presence of that extra object. Going through this now...

Comment on attachment 8454098 [details] [diff] [review] postCleanupForReview.patch - All tests passing Review of attachment 8454098 [details] [diff] [review]: ----------------------------------------------------------------- I'm out for most of this week (excepting only Thursday), unfortunate timing for this to be done, but oh well. :-\ shu is probably the next-best person to ask questions for now. So on looking at this again, it seems to me that we might have gone too far, slightly. For global environment lookups, because any global script might not mention the lets it uses (or the vars), we have to always have both lexical and global in the scope chain. But inside other scripts, the let-object to hold, say, the let in eval("let x = 42");, should only come into play when executing the script itself, and not at all if there's no let in it. In other words, during parsing (or bytecode compilation or emission, not sure which phase exactly), we should observe whether a top-level let occurs, and only *then* insert an extra scope object for lets. Lookups homed against the global environment record are special; others get/don't get a let object according to regular practice. As far as modifying the parser to flag this, Parser::letDeclaration is probably the right place to do that. Where to store the bit to record "lets were used", not 100% sure off the top of my head. Check with shu. I had a few other comments written out, before I got to the point of realizing that inserting an extra hop everywhere, versus only when the global object is affected, was desirable. Some may apply still, some might not -- use your judgment/ask shu if you're unsure about any of them. ::: js/src/vm/GlobalObject.cpp @@ +234,5 @@ > Rooted<GlobalObject *> global(cx, &obj->as<GlobalObject>()); > > cx->compartment()->initGlobal(*global); > > + LexicalObject *lexObj = CreateLexicalObject(cx, global); Rooted<LexicalObject*> lexicalScope(cx, CreateLexicalObject(cx, global)); will let you get rid of an extra local. @@ +240,5 @@ > + return nullptr; > + > + Rooted<LexicalObject *> lexicalScope(cx, lexObj); > + // Put the lexicalScope in a reserved slot in the global so it isn't GCed. > + global->setReservedSlot(LEXICAL_SCOPE, ObjectValue(*lexicalScope)); I think we can do without this comment, if it immediately follows creation of the lexical object. Objects have to remain rooted somehow. Storing into a reserved slot of a related object is a super-common concept, not worth calling out explicitly. Rooted<LexicalObject*> lexicalScope(cx, CreateLexicalObject(cx, global)); if (!lexicalScope) return nullptr; global->setReservedSlot(LEXICAL_SCOPE, ObjectValue(*lexicalScope)); is nice and concise, and I think quite clear about what it's doing. No need to say more. ::: js/src/vm/GlobalObject.h @@ +267,5 @@ > > public: > static GlobalObject *create(JSContext *cx, const Class *clasp); > > + JSObject & Make this LexicalObject & please. You'll have to static_cast<LexicalObject&>(), but that's not too bad. @@ +269,5 @@ > static GlobalObject *create(JSContext *cx, const Class *clasp); > > + JSObject & > + lexicalScope() { > + return getReservedSlot(LEXICAL_SCOPE).toObject(); MOZ_ASSERT(getReservedSlot(LEXICAL_SCOPE).isObject(), "global without an associated lexical scope"); ::: js/src/vm/ScopeObject.h @@ +449,5 @@ > } > }; > > +//Used for lexical scopes > +class ExtensibleBlockObject : public BlockObject "lexical scope" is probably a little bit too broad a description. Every script's lets are stored in a lexical scope. The global lexical scope fits that bill. But, *only* the global lexical scope can ever grow new variables. So how about // A lexical scope, containing let declarations, whose binding set isn't fixed // at construction time but instead may gain (but never lose) new declarations // over time. This kind of scope object is currently only used to store let // declarations in global code. class Extensible BlockObject : public BlockObject (Also note the space between comment-start characters and the actual text of the comment.) @@ +453,5 @@ > +class ExtensibleBlockObject : public BlockObject > +{ > + public: > + static const Class class_; > + static ExtensibleBlockObject *create(ExclusiveContext *cx); I think you have your indentation off by two here. { public: static const Class class_; static ExtensibleBlockObject *create(ExclusiveContext *cx); }; @@ +934,5 @@ > return is<js::BlockObject>() && !getProto(); > } > > + > +typedef js::ExtensibleBlockObject LexicalObject; If we're going to have this typedef, it should be inside namespace js. But now that we have an explicitly different block object class here, I don't quite think we need it. ExtensibleBlockObject should be fully adequate for distinguishing these sorts of scope objects from the others (including from non-extensible block objects).

So this is an updated WIP. A linux64 try is currently running to see what fails there, but we've gotten a lot closer with this.

Step 1: add another scope works and passes Try!

Comment on attachment 8471948 [details] [diff] [review] step1PassesTry.patch Review of attachment 8471948 [details] [diff] [review]: ----------------------------------------------------------------- My eyes glossed over a bit on the debugger tests, and I mostly assumed that numeric increments and all that made any sense. We'll want jimb to do a full, proper review of those changes once this patch is in complete shape. ::: browser/devtools/debugger/test/browser_dbg_variables-view-filter-02.js @@ -214,5 @@ > > withScope.expand(); > functionScope.expand(); > globalScope.expand(); > - Probably don't remove this blank line, as I'm guessing it was there for some sort of style reason and doesn't need to change. ::: browser/devtools/debugger/test/browser_dbg_variables-view-frame-parameters-03.js @@ +40,5 @@ > > function expandGlobalScope() { > let deferred = promise.defer(); > > + //let globalScope = gVariables.getScopeAtIndex(1); Remove the commented-out line, same for the other hits in this file. And in other test files you've changed this way. (Looks like you were halfway through before you decided to stop commenting out, or something.) ::: dom/bindings/BindingUtils.h @@ +1468,4 @@ > > // A way to differentiate between nodes, which use the parent object > // returned by native->GetParentObject(), and all other objects, which > // just use the parent's global. This comment needs to be consistent with the code. @@ +1478,5 @@ > > static inline JSObject* > GetRealParentObject(Element* aParent, JSObject* aParentObject) > { > + return (aParentObject && js::GetGlobalForObjectCrossCompartment(aParentObject) == aParentObject) ? js::GetLexicalForObjectCrossCompartment(aParentObject) : aParentObject; Latter condition should just be JS_IsGlobalObject(aParentObject). ::: dom/xbl/nsXBLProtoImplMethod.cpp @@ +297,5 @@ > nsresult rv = nsContentUtils::WrapNative(cx, aBoundElement, &v); > NS_ENSURE_SUCCESS(rv, rv); > > JS::Rooted<JSObject*> thisObject(cx, &v.toObject()); > + JS::Rooted<JSObject*> thisObjectAlso(cx, &v.toObject()); Debugging detritus? ::: js/src/builtin/Eval.cpp @@ +290,5 @@ > // Use the global as 'this', modulo outerization. > + JSObject *thisobj = scopeobj; > + RootedObject rootedThis(cx, HopLexicalScopes(thisobj)); > + thisobj = JSObject::thisObject(cx, rootedThis); > + Why not RootedObject rootedThis(cx, HopLexicalScopes(scopeobj)); RootedObject thisobj(cx, JSObject::thisObject(cx, rootedThis)); if (!thisobj) ... ? And be sure to kill the blank line between assignment and returned-null check. @@ +353,5 @@ > esg.setNewScript(compiled); > } > > + // Insert a new lexical scope here (with scopeobj as parent) if the script needs it > + // (that is, if the script has let variables) Let's expand this comment some and point out the current ill-defined nature of 'let' variables in eval code: // ES6 draft 20140718 currently doesn't specify how 'let' declarations in // eval code work. See 18.2.1.2. It is possible to imagine that top-level // eval, or indirect eval, might define 'let' variables in the global // lexical scope. (It's also possible this behavior could depend on the // strictness of the eval code, with strict eval code not doing so and with // non-strict eval code doing so.) However, it seems likelier that 'let' // variables in eval code will be visible *only* to that eval code, eval // being so screwball. For now we store 'let' variables in eval code in // their own environment, never in the declarative component of the global // environment record. // // With this in mind, we add a lexical scope here, enclosed by the provided // scope, if the script defines top-level 'let' variables. We're going to have to figure out something so that adding in a lexical object, if necessary, is done by the script execution code itself, at some point. (Or perhaps by InterpreterFrame::prologue -- which is where strict eval code picks up a custom object to store variables.) It shouldn't be necessary for EvalKernel to be consulting and adding it manually. But let's run with this for now, and we can change to the prologue mechanism (perhaps) later. @@ +358,5 @@ > + RootedObject newScopeObj(cx, esg.script()->hasTopLevelLet() > + ? CreateExtensibleBlockObject(cx, scopeobj) > + : (JSObject*) scopeobj); > + if (!newScopeObj) > + return false; I'd probably have RootedObject scope(cx); if (esg.script()->hasTopLevelLet()) { scope = CreateExtensibleBlockObject(cx, scopeobj); if (!scope) return false; } else { scope = scopeobj; } @@ +364,5 @@ > NullFramePtr() /* evalInFrame */, args.rval().address()); > } > > bool > js::DirectEvalStringFromIon(JSContext *cx, Your patch not making any changes to this method seems awfully dodgy. :-) I think you need to do some scope-patching here. (I am totally unsurprised that eval from Ion code is undertested here. Potentially jit-tests.py --tbpl [that is, with --ion-eager in the arguments] will point out issues even with the existing tests.) @@ +502,5 @@ > if (!script) > return false; > } > > + // Put the global's lexical scope back on the chain Period at end of complete sentence. Also add // XXX Hack! We should be using an actual ScopeObject here, not a regular // object. But it at least serves the purpose for now. @@ +522,5 @@ > RootedValue thisv(cx, ObjectValue(*thisobj)); > RootedValue rval(cx); > + > + // Insert a new lexical scope here (with scopeobj as parent) if the script needs it > + // (that is, if the script has let variables) Period at end of sentence. Also add // Unlike in EvalKernel's case, where there are specification questions // concerning whether 'let' variables should be placed in the global // environment record or not, ExecuteInGlobalAndReturnScope is totally our // own baby, defined however we want it. So regardless what the spec might // say happens for eval, here we will *always* put 'let' variables in a // scope only for the code executed here. ::: js/src/frontend/Parser.cpp @@ +3685,2 @@ > */ > + pc->sc->noteTopLevelLet(); This affects all body-level lets, right? That is, both of the lets here: <script> let x; function f() { let y; } </script> For now we only want to affect the first one, I think. Probably. Shouldn't be too hard to do with some pc->sc->isFunctionBox() checking or so. Looks like you could have if (!pc->sc->isFunctionBox()) { stuff from above rejecting top-level lets in self-hosted code; /* ES6 specifies... */ pc->sc->noteTopLevelLet(); } ::: js/src/jit-test/tests/debug/Debugger-debuggees-19.js @@ +5,5 @@ > // If we eval in a debuggee, log which debuggee it was. > var log; > dbg.onEnterFrame = function (frame) { > log += 'e'; > + //Jump through a lexical (the eval only adds a lexical if needed) Space after // and before the comment text, for consistency with the rest of the test. ::: js/src/jit-test/tests/debug/Environment-callee-01.js @@ +11,5 @@ > dbg.onDebuggerStatement = function (frame) { > hits++; > var env = frame.environment; > assertEq(env.type, expectedType); > + //Check if the parent is the expected one (because we jump through a lexical) Space after //. @@ +41,5 @@ > > // Lenient direct eval shares eval's caller's environment, > // although this is a great evil. > +// Declarative because of the lexical > +check('eval("debugger");', 'declarative', null); It seems somewhat of an open question to me, now, whether we want to always insert a lexical scope for eval code. Always inserting is simpler, to be sure. (Although it may well break things like the eval cache, potentially.) Yet we have to accept some complexity about not always inserting, because of the global lexical scope stuff for code evaluating against the global environment record. Something to keep in mind as the code evolves, I guess, to figure out what makes most sense and is as simple as we might or might not want. ::: js/src/jit-test/tests/debug/Environment-identity-03.js @@ +35,1 @@ > "environments nested within the one containing '" + name + "' should not be shared"); If the assertion no longer makes sense, it probably should be removed. ::: js/src/jit-test/tests/debug/Environment-setVariable-08.js @@ +8,5 @@ > var hits = 0; > dbg.onDebuggerStatement = function (frame) { > var env = frame.older.environment; > + //The new behavior of setVariable climbs scope, so just check that a variable that does not exist will still throw > + assertThrowsInstanceOf(function () { env.setVariable("yNOT", 2); }, Error); Given my comments in Debugger.cpp, I'm not sure that this change should occur, if Debugger is done the way it seems was agreed upon by jimb. ::: js/src/jit-test/tests/debug/Environment-type-01.js @@ +1,3 @@ > +// env.type is 'object' in global environments and with-blocks, and 'declarative' otherwise, > +// except now we have the lexical scopes, which are also declarative, and are not only always under > +// the global, but can also be introduced by evals with lets and debugger evals.. This comment will be wrong with eval scope objects being distinct from the global lexical scope object. ::: js/src/jit/BaselineCompiler.cpp @@ +281,5 @@ > + } else { > + Register scope = R1.scratchReg(); > + Label notGlobal; > + masm.branchTestObjClass(Assembler::NotEqual, scope, R0.scratchReg(), > + script->global().getClass(), &notGlobal); Align the second line of arguments with the "A" in "Assembler". @@ +589,5 @@ > masm.loadPtr(Address(callee, JSFunction::offsetOfEnvironment()), scope); > + > + Label notGlobal; > + masm.branchTestObjClass(Assembler::NotEqual, scope, R0.scratchReg(), > + script->global().getClass(), &notGlobal); Same alignment issue. ::: js/src/jit/BaselineIC.cpp @@ +5673,5 @@ > HandlePropertyName name) > { > + JS_ASSERT(IsLexicalScope(lexicalScope)); > + > + RootedObject global(cx, HopLexicalScopes(lexicalScope)); So this may make this code "work", in some sense. But without a modification to ICGetName_Global::Compiler::generateStubCode, I suspect this doesn't really work. The issue is this: what object does the IC *receive* when it's being evaluated? If you look at that method right now, it clearly expects to receive the global object. Per BaselineCompiler::emit_JSOP_GETGNAME(), I'm pretty sure that the IC is (now) being passed the lexical. So when the IC is hit, we enter with the wrong object, immediately fail the shape-guard test, hit stub-guard failure code, and over a little bit of failing every time, the stub eventually gets disabled and we lose. For now I think you can address this by just skipping up the scope chain to the global in that method as well, and things will be consistent. More careful code will be required to probe the lexical, then the global, when lets are put in the former. @@ +6490,5 @@ > RootedObject obj(cx, ToObjectFromStack(cx, val)); > if (!obj) > return false; > > + obj = HopLexicalScopes(obj); Hm, so this is invoked from getprop fallback code. I guess this is a consequence of our occasionally reusing JSOP_GETPROP code for name lookups, perhaps *only* in the JSOP_GETXPROP case. :-\ This looks like it might be closer to actually working with lexical scopes, in the JIT-generated code case. I think you'd hit the TryAttachNativeGetPropStub and |!isDOMProxy && IsCacheableGetPropReadSlot| path and successfully generate reasonable code for this case. I'd have to verify in a debugger to have much confidence in that assessment, tho. :-) Did you ever do so? Definitely worth doing. ::: js/src/jit/IonBuilder.cpp @@ +1015,5 @@ > } > } else { > // For CNG global scripts, the scope chain is the global object. > MOZ_ASSERT(script()->compileAndGo()); > + scope = constant(ObjectValue(script()->global().lexicalScope())); Comment needs correction. @@ +6439,5 @@ > return gc::GetGCKindSlots(kind, object->getClass()); > } > > bool > IonBuilder::getStaticName(JSObject *staticObject, PropertyName *name, bool *psucceeded) The "static" in here once referred to how this operated exactly on one object (global or call object), I believe. We should probably keep this in mind and change it to something better in subsequent patches. Or split this into one that does work on static objects, one that knows it may have to climb the prototype chain. @@ +6554,5 @@ > IonBuilder::setStaticName(JSObject *staticObject, PropertyName *name) > { > jsid id = NameToId(name); > > + JS_ASSERT(IsLexicalScope(staticObject) || staticObject->is<CallObject>()); I don't think simply changing argument type here is really enough to work here. If you look further in this code, we're doing a bunch of jsop_setprop(name) calls that work not upon the |staticObject| passed here, but rather upon the object that was pushed onto the stack. See this line later on: JS_ASSERT(&obj->toConstant()->value().toObject() == staticObject); Given we push the lexical scope all the places I can see, I suspect that this assertion can be hit and fail, with the right input. It's very unclear to me why this doesn't fail now with at least *some* of our test inputs. As for what's necessary to do: Does temporarily just grouping into JSOP_SETNAME/JSOP_SETPROP do the trick for correctness? We'll need to optimize that again later, but at least this keeps us moving. After that temporary fix: I think we should first copy this method into a setGlobalName method and eliminate the JSObject* argument to it. (I think we should do the same thing to getStaticName as well, although it's a somewhat less pressing concern there, because I don't think this problem quite exists there.) The other setStaticName caller that clearly passes a Call object can keep using setStaticName, because it really truly is acting directly upon one static object. Once that's done, then we need to go back and refactor this a bit to hop over the lexical storage object and do all its work on that. I don't think there's a minimal fix for this: it's going to require some rewriting. I would have to sit down and experiment with this a bit to figure out exactly what would need to be done for this. @@ +6600,5 @@ > > bool > IonBuilder::jsop_getgname(PropertyName *name) > { > + JSObject *obj = &script()->global().lexicalScope(); I'm somewhat leery of letting a lexical scope flow through this code this way. Can you just delegate to jsop_getname, without sacrificing too much performance in the short run? That at least is going to be safe, as name lookups intrinsically have to go through a scope chain of stuff and all in general. Longer run this needs similar tuning to what setname needed, but just to make forward progress that change is probably right for now. ::: js/src/jit/IonCaches.cpp @@ +2845,1 @@ > return false; The |inlinable| code path above here, being unaffected by this change, seems suspicious. Tracing through that, I suspect CanAttachNativeSetProp would return SetPropertyIC::MaybeCanAttachAddSlot, then we fall into the IsPropertyAddInlineable case below. But this code specifically only really handles property *additions*, apparently, per the "If we have a shape at this point and the object's shape changed" comment by an assertion that the shape for the id is the last property on the object. That's not necessarily the case here at all. (I'm also unsure if the IC would be given a lexical object or the global, when doing its thing, but I haven't really investigated it.) It looks to me like *if* we passed that point, we'd not be doing the "the inlined cache are setup for the next execution" bit of the comment above here. The SetProperty would work, and affect the right object. But we'd basically cache-miss every time because of lack of update, so very quickly this cache would amount to naught. As far as what needs changing. We need to do some scope chain walking here, to do much to fix this. From what I can tell, we don't really have an IC that's well-suited to all this, if we can't just throw the global object at it. So we probably need a new one. @@ +4039,5 @@ > bool > BindNameIC::attachGlobal(JSContext *cx, HandleScript outerScript, IonScript *ion, > HandleObject scopeChain) > { > + JS_ASSERT(HopLexicalScopes(scopeChain)->is<GlobalObject>()); Should just be able to assert IsLexicalScope(scopeChain) here, I think, with the eval scope change. @@ +4048,5 @@ > // Guard on the scope chain. > attacher.branchNextStub(masm, Assembler::NotEqual, scopeChainReg(), > ImmGCPtr(scopeChain)); > + // Get global from scope chain > + masm.movePtr(ImmGCPtr(HopLexicalScopes(scopeChain)), outputReg()); Huh. So I guess we get a lexical scope object through these ICs sometimes, but not always. Maybe? Perhaps that's why things seem not quite so much to be falling over, even wrt some of the inconsistencies in this patch. @@ +4191,5 @@ > > // Stop generating new stubs once we hit the stub count limit, see > // GetPropertyCache. > if (cache.canAttachStub()) { > + if (HopLexicalScopes(scopeChain)->is<GlobalObject>()) { IsLexicalScope should work here as well, with a custom eval scope object. ::: js/src/jsapi.cpp @@ +4797,5 @@ > options.setNoScriptRval(!rval); > SourceCompressionTask sct(cx); > + > + // Climb the scope chain to see if we hit a global before we hit a lexical object > + // If we do, we reparent the last thing to chain to the lexical instead Complete sentences should end with periods. And add on "This is an awful, awful hack that will go away when the parent chain is converted into a chain of scopes (where scope is not the same as object)." so we're clear that this is horrible. :-) ::: js/src/jscompartment.cpp @@ +367,5 @@ > // parent without being a proper global object (JSCLASS_IS_GLOBAL). Instead, > // we parent all wrappers to the global object in their home compartment. > // This loses us some transparency, and is generally very cheesy. > HandleObject global = cx->global(); > + RootedObject lexical(cx, &global->as<GlobalObject>().lexicalScope()); Change |HandleObject global| to |Handle<GlobalObject*> global| and get rid of the as-cast in here. @@ +426,5 @@ > RootedValue key(cx, ObjectValue(*obj)); > if (WrapperMap::Ptr p = crossCompartmentWrappers.lookup(CrossCompartmentKey(key))) { > obj.set(&p->value().get().toObject()); > JS_ASSERT(obj->is<CrossCompartmentWrapperObject>()); > + JS_ASSERT(obj->getParent() == lexical); Hmm, how do we know this? It looks to me like this is an implicit requirement on JSWrapObjectCallbacks, but I don't see anything enforcing it. @@ +437,5 @@ > if (!existing->getTaggedProto().isLazy() || > // Note: don't use is<ObjectProxyObject>() here -- it also matches subclasses! > existing->getClass() != &ProxyObject::uncallableClass_ || > existing->getParent() != global || > + existing->getParent() != lexical || I could be mistaken, but don't we want to do s/global/lexical/ here, rather than adding a separate check for lexical? We want to ignore the existing wrapper if it doesn't parent to the lexical, like the one we potentially create just below will. And speaking as a pure matter of logic. In the current world, any parent but the global will trigger nulling. In this new world, any parent but the global will trigger nulling, *then* as a second step any parent but the lexical will trigger nulling. If we get the lexical as parent, then we'll hit the first step and decide we can't use |existing|. So isn't this buggy just as a matter of plain logic? ::: js/src/jsfriendapi.cpp @@ +397,5 @@ > return &obj->global(); > } > > +JS_FRIEND_API(JSObject *) > +js::GetLexicalForObjectCrossCompartment(JSObject *obj) Name this GetGlobalEnvironmentRecord, please, to be consistent with the spec terminology (and thus the terminology JSAPI will use in the long run). @@ +403,5 @@ > + return &obj->global().lexicalScope(); > +} > + > +JS_FRIEND_API(JSObject *) > +js::NewWithObject(JSContext *cx, HandleObject object, HandleObject enclosing) { { goes on new line, and a few other places in this file. @@ +405,5 @@ > + > +JS_FRIEND_API(JSObject *) > +js::NewWithObject(JSContext *cx, HandleObject object, HandleObject enclosing) { > + StaticWithObject *staticWithObj = StaticWithObject::create(cx); > + RootedObject staticWith(cx, staticWithObj); Combine these into a single line. And because all object-creation methods can run out of memory, they're fallible, so you need to null-check and return nullptr if !staticWith. @@ +407,5 @@ > +js::NewWithObject(JSContext *cx, HandleObject object, HandleObject enclosing) { > + StaticWithObject *staticWithObj = StaticWithObject::create(cx); > + RootedObject staticWith(cx, staticWithObj); > + DynamicWithObject *dynamicWith = DynamicWithObject::create(cx, object, enclosing, staticWith); > + return dynamicWith; No need for a local if you're just immediately returning it. @@ +418,5 @@ > + > +JS_FRIEND_API(void) > +js::ChangeEnclosingScope(JSContext *cx, HandleObject obj, HandleObject newScope) > +{ > + JS_ASSERT(!obj->is<js::DebugScopeObject>()); MOZ_ASSERT in new code, please. (Eventually we'll switch everything to MOZ_ASSERT, just hasn't happened yet.) @@ +419,5 @@ > +JS_FRIEND_API(void) > +js::ChangeEnclosingScope(JSContext *cx, HandleObject obj, HandleObject newScope) > +{ > + JS_ASSERT(!obj->is<js::DebugScopeObject>()); > + if(obj->is<js::ScopeObject>()) Space between 'if' and '('. @@ +422,5 @@ > + JS_ASSERT(!obj->is<js::DebugScopeObject>()); > + if(obj->is<js::ScopeObject>()) > + obj->as<js::ScopeObject>().setEnclosingScope(newScope); > + else > + JSObject::setParent(cx, obj, newScope); This method is fallible -- returning a bool success/failure value. It's not okay to drop that on the ground. So it should be propagated into the function's return value. (Or rather, into the alternative method I suggest you add, instead of this method.) ::: js/src/jsfriendapi.h @@ +700,5 @@ > JS_FRIEND_API(JSObject *) > GetGlobalForObjectCrossCompartment(JSObject *obj); > > +JS_FRIEND_API(JSObject *) > +GetLexicalForObjectCrossCompartment(JSObject *obj); // Gets the lexical scope used to store let variables at top level in global code. Don't use this as an object -- only as a scope! @@ +702,5 @@ > > +JS_FRIEND_API(JSObject *) > +GetLexicalForObjectCrossCompartment(JSObject *obj); > + > +// Create a with object Slight wording tweaks, we don't want people thinking this is a good thing to use, just yet. (Or ever, but once it's cleaned up API-wise, at least it's understandable.) Something like this maybe: Create a scope object for for |obj| that acts like |with (obj) ...|. This object must only be passed as a scope to calls to JS_EvaluateScript and similar -- don't treat it as an actual object that can be safely exposed to script or the like! @@ +708,5 @@ > +NewWithObject(JSContext *cx, JS::HandleObject object, JS::HandleObject enclosing); > + > +// Create a call object > +JS_FRIEND_API(JSObject *) > +NewCallObject(JSContext *cx, JS::HandleScript script, JS::HandleObject enclosing); Actually, with no users yet, let's just remove this. Trying to describe what it does for a comment, I realized I don't actually quite know, which is...um...kind of a warning sign. :-) It may well be that this is something for which no valid use case exists. @@ +710,5 @@ > +// Create a call object > +JS_FRIEND_API(JSObject *) > +NewCallObject(JSContext *cx, JS::HandleScript script, JS::HandleObject enclosing); > + > +// Change an object's enclosing scope (cannot be a DebugScopeObject) Add a period, and note (as above) that this is an awful hack. ::: js/src/jsfun.cpp @@ +1699,2 @@ > RootedFunction fun(cx, NewFunctionWithProto(cx, js::NullPtr(), nullptr, 0, > + JSFunction::INTERPRETED_LAMBDA, lexicalScope, In theory this is no longer necessary with the parent-mutation thing, right? (But leave it, this is the direction we want to head anyway.) ::: js/src/jsobj.cpp @@ +4479,5 @@ > RootedObject pobj(cx); > RootedShape prop(cx); > > RootedObject scope(cx, scopeChain); > + for (; !(scope->is<GlobalObject>() || (IsLexicalScope(scope) && scope->enclosingScope()->is<GlobalObject>())); scope = scope->enclosingScope()) { This condition looks wrong. Why shouldn't it remain just as it was before? Doesn't reparenting, and proper parenting of actual scopes created, to chain to the lexical and then global, mean this change is unnecessary? I think this should stay as it was before, basically. @@ +4500,5 @@ > RootedObject pobj(cx); > RootedShape prop(cx); > > RootedObject scope(cx, scopeChain); > + for (; !(scope->isUnqualifiedVarObj() || (IsLexicalScope(scope) && scope->enclosingScope()->isUnqualifiedVarObj())); scope = scope->enclosingScope()) { It doesn't seem to me this needs to change, either. ::: js/src/jsobj.h @@ +1408,5 @@ > LookupNameNoGC(JSContext *cx, PropertyName *name, JSObject *scopeChain, > JSObject **objp, JSObject **pobjp, Shape **propp); > > /* > + * Like LookupName except returns the lexical object if 'name' is not found in "the global lexical scope" is probably a better way to put it. @@ +1420,1 @@ > MutableHandleObject objp); Fix up the indentation. ::: js/src/jsscript.h @@ +1175,5 @@ > > bool warnedAboutUndefinedProp() const { return warnedAboutUndefinedProp_; } > void setWarnedAboutUndefinedProp() { warnedAboutUndefinedProp_ = true; } > > + // Whether we have a let variable. We need to knew this so know ::: js/src/vm/Debugger.cpp @@ +115,5 @@ > return false; > } > > static bool > +ValueToName(JSContext *cx, HandleValue v, MutableHandle<PropertyName*> name) I believe you should be able to simply modify ValueToIdentifier to return its name via this kind of outparam, and maybe modify one or two callers to convert the returned name into a jsid using NameToId. There's no need/desire to have this method and ValueToIdentifier doing nearly exactly the same things. *Particularly* not when there's a call to one immediately adjacent a call to the next one. @@ +4945,4 @@ > /* If evalWithBindings, create the inner environment. */ > if (evalWithBindings) { > /* TODO - This should probably be a Call object, like ES5 strict eval. */ > + env = NewObjectWithGivenProto(cx, &JSObject::class_, nullptr, newLexicalScope); Hmm. I can't honestly tell if this was violating the so-called invariant in GetDebugScope or not. I guess probably not, because it seems inconceivable that this object would never have been passed through it. Still, a violation of that old (now modified) invariant. I am not actually sure how bad it is to have this interleaving for absolute sure in the new code. Again, need to consult people more familiar with this code. @@ +6007,5 @@ > if (!envobj) \ > return false; \ > Rooted<Env*> env(cx, static_cast<Env *>(envobj->getPrivate())); \ > JS_ASSERT(env); \ > + JS_ASSERT(!env->is<ScopeObject>() || IsLexicalScope(env)) I guess maybe it wasn't violating the invariant before, if a checkThis'd object's private was for sure not a ScopeObject before. Well, that's at least comforting regarding the old code. @@ +6204,5 @@ > RootedId id(cx); > if (!ValueToIdentifier(cx, args[0], &id)) > return false; > + Rooted<PropertyName*> name(cx); > + if (!ValueToName(cx, args[0], &name)) Just use NameToId to get an id from this, no need for the ValueToIdentifier above. @@ +6262,5 @@ > + /* Now we run up the scope chain to find which object, if any, has this variable. */ > + bool has = false; > + for (; env; env = env->enclosingScope()) { > + if (!JSObject::hasProperty(cx, env, id, &has)) > + return false; Huh, this is a mildly interesting pre-existing bug. Because hasProperty walks the entire prototype chain asking this question, if it hits a proxy along the way, this can invoke stuff. Which I guess "trigger setters" above already sort of acknowledged. Oh well, this'll get the job done not worse than before. Wait. Now I see your test that says we changed the behavior of setVariable to walk the *entire* chain, not just examine that single environment. Bleah. That's no good. We really do still want to affect only that single environment. It's just that *if* it's the lexical scope, we want to check there and one further along. Given the change to make the lexical scope its own standalone environment in the chain of DebuggerFrames, I think what this means is that the old code was correct for these. Or is my brain addled and I'm mistaken because it's late'o'clock here as I write this very last comment on this patch? ::: js/src/vm/GlobalObject.cpp @@ +232,5 @@ > return nullptr; > > Rooted<GlobalObject *> global(cx, &obj->as<GlobalObject>()); > > cx->compartment()->initGlobal(*global); Should this go after the global->setReservedSlot in the added code? I could imagine this needing to be either way, to make other code work correctly. Just want to be sure this chosen location is the correct/sensible one and that we chose it deliberately because it was the right place. ::: js/src/vm/Interpreter-inl.h @@ +211,5 @@ > return true; > } > > +static inline bool > +NameOperation(JSContext *cx, JSObject *obj, PropertyName *name, jsbytecode *pc, MutableHandleValue vp) HandleObject for the obj parameter. You'll have to add a RootedObject in the method to deal with the global-scope case (maybe rename the parameter to |scope| to make scope-ness clearer), but that's okay. Also HandlePropertyName for name. (Generally we want to use handle types for everything we can, except in rare cases local variables in tightly-constrained circumstances.) @@ +216,5 @@ > +{ > + > + /* > + * Skip along the scope chain to the enclosing global object. This is > + * used for GNAME opcodes where the bytecode emitter has determined a * name access must be on the global. It also insulates us from bugs Rewrap. Rather than "a name access must be on the global", maybe say "the name access will occur in the global environment record", since "global" implies that particular object, which won't be the case when we're done with all this. @@ +220,5 @@ > + * used for GNAME opcodes where the bytecode emitter has determined a * name access must be on the global. It also insulates us from bugs > + * in the emitter: type inference will assume that GNAME opcodes are > + * accessing the global object, and the inferred behavior should match > + * the actual behavior even if the id could be found on the scope chain > + * before the global object. This insulate-from-bugs thing sounds only dubiously true, if at all, once we're done here. A pitfall to be wary of in future patching. @@ +293,5 @@ > baseops::Unqualified, &valCopy, > strict); > } > > return JSObject::setProperty(cx, scope, scope, name, &valCopy, strict); Add this here: MOZ_ASSERT(!IsLexicalScope(scope), "the global lexical scope's enclosing scope is a global where" "isUnqualifiedVarObj(), per GlobalObject::create"); to document that no lexical scope ever flows into the generic property-setting path. ::: js/src/vm/Interpreter.cpp @@ +600,5 @@ > JS_ASSERT_IF(!s->enclosingScope(), s->is<GlobalObject>()); > } while ((s = s->enclosingScope())); > #endif > > + /* Use the scope chain as 'this', modulo outerization. However, /* * ... style nit. @@ +616,5 @@ > return false; > } > > + if (scopeChain->is<GlobalObject>()) > + scopeChain = GetInnerObject(&scopeChain->as<GlobalObject>().lexicalScope()); I don't think you need the GetInnerObject call around this. Inner and outer objects are mostly only relevant to the global object, and to |with| objects. The lexical scope is a far purer concept. ;-) @@ +618,5 @@ > > + if (scopeChain->is<GlobalObject>()) > + scopeChain = GetInnerObject(&scopeChain->as<GlobalObject>().lexicalScope()); > + > + return ExecuteKernel(cx, script, *scopeChain, thisv.get(), EXECUTE_GLOBAL, You don't need .get() here. @@ +2344,2 @@ > MutableHandleValue lval = REGS.stackHandleAt(-1); > + RootedValue newLval(cx); We have spare roots in Interpret ready for use, so we don't have to push/pop extra roots in all the different ops. So do this instead (which also cuts down some on duplication here): { RootedValue &newLval = rootValue0; MutableHandleValue lval = REGS.stackHandleAt(-1); if (*REGS.pc == JSOP_GETXPROP && lval.isObject() && HopLexicalScopes(&lval.toObject())->is<GlobalObject>()) { newLval.setObject(lval.toObject().global()); } else { newLval = lval; } if (!GetPropertyOperation(cx, REGS.fp(), script, REGS.pc, newLval, lval)) goto error; TypeScript::Monitor(cx, script, REGS.pc, newLval); assertSameCompartmentDebugOnly(cx, newLval); } Either that, *or* split JSOP_GETXPROP out into its own opcode section here. Which is probably worthwhile, actually, given our discussion about making this into a GetNameOperation sort of thing. @@ +2385,5 @@ > HandleValue value = REGS.stackHandleAt(-1); > > + JSObject* fpsScope = REGS.fp()->scopeChain(); > + JSObject* fpsScope2 = activation.regs().fp()->scopeChain(); > + Remove this. @@ +2399,5 @@ > { > HandleValue lval = REGS.stackHandleAt(-2); > HandleValue rval = REGS.stackHandleAt(-1); > > + if (lval.isObject() && HopLexicalScopes(&lval.toObject())->is<GlobalObject>()) { Apply the same sort of treatment here, that you applied to the JSOP_GETXPROP case above, to use rootValue0 setObject(global) and share all the SetPropertyOperation bits in the tail of this between the two cases. @@ +2442,5 @@ > FETCH_OBJECT(cx, -3, obj); > RootedId &id = rootId0; > FETCH_ELEMENT_ID(-2, id); > Value &value = REGS.sp[-1]; > + if (HopLexicalScopes(obj)->is<GlobalObject>()) { Same sort of tail-sharing comment again. @@ +2634,5 @@ > RootedObject &scopeObj = rootObject0; > scopeObj = REGS.fp()->scopeChain(); > > RootedObject &scope = rootObject1; > + if (!FindScopeForName(cx, name, scopeObj, &scope)) Hmm. FindScopeContainingName seems like a better name, reading in context here. Make the name change? @@ +2649,5 @@ > CASE(JSOP_NAME) > { > RootedValue &rval = rootValue0; > > + if (!NameOperation(cx, REGS.fp()->scopeChain(), REGS.fp()->script()->getName(REGS.pc), REGS.pc, &rval)) Use some locals (references to the rootValue0, rootObject0, etc. sorts of things) for all these arguments. Too much noise on the line otherwise. ::: js/src/vm/ScopeObject.cpp @@ +610,5 @@ > > /*****************************************************************************/ > > +ExtensibleBlockObject* > +CreateExtensibleBlockObject(JSContext *cx, js::HandleObject enclosingScope) { { on new line. @@ +615,5 @@ > + ExtensibleBlockObject *lexObj = ExtensibleBlockObject::create(cx); > + if (!lexObj) > + return nullptr; > + js::Rooted<ExtensibleBlockObject*> lexicalScope(cx, lexObj); > + lexicalScope->setEnclosingScope(enclosingScope); I don't think you need the Rooted at all here. Just do ExtensibleBlockObject *lexObj = ExtensibleBlockObject::create(cx); if (!lexObj) return nullptr; lexObj->setEnclosingScope(enclosingScope); return lexObj; If you did need it, you'd want to make |lexObj| itself a Rooted. @@ +678,5 @@ > } > } > > +ExtensibleBlockObject * > +ExtensibleBlockObject::create(ExclusiveContext *cx) It doesn't appear to me there's any reason to have both this method, and CreateExtensibleBlockObject. There should only be one method -- I guess this one -- that performs this one unified task. @@ +2306,5 @@ > /* > * As an engine invariant (maintained internally and asserted by Execute), > * ScopeObjects and non-ScopeObjects cannot be interleaved on the scope > * chain; every scope chain must start with zero or more ScopeObjects and > * terminate with one or more non-ScopeObjects (viz., GlobalObject). *gulp* about changing this invariant. It doesn't actually hold any more, if you think about cases like ExecuteInGlobalAndReturnScope -- which sets up a {plain old object} -> lexical scope -> global object interleaving. Same is probably true for the other screwball case we were looking at, subscript loading scopes. The consequences of this change are very unclear to me. :-\ We definitely need to be very careful about doing this. In the worst extreme, where it's not feasible to change this, it might be necessary to add that WithSupportingVariablesObject scope object, to use in at least those two cases, to re-establish this invariant. We need to consult people with better understanding of this invariant, to be sure it's okay to change it. @@ +2313,2 @@ > #ifdef DEBUG > + // Note that we can still get lexical objects on the scope chain here - we could have stopped on a Proxy that chains to a lexical. Wrap at 80ch, please. (And, um, lovely.) ::: js/src/vm/ScopeObject.h @@ +938,5 @@ > return is<js::BlockObject>() && !getProto(); > } > > +js::ExtensibleBlockObject* > +CreateExtensibleBlockObject(JSContext *cx, js::HandleObject enclosingScope); This is a fine method to have. But for our purposes in this patch, I think we really want to be using a CreateGlobalLexicalScope method: js::ExtensibleBlockObject * js::CreateGlobalLexicalScope(JSContext *cx, JS::Handle<GlobalObject*> global); That method can then directly call CreateExtensibleBlockObject to do its work, while giving clear indication (and typewise enforcement) of the exact, narrow purpose of the method, and that it will *only* create objects of this nature with a global as enclosing scope. @@ +947,5 @@ > + return object->is<js::ExtensibleBlockObject>(); > +} > + > +inline JSObject * > +HopLexicalScopes(JSObject *scope) { { on new line. All three (four, after my extra addition) methods here should be in namespace js. Let's name this SkipOverLexicalScopes, actually. ::: js/src/vm/Stack.cpp @@ +276,5 @@ > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) > DebugScopes::onPopStrictEvalScope(this); > } else if (isDirectEvalFrame()) { > if (isDebuggerFrame()) > + JS_ASSERT(IsLexicalScope(scopeChain()) || !scopeChain()->is<ScopeObject>()); Mild preference for other ordering of the conditions here. Just because currently, the "not a scope chain" bit is the common case for debugger frames, and the is-lexical case is the exception to it. @@ +288,5 @@ > */ > if (isDebuggerFrame()) { > JS_ASSERT(scopeChain()->is<GlobalObject>() || > + IsLexicalScope(scopeChain()) || > + IsLexicalScope(scopeChain()->enclosingScope())); // Could also check that the lexicalScopes end up at a global... I don't think we need the "could also" check, or a comment on its absence. We just need to ensure that we only ever create lexical scopes whose enclosing scope is the global object. We can and should enforce that in CreateGlobalLexicalScope. @@ +293,2 @@ > } else { > + JS_ASSERT(HopLexicalScopes(scopeChain())->is<GlobalObject>()); This can't be IsLexicalScope(scopeChain())? @@ +297,5 @@ > return; > } > > if (isGlobalFrame()) { > + JS_ASSERT(IsLexicalScope(scopeChain()) || !scopeChain()->is<ScopeObject>()); Isn't IsLexicalScope always the case with this patch in place, now? Don't want to over-allow things in assertions, partly because of it allowing bugs, but mostly because it makes for bad documentation of the possible runtime states that might exist. And this one's a bit confusing, observed several weeks after the last time I looked at this code or discussed it deeply with you or truly understood it in any sense. ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +327,5 @@ > cachePath.AppendPrintf("jssubloader/%d", version); > PathifyURI(uri, cachePath); > > + // Climb the scope chain to see if we hit a global before we hit a lexical object > + // If we do, we reparent the last thing to chain to the lexical instead This all looks very similar to what's in...<checks rest of patch>...Evaluate in jsapi.cpp. Rather than exposing ChangeEnclosingScope as the primitive operation, instead expose extern bool js::EnsureScopeChainsToLexical(JSContext *cx, JS::HandleObject obj); and then use that. With a comment about "Hack! We should instead use a custom kind of environment akin to WithObject, but where declared variables are added onto |targetObj| rather than onto the WithObject itself." to be clear this is screwball code. (Comment *all* the screwball code as being screwball! :-) ) ::: toolkit/devtools/server/tests/unit/test_framebindings-02.js @@ +40,5 @@ > do_check_neq(parentEnv, undefined); > let objClient = gThreadClient.pauseGrip(parentEnv.object); > objClient.getPrototypeAndProperties(function(aResponse) { > + print("THIS BETTER PRINT"); > + print(aResponse); Don't add this.

Please make sure that this doesn't allow shadowing things like "location" on the global on the web.

(In reply to Boris Zbarsky [:bz] from comment #14) > Please make sure that this doesn't allow shadowing things like "location" on > the global on the web. Why is this an issue any more? I thought plugins had better ways to access origin information these days, and I thought they were the only user depending upon location to be accurate. (I had thought the spec actually handled this via the redeclaration stuff, but a closer look at the GlobalDeclarationInstantiation algorithm shows that it checks for redeclarations using HasVarDeclaration and HasLexicalDeclaration. While the latter consults the lexical storage space for the instantaneous set of lexical declarations [including past additions], the former looks purely at actual var declarations in the current script. So there is an issue along these lines to resolve. But it's not clear to me *why* it's an issue, any more.)

> I thought plugins had better ways to access origin information these days They do. Do they all use it? I have no idea, and would rather not introduce security bugs.... Note the es-discuss thing about this issue, for "undefined" as well, today.

I'm pretty sure I've seen us treat multiple cases of location being shadowable, or modifiable, or whatever, as security issues for awhile now, at least a couple few years. And any plugin that hasn't been updated for this (...we could have telemetry to detect instances of this, right? any reason other than laziness/no one's thought of it that we don't, so as to understand the extent of this problem, if indeed it is one?), probably hasn't been provided security updates anyway for other issues, so it seems like water under the bridge to me. Frankly, I don't understand the es-discuss complaint about |undefined|. It seems conceptually straightforward to introduce a constraint on the lexical storage object not acquiring a property of a given name, for each name assumed to refer to a global property. The idea is basically what we do for making optimized typed array accesses deal with neutering -- we add a constraint on the typed array data not going away, then if someone neuters the underlying ArrayBuffer, we invalidate all JIT code that depended on that constraint. I don't see any reason why global accesses in general must be much slower to account for global let-variables.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17) > I'm pretty sure I've seen us treat multiple cases of location being > shadowable, or modifiable, or whatever, as security issues for awhile now, > at least a couple few years. ...seen us *not* treat...

Bobby, do you know exactly what the location situation is?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15) > Why is this an issue any more? I thought plugins had better ways to access > origin information these days, and I thought they were the only user > depending upon location to be accurate. Yes, but they don't always use it, last dveditz and I checked. If we're going to suddenly decide to get rid of the whole [Unforgeable] location stuff, that warrants a separate, very thorough discussion.

Any ETA for this?

(In reply to Florian Bender from comment #21) > Any ETA for this? I do not know an ETA, but I am working on this several hours a day again. I'd like to apologise for my lack of progress over the past months, I was busier then I had anticipated and ended up procrastinating on this. I'm very sorry for if I held anyone up or have caused problems by my lack of progress. During winter break I'm trying to get as much done on this as I can and have finished making a first pass of code modification through Waldo's review, and was actually preparing to post a revised patch in 2 or 3 days.

Ok, I made a first run though Waldo's review and got everything compiling again and wanted to post a wip. I haven't rebased yet; I'm not sure if we should do testing before we rebase. Below I made notes on on the review when ever I had questions or notes. As I have yet to rebase, it should be noted that I have this patch on top of changeset: 199438:5299864050ee I may have missed some of the minor spacing stuff, but in the interest of getting moving, I'd like to put this up now and fix that next patch. I've also left some code commented out pending testing, I will definitly remove this afterwards ::: js/src/jit-test/tests/debug/Environment-setVariable-08.js Has also been left the same pending discussion - I had thought that this was actually how we wanted it. Need to make sure. ::: js/src/jit/BaselineIC.cpp @@ +5673,5 @@ Nothing here yet - it's been too long for me to mess with this yet. ::: js/src/jit/BaselineIC.cpp @@ +6490,5 @@ Supposed to check this one to make sure it works, that can wait too. ::: js/src/jit/IonBuilder.cpp @@ +6554,5 @@ I don't understand the change to be made here. ::: js/src/jit/IonBuilder.cpp @@ +6600,5 @@ Should delagate to jsop_getname, but I'm not sure where this change is supposed to be made. ::: js/src/jit/IonCaches.cpp @@ +2845,1 @@ New IC possibly? This needs work. ::: js/src/jit/IonCaches.cpp @@ +4039,5 @@ Made, but old code commented out just in case. ::: js/src/jit/IonCaches.cpp @@ +4191,5 @@ Ditto for here ::: js/src/jscompartment.cpp @@ +426,5 @@ I'm not sure what should be done here. ::: js/src/jsfriendapi.h @@ +710,5 @@ Not sure if I'm supposed to remove NewWithObject and NewCallObject or just NewCallObject. For now I have just removed NewCallObject. ::: js/src/jsobj.cpp @@ +4479,5 @@ @@ +4500,5 @@ These have been changed, but the old code has been left commented. ::: js/src/vm/Debugger.cpp @@ +115,5 @@ @@ +6262,5 @@ There was only one caller of ValueToName and several callers to ValueToIdentifier, and as the toName part was an extra step on top of the ValueToIdentifier, I left ValueToIdentifier as it was and fixed up the one caller to use JSID_TO_ATOM(id)->asPropertyName()) ::: js/src/vm/Debugger.cpp @@ +6262,5 @@ Same as with js/src/jit-test/tests/debug/Environment-setVariable-08.js, I thought that this was actually what we wanted. Need to double check. ::: js/src/vm/GlobalObject.cpp @@ +232,5 @@ I have a feeling that this is right, but I can't remember ::: js/src/vm/Stack.cpp @@ +293,2 @@ Kept old code commented out @@ +297,5 @@ ditto

NI myself to look at the patch.

Sorry for the double update - I accidentally hit enter while uploading the other patch. So the new patch just fixes one set of missing perens in a JS_ASSERT that I missed earlier. Here are the patch failures without the patch: REGRESSIONS js1_8_5/extensions/findReferences-02.js TIMEOUTS ecma_5/Object/15.2.3.6-middle-redefinition-4-of-8.js ecma_5/Object/15.2.3.6-redefinition-3-of-4.js ecma_5/Object/15.2.3.6-middle-redefinition-3-of-8.js ecma_5/Object/15.2.3.6-redefinition-1-of-4.js ecma_5/Object/15.2.3.6-redefinition-2-of-4.js FAIL And with the patch: REGRESSIONS ecma/Number/15.7.4.2-4.js ecma/Number/15.7.4.3-2.js ecma/Boolean/15.6.4.3-3.js ecma/Boolean/15.6.4.2-3.js js1_8_1/strict/8.7.2.js js1_8_1/regress/regress-420399.js ecma_5/strict/unbrand-this.js ecma_5/strict/15.5.5.1.js js1_8_5/extensions/findReferences-02.js js1_8_5/regress/regress-541255-2.js js1_8_5/regress/regress-541255-4.js js1_8_5/regress/regress-541255-1.js js1_8_5/regress/regress-541255-0.js js1_5/Regress/regress-420919.js ecma_6/Symbol/as-base-value.js TIMEOUTS ecma_5/Object/15.2.3.6-redefinition-3-of-4.js ecma_5/Object/15.2.3.6-middle-redefinition-2-of-8.js ecma_5/Object/15.2.3.6-dictionary-redefinition-17-of-32.js ecma_5/Object/15.2.3.6-middle-redefinition-5-of-8.js ecma_5/Object/15.2.3.6-middle-redefinition-3-of-8.js ecma_5/Object/15.2.3.6-redefinition-1-of-4.js ecma_5/Object/15.2.3.6-dictionary-redefinition-14-of-32.js ecma_5/Object/15.2.3.6-dictionary-redefinition-21-of-32.js ecma_5/Object/15.2.3.6-redefinition-2-of-4.js ecma_5/Object/15.2.3.6-redefinition-4-of-4.js FAIL So we have definitely reintroduced some bugs with the review changes. Thanks for the need info, Shu, I want to do what ever I can but am also pretty swamped with school.

I would like the lexical scope we're adding under the global to participate in the static scope chain. After all, it is an actual lexical scope, so it should participate in the static scope chain. As such, I'd like to take the opportunity to tighten our scope chain invariant story. Currently, our invariant is: - The static scope chain determines the dynamic scope chain of *syntactic ScopeObjects*. - The dynamic scope chain is terminated by 1+ non-syntactic ScopeObjects. With the addition of the lexical scope, I'd like to reformulate this to: - The static scope chain determines the entirety of the dynamic scope chain up to the terminating GlobalObject. That is, the static scope will always be of the form StaticGlobalBlockObject (the global lexical) | 0+ StaticWithObjects (for the non-syntactic WithObjects) | 0+ ScopeObjects (the syntactic static scopes) - The dynamic scope chain will always be of the form GlobalObject | ExtensibleBlockObject (the global lexical) | 0+ non-syntactic WithObjects | 0+ syntactic ScopeObjects Boris, with your parent changes, is the above proposal feasible? Right now, are all JSScripts created by DOM objects on a scope chain with 0+ non-syntactic WithObjects followed by a GlobalObject?

> Boris, with your parent changes, is the above proposal feasible? So first of all, the current invariant as stated is not quite right. Things that can appear on the dynamic scope chain include, in addition to ScopeObjects and GlobalObjects, at least DebugScopeObject (which is not a ScopeObject) and that object that js::ExecuteInGlobalAndReturnScope sticks on there (I never managed to convert that to a non-syntactic With object, because our handling of the unqualified var obj has assumptions that don't hold for With objects). I think bug 1143794 has a description of our current understanding of what our current scope chain is allowed to be.

What still needs to be done is non-syntactic scope stuff (Gecko use) and vetting chrome code.

Comment on attachment 8655700 [details] [diff] [review] Make a global lexical scope and hook it up to JS entry points. Review of attachment 8655700 [details] [diff] [review]: ----------------------------------------------------------------- OK. ::: js/src/asmjs/AsmJSLink.cpp @@ +877,5 @@ > ? SourceBufferHolder::GiveOwnership > : SourceBufferHolder::NoOwnership; > SourceBufferHolder srcBuf(chars, end - begin, ownership); > + Rooted<ScopeObject*> staticLexical(cx, &cx->global()->lexicalScope().staticBlock()); > + if (!frontend::CompileFunctionBody(cx, &fun, options, formals, srcBuf)) Where is this new scope object used? We are not passing it in? This seems odd. ::: js/src/frontend/BytecodeCompiler.cpp @@ +937,5 @@ > + const ReadOnlyCompileOptions& options, > + Handle<PropertyNameVector> formals, JS::SourceBufferHolder& srcBuf) > +{ > + Rooted<ScopeObject*> staticLexical(cx, &cx->global()->lexicalScope().staticBlock()); > + return CompileFunctionBody(cx, fun, options, formals, srcBuf, staticLexical, NotGenerator); OK, so the rooted above is just dead. Please remove it. @@ +946,5 @@ > frontend::CompileStarGeneratorBody(JSContext* cx, MutableHandleFunction fun, > const ReadOnlyCompileOptions& options, > Handle<PropertyNameVector> formals, > JS::SourceBufferHolder& srcBuf) > +{ Rooted<ScopeObject*> staticLexical(cx, &cx->global()->lexicalScope().staticBlock()); nit: formatting ::: js/src/jsscript.cpp @@ +3438,5 @@ > JSScript* > js::CloneGlobalScript(JSContext* cx, Handle<ScopeObject*> enclosingScope, HandleScript src) > { > + MOZ_ASSERT(IsGlobalLexicalScope(enclosingScope) || > + enclosingScope->is<StaticNonSyntacticScopeObjects>()); Is this right? Shouldn't this be IsStaticGlobalLexicalScope? ::: js/src/vm/GlobalObject.h @@ +92,5 @@ > FROM_BUFFER_FLOAT64, > FROM_BUFFER_UINT8CLAMPED, > > /* One-off properties stored after slots for built-ins. */ > + LEXICAL_SCOPE, As an aside, if there's an enum taking care of counting slot numbers, then why do we need to manually update the slot count macro above? ::: js/src/vm/ScopeObject.cpp @@ +697,5 @@ > +{ > + Rooted<StaticBlockObject*> staticLexical(cx, StaticBlockObject::create(cx)); > + if (!staticLexical) > + return nullptr; > + // Currently the global lexical scope cannot have any bindings with frame nit: newline before comment @@ +704,5 @@ > + staticLexical->initEnclosingScope(nullptr); > + Rooted<ClonedBlockObject*> lexical(cx, ClonedBlockObject::create(cx, staticLexical, global)); > + if (!lexical) > + return nullptr; > + if (!JSObject::setSingleton(cx, lexical)) You said this was for ion purposes. That's maybe worth a comment here? Maybe not, it does kinda make sense. @@ +863,5 @@ > + nullptr, /* deleteProperty */ > + nullptr, nullptr, /* watch/unwatch */ > + nullptr, /* getElements */ > + nullptr, /* enumerate (native enumeration of target doesn't work) */ > + block_ThisObject, "you will find the princess is the highest room in the tallest tower" ::: js/src/vm/Stack.cpp @@ +261,5 @@ > + // Confusingly, global frames may run in non-global scopes (that is, > + // not directly under the GlobalObject and its lexical scope). > + // > + // Gecko often runs global scripts under custom scopes. See users of > + // CreateNonSyntacticScopeChain. wat? That's clownshoes. Also totally not your fault.

Comment on attachment 8655701 [details] [diff] [review] Parse and emit bytecode for global lexicals. Review of attachment 8655701 [details] [diff] [review]: ----------------------------------------------------------------- r=me with both the class tests I have yet to attach to this bug, and the fix you showed me while we were doing the review. ::: js/src/frontend/BytecodeCompiler.cpp @@ +572,5 @@ > + // > + // Non-syntactic scripts gets a non-extensible lexical scope by > + // design. Having non-syntactic scripts modifying the global lexical scope > + // is confusing. > + if (isEvalCompilationUnit() || isNonSyntacticCompilationUnit()) { Ignoring for now, as per reviewee request ::: js/src/frontend/BytecodeEmitter.cpp @@ +1357,5 @@ > + // 'eval' and non-syntactic scripts are always under an invisible lexical > + // scope, but since it is not syntactic, it should still be considered at > + // body level. > + if (sc->staticScope()->is<StaticEvalObject>() || > + sc->staticScope()->is<StaticNonSyntacticScopeObjects>()) Ignoring, as above. @@ +1638,5 @@ > > JSOp op; > switch (pn->getOp()) { > + case JSOP_GETNAME: op = JSOP_GETGNAME; break; > + case JSOP_SETNAME: op = strictifySetNameOp(JSOP_SETGNAME); break; nit: we don't actually need to add spacing between the colon and the assignment here, right? ::: js/src/frontend/FullParseHandler.h @@ +119,5 @@ > ParseNode* newName(PropertyName* name, uint32_t blockid, const TokenPos& pos, > ExclusiveContext* cx) > { > + ParseNode* node = new_<NameNode>(PNK_NAME, JSOP_GETNAME, name, blockid, pos); > + return node; This seems like an unnecessary change. @@ +984,5 @@ > } else { > pn->pn_expr = init; > } > > + if (pn->pn_dflags & PND_BOUND) OK, so this is removed because we don't care what we set it to here, right? Because we will call setLexicalDeclarationOp below and smash it again? ::: js/src/frontend/Parser.cpp @@ +768,2 @@ > Directives directives(options().strictOption); > + GlobalSharedContext globalsc(context, staticLexical, directives, Shouldn't this hunk be in the last patch? Oh well. @@ +1306,5 @@ > letData_.overflow = overflow; > } > > + void initVar(JSOp op) { > + init(VarBinding, op, false); We coiuld consider making the const-ness part of lesData, if we were feeling frisky, here. @@ -4063,5 @@ > > - /* > - * SpiderMonkey const is really "write once per initialization evaluation" > - * var, whereas let is block scoped. ES-Harmony wants block-scoped const so > - * this code will change soon. haha. I wonder how old that comment was :P ::: js/src/jsfun.cpp @@ +591,2 @@ > fun = NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > + globalLexical, nullptr, proto, Should land with the previous patch.

Comment on attachment 8655702 [details] [diff] [review] Support global lexicals in the interpreter. Review of attachment 8655702 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfun.cpp @@ +590,2 @@ > fun = NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > + /* enclosingDynamicScope = */ nullptr, nullptr, proto, OK, this and the XDR stuff in the last patch should make up their minds. ::: js/src/jsscript.cpp @@ +3451,5 @@ > > JSScript* > js::CloneGlobalScript(JSContext* cx, Handle<ScopeObject*> enclosingScope, HandleScript src) > { > + MOZ_ASSERT(IsStaticGlobalLexicalScope(enclosingScope) || This fix should live in the patch in which I commented on it. ::: js/src/vm/Debugger.cpp @@ +6337,5 @@ > * boundaries, and we are putting a DebugScopeProxy or non-syntactic With on > * the scope chain. > */ > Rooted<ScopeObject*> enclosingStaticScope(cx); > + if (!IsGlobalLexicalScope(env)) { Did we need this before in this patch stack? I assume so.

Comment on attachment 8655706 [details] [diff] [review] Fix eval static scope to play with the global lexical scope. Review of attachment 8655706 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8655697 [details] [diff] [review] Cleanup: remove unused DEPTH_SLOT from BlockObject. Review of attachment 8655697 [details] [diff] [review]: ----------------------------------------------------------------- APPROVED

Comment on attachment 8655709 [details] [diff] [review] Fix jit-tests. Review of attachment 8655709 [details] [diff] [review]: ----------------------------------------------------------------- You should also run js/src/tests/js_1_8_5/reflect-parse/ and make sure nobody is looking for Vars or Lets there in the AST. I think the classes tests might.

Comment on attachment 8655704 [details] [diff] [review] Support global lexicals in Baseline. Review of attachment 8655704 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineCompiler.cpp @@ +2149,3 @@ > } > > return emit_JSOP_BINDNAME(); We probably also want a stub for BINDNAME now? Followup seems fine if these patches don't regress benchmarks. @@ +2820,5 @@ > + frame.popRegsAndSync(1); > + frame.push(ObjectValue(script->global().lexicalScope())); > + frame.push(R0); > + emit_JSOP_SETPROP(); > + return true; This should be `return emit_JSOP_SETPROP();` ::: js/src/jit/BaselineIC.cpp @@ +6849,5 @@ > MOZ_CRASH("Bad stub kind"); > } > } > > +void Nit: static void @@ +8069,5 @@ > } else if (op == JSOP_SETALIASEDVAR || op == JSOP_INITALIASEDLEXICAL) { > obj->as<ScopeObject>().setAliasedVar(cx, ScopeCoordinate(pc), name, rhs); > + } else if (op == JSOP_INITGLEXICAL) { > + RootedValue v(cx, rhs); > + InitGlobalLexicalOperation(cx, script, pc, v); I expected InitGlobalLexicalOperation to be fallible, but it looks like it's a simple setSlot. I wonder if we can get rid of the cx argument to make that more obvious. Just a thought; nothing urgent. ::: js/src/jit/VMFunctions.cpp @@ +182,2 @@ > { > + return DefLexicalOperation(cx, name, attrs); I'd remove DefLexical and call DefLexicalOperation directly; they have the same signature AFAICS.

Comment on attachment 8655705 [details] [diff] [review] Support global lexicals in Ion. Review of attachment 8655705 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, nice that we can use TI for this. ::: js/src/jit/CompileInfo.h @@ +222,5 @@ > fixedLexicalBegin_ = script->fixedLexicalBegin(); > + // The minimum stack size is two, because INITGLEXICAL (stack depth 1) > + // is compiled as a SETPROP (stack depth 2) on the global lexical > + // scope. > + nstack_ = Max<unsigned>(script->nslots() - script->nfixed(), 2); Hm I wonder if we could make this `2` a constant somewhere, so we don't need to update this in multiple places when it changes. Not sure what's a good place for it though.

(In reply to Jan de Mooij [:jandem] from comment #46) > Comment on attachment 8655704 [details] [diff] [review] > Support global lexicals in Baseline. > > Review of attachment 8655704 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +8069,5 @@ > > } else if (op == JSOP_SETALIASEDVAR || op == JSOP_INITALIASEDLEXICAL) { > > obj->as<ScopeObject>().setAliasedVar(cx, ScopeCoordinate(pc), name, rhs); > > + } else if (op == JSOP_INITGLEXICAL) { > > + RootedValue v(cx, rhs); > > + InitGlobalLexicalOperation(cx, script, pc, v); > > I expected InitGlobalLexicalOperation to be fallible, but it looks like it's > a simple setSlot. I wonder if we can get rid of the cx argument to make that > more obvious. Just a thought; nothing urgent. > It is infallible, but can GC.

(In reply to Shu-yu Guo [:shu] from comment #50) > It is infallible, but can GC. How? Calls that can GC are usually fallible, else we're sweeping something under the rug.

(In reply to Jan de Mooij [:jandem] from comment #51) > (In reply to Shu-yu Guo [:shu] from comment #50) > > It is infallible, but can GC. > > How? Calls that can GC are usually fallible, else we're sweeping something > under the rug. Transitioning to dictionary mode via shape lookup.

(In reply to Shu-yu Guo [:shu] from comment #53) > Created attachment 8668130 [details] [diff] [review] > ROLLUP FOR FUZZING Applies on top of m-c 9169f652fe5e

s = newGlobal(); try { evalcx("const x = t;", s); } catch (e) {} evalcx("\ function f() {\ this[\"\"] = 7;\ x;\ }\ for(var i = 0; i < 1; i++) {\ try {\ new f();\ } catch(e) {}\ }\ ", s); Run with --fuzzing-safe --no-threads --ion-eager and get: Assertion failure: obj->is<CallObject>(), at vm/TypeInference.cpp

gczeal(14); __defineGetter__("f", decodeURIComponent); for (var i = 0; i < 2; ++i) { try { f(); } catch (e) {} } Run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager and get a crash at a weird memory address with EnterBaseline on the stack.

This is an automated crash issue comment: Summary: Crash [@ ??] Build version: mozilla-central-patch revision 7b47b23c1fab Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug Runtime options: --ion-eager Testcase: setJitCompilerOption("ion.warmup.trigger", 50); __defineGetter__("x", gc) gczeal(14); function f() {} for (var j = 0; j < 99; ++j) { x += f(); } Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7e5c1a6 in ?? () #0 0x00007ffff7e5c1a6 in ?? () [...] #15 0x0000000000000000 in ?? () rax 0x7ffff3f6fb30 140737286437680 rbx 0xfff8800000000063 -2111062325329821 rcx 0x7ffff3f88070 140737286537328 rdx 0x7ffff3f60060 140737286373472 rsi 0x7fffffffd690 140737488344720 rdi 0x7ffff69ce170 140737330864496 rbp 0x7fffffffd948 140737488345416 rsp 0x7fffffffd8f8 140737488345336 r8 0x7fffffffd7b0 140737488345008 r9 0x7fffffffd7c0 140737488345024 r10 0x7fffffffd8f0 140737488345328 r11 0xfff9000000000000 -1970324836974592 r12 0x8 8 r13 0x7fffffffdfe0 140737488347104 r14 0x7ffff3f60060 140737286373472 r15 0x7fffffffd9e0 140737488345568 rip 0x7ffff7e5c1a6 140737352417702 => 0x7ffff7e5c1a6: cmp %rax,0x8(%rdx) 0x7ffff7e5c1aa: jne 0x7ffff7e5c233

I didn't see gkw's test prior to posting, so my test is likely the same issue as the one in comment 56.

This is an automated crash issue comment: Summary: Assertion failure: obj->is<CallObject>(), at js/src/vm/TypeInference.cpp:2571 Build version: mozilla-central-patch revision 7b47b23c1fab Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug Runtime options: --fuzzing-safe --no-threads --ion-check-range-analysis Testcase: function TestCase(e, a) this.passed = getTestCaseResult(e, a); function reportCompare (e,a) { new TestCase(e, a); } function getTestCaseResult(expected, actual) { if (actual != expected) return Math.abs(actual - expected) <= 1E-10; } try { evaluate("let [ , ] = 1, Math"); } catch(exc1) {} toStringResult = toString; expect = true; actual = ((toStringResult + '') == (toString + '')); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); reportCompare(expect, actual); ForIn_1( { length:4, company:"netscape", year:2000, 0:"zero" } ); function ForIn_1( object ) { PropertyArray = new Array(); for ( PropertyArray[PropertyArray.length] in object ) { new TestCase(); new TestCase(); new TestCase(); } new TestCase(); } Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x0000000000777dd4 in UpdatePropertyType (cx=0x7ffff6907400, types=0x7ffff69a3248, obj=0x7ffff7e60070, shape=0x7ffff7e7c6a0, indexed=<optimized out>) at js/src/vm/TypeInference.cpp:2571 #0 0x0000000000777dd4 in UpdatePropertyType (cx=0x7ffff6907400, types=0x7ffff69a3248, obj=0x7ffff7e60070, shape=0x7ffff7e7c6a0, indexed=<optimized out>) at js/src/vm/TypeInference.cpp:2571 #1 0x0000000000777fca in js::ObjectGroup::updateNewPropertyTypes (this=this@entry=0x7ffff7e5ca90, cx=cx@entry=0x7ffff6907400, objArg=objArg@entry=0x7ffff7e60070, id=id@entry=..., types=types@entry=0x7ffff69a3248) at js/src/vm/TypeInference.cpp:2636 #2 0x00000000007ee65c in js::ObjectGroup::getProperty (this=0x7ffff7e5ca90, cx=0x7ffff6907400, obj=0x7ffff7e60070, id=...) at js/src/vm/TypeInference-inl.h:1025 #3 0x000000000077e049 in js::EnsureTrackPropertyTypes (cx=0x7ffff6907400, obj=obj@entry=0x7ffff7e60070, id=...) at js/src/vm/TypeInference.cpp:1287 #4 0x000000000077e191 in js::TypeSet::ObjectKey::ensureTrackedProperty (this=this@entry=0x7ffff7e60071, cx=<optimized out>, id=..., id@entry=...) at js/src/vm/TypeInference.cpp:1268 #5 0x000000000091c6f7 in js::jit::IonBuilder::testGlobalLexicalBinding (this=this@entry=0x7fffffffaf40, name=name@entry=0x7ffff7e1d0a0) at js/src/jit/IonBuilder.cpp:8140 #6 0x0000000000988c00 in js::jit::IonBuilder::jsop_getgname (this=this@entry=0x7fffffffaf40, name=0x7ffff7e1d0a0) at js/src/jit/IonBuilder.cpp:8163 #7 0x0000000000980247 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffaf40, op=op@entry=JSOP_GETGNAME) at js/src/jit/IonBuilder.cpp:1869 #8 0x0000000000980b00 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffaf40) at js/src/jit/IonBuilder.cpp:1504 #9 0x0000000000983756 in js::jit::IonBuilder::buildInline (this=this@entry=0x7fffffffaf40, callerBuilder=callerBuilder@entry=0x7fffffffc020, callerResumePoint=callerResumePoint@entry=0x7ffff69a6098, callInfo=...) at js/src/jit/IonBuilder.cpp:1071 #10 0x0000000000983c85 in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7fffffffc020, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:4978 #11 0x0000000000984250 in js::jit::IonBuilder::inlineSingleCall (this=0x7fffffffc020, callInfo=..., targetArg=<optimized out>) at js/src/jit/IonBuilder.cpp:5484 #12 0x00000000009858fc in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7fffffffc020, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:5540 #13 0x0000000000985c95 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffffc020, argc=2, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:6418 #14 0x000000000097f80c in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffc020, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:1853 #15 0x0000000000980b00 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffc020) at js/src/jit/IonBuilder.cpp:1504 #16 0x0000000000980f45 in js::jit::IonBuilder::build (this=0x7fffffffc020) at js/src/jit/IonBuilder.cpp:900 #17 0x0000000000981c91 in js::jit::AnalyzeNewScriptDefiniteProperties (cx=cx@entry=0x7ffff6907400, fun=<optimized out>, group=group@entry=0x7ffff7e5c9d0, baseobj=..., baseobj@entry=..., initializerList=initializerList@entry=0x7fffffffca00) at js/src/jit/IonAnalysis.cpp:3314 #18 0x00000000007804b3 in js::TypeNewScript::maybeAnalyze (this=0x7ffff520cbb0, cx=cx@entry=0x7ffff6907400, group=0x7ffff7e5c9d0, regenerate=regenerate@entry=0x7fffffffcb80, force=force@entry=false) at js/src/vm/TypeInference.cpp:3712 #19 0x0000000000b5a784 in js::CreateThisForFunctionWithProto (cx=cx@entry=0x7ffff6907400, callee=..., callee@entry=..., proto=proto@entry=..., newKind=newKind@entry=js::GenericObject) at js/src/jsobj.cpp:982 #20 0x0000000000b5af2c in js::CreateThisForFunction (cx=cx@entry=0x7ffff6907400, callee=callee@entry=..., newKind=js::GenericObject) at js/src/jsobj.cpp:1017 #21 0x00000000006d8e3f in js::RunState::maybeCreateThisForConstructor (this=this@entry=0x7fffffffd148, cx=cx@entry=0x7ffff6907400) at js/src/vm/Interpreter.cpp:352 #22 0x00000000009a7dd0 in js::jit::CanEnter (cx=cx@entry=0x7ffff6907400, state=...) at js/src/jit/Ion.cpp:2547 #23 0x00000000006f9162 in Interpret (cx=cx@entry=0x7ffff6907400, state=...) at js/src/vm/Interpreter.cpp:3089 [...] #33 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6574 rax 0x0 0 rbx 0x7ffff69a3248 140737330688584 rcx 0x7ffff6ca53b0 140737333842864 rdx 0x0 0 rsi 0x7ffff6f7a9d0 140737336814032 rdi 0x7ffff6f791c0 140737336807872 rbp 0x7fffffffaa10 140737488333328 rsp 0x7fffffffa9d0 140737488333264 r8 0x7ffff7fe0780 140737354008448 r9 0x6568637461702d6c 7307199746910727532 r10 0x7fffffffa790 140737488332688 r11 0x7ffff6c27960 140737333328224 r12 0x7ffff7e7c6a0 140737352550048 r13 0x7ffff6907400 140737330050048 r14 0x7ffff7e60070 140737352433776 r15 0x7ffff7e5ca90 140737352419984 rip 0x777dd4 <UpdatePropertyType(js::ExclusiveContext*, js::HeapTypeSet*, js::NativeObject*, js::Shape*, bool)+836> => 0x777dd4 <UpdatePropertyType(js::ExclusiveContext*, js::HeapTypeSet*, js::NativeObject*, js::Shape*, bool)+836>: movl $0xa0b,0x0 0x777ddf <UpdatePropertyType(js::ExclusiveContext*, js::HeapTypeSet*, js::NativeObject*, js::Shape*, bool)+847>: callq 0x499510 <abort()>

(In reply to Shu-yu Guo [:shu] from comment #60) > Created attachment 8668707 [details] [diff] [review] > Implement all-or-nothing redeclaration checks for global and eval scripts. As a followup, we can look at removing the JS_DEF* family of ops later.

Comment on attachment 8668707 [details] [diff] [review] Implement all-or-nothing redeclaration checks for global and eval scripts. Review of attachment 8668707 [details] [diff] [review]: ----------------------------------------------------------------- In the long-term, I'm with Jason. We should find a way to think about not doing this piecemeal script generation. I agree, however, that this is not reasonable for the moment. r=me ::: js/src/jit/VMFunctions.cpp @@ +844,5 @@ > return false; > } > > bool > +InitGlobalOrEvalScopeObjects(JSContext* cx, BaselineFrame* frame) I was going to complain that this was now slower because we had inescapable call overhead, now, but that was unavoidable, since we would be calling CheckGlobalDeclarationConflicts anyways. Now I will merely complain lightly about the name, but at best halfheartedly.

This is an automated crash issue comment: Summary: Assertion failure: scope == cx->global() || scope == &cx->global()->lexicalScope() || scope->is<UninitializedLexicalObject>(), at js/src/vm/Interpreter-inl.h:280 Build version: mozilla-central-patch revision 7b47b23c1fab Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug Runtime options: --ion-eager Testcase: var lfcode = new Array(); lfcode.push = loadFile; lfcode.push("3"); lfcode.push(` var actual = ''; anonymous = true; function test() reportCompare(expect, actual, summary); `); function loadFile(lfVarx) { if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) { switch (lfRunTypeId) { case 3: function newFunc(x) { new Function(x)(); }; newFunc(lfVarx); break; } } else if (!isNaN(lfVarx)) { lfRunTypeId = parseInt(lfVarx); } } Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x000000000072c165 in js::SetNameOperation (cx=0x7ffff6906c00, script=0x7ffff3f657e0, pc=0x7ffff3eefd04 "\233", scope=..., val=...) at js/src/vm/Interpreter-inl.h:276 #0 0x000000000072c165 in js::SetNameOperation (cx=0x7ffff6906c00, script=0x7ffff3f657e0, pc=0x7ffff3eefd04 "\233", scope=..., val=...) at js/src/vm/Interpreter-inl.h:276 #1 0x00000000008d7818 in js::jit::DoSetPropFallback (cx=0x7ffff6906c00, frame=0x7fffffffb218, stub_=<optimized out>, lhs=..., rhs=..., res=...) at js/src/jit/BaselineIC.cpp:7615 #2 0x00007ffff7e551af in ?? () [...] #6 0xfff9000000000000 in ?? () #7 0x0000000001b6efc0 in js::jit::DoCallNativeSetterInfo () #8 0x00007ffff3f56c10 in ?? () [...] #30 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffff3eefd04 140737285913860 rcx 0x7ffff6ca588d 140737333844109 rdx 0x0 0 rsi 0x7ffff6f7a9d0 140737336814032 rdi 0x7ffff6f791c0 140737336807872 rbp 0x7fffffffae60 140737488334432 rsp 0x7fffffffad90 140737488334224 r8 0x7ffff7fe8780 140737354041216 r9 0x6568637461702d6c 7307199746910727532 r10 0x7fffffffab50 140737488333648 r11 0x7ffff6c27ee0 140737333329632 r12 0x7ffff6906c00 140737330048000 r13 0x7ffff3f657e0 140737286395872 r14 0x7fffffffafc0 140737488334784 r15 0x7ffff6906c00 140737330048000 rip 0x72c165 <js::SetNameOperation(JSContext*, JSScript*, unsigned char*, JS::Handle<JSObject*>, JS::Handle<JS::Value>)+581> => 0x72c165 <js::SetNameOperation(JSContext*, JSScript*, unsigned char*, JS::Handle<JSObject*>, JS::Handle<JS::Value>)+581>: movl $0x118,0x0 0x72c170 <js::SetNameOperation(JSContext*, JSScript*, unsigned char*, JS::Handle<JSObject*>, JS::Handle<JS::Value>)+592>: callq 0x499510 <abort()>

{ var w; } let w; Run with --fuzzing-safe --no-threads --no-ion --no-baseline and get: Assertion failure: CheckLexicalNameConflict(cx, lexicalScope, varObj, name), at vm/Interpreter-inl.h === (lldb) bt 5 * thread #1: tid = 0xd6f5a, 0x000000010032a125 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::DefLexicalOperation(cx=<unavailable>, lexicalScope=<unavailable>, varObj=<unavailable>, name=<unavailable>, attrs=<unavailable>) + 341 at Interpreter-inl.h:441, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x000000010032a125 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::DefLexicalOperation(cx=<unavailable>, lexicalScope=<unavailable>, varObj=<unavailable>, name=<unavailable>, attrs=<unavailable>) + 341 at Interpreter-inl.h:441 frame #1: 0x0000000100323ac1 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::DefLexicalOperation(cx=0x0000000102b45400, lexicalScopeArg=<unavailable>, varObjArg=<unavailable>, script=<unavailable>, pc=<unavailable>) + 257 at Interpreter-inl.h:463 frame #2: 0x00000001002d294e js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`Interpret(cx=0x0000000102b45400, state=0x00007fff5fbff128) + 65774 at Interpreter.cpp:3520 frame #3: 0x00000001002c27e9 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::RunScript(cx=0x0000000102b45400, state=0x00007fff5fbff128) + 441 at Interpreter.cpp:709 frame #4: 0x00000001002db194 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::ExecuteKernel(cx=0x0000000102b45400, script=<unavailable>, scopeChainArg=0x0000000102c5d070, thisv=0x00007fff5fbff240, newTargetValue=0x00007fff5fbff238, type=EXECUTE_GLOBAL, evalInFrame=<unavailable>, result=<unavailable>) + 1444 at Interpreter.cpp:984 (lldb)

Comment on attachment 8668758 [details] [diff] [review] ROLLUP FOR FUZZING (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #65) > Assertion failure: CheckLexicalNameConflict(cx, lexicalScope, varObj, name), > at vm/Interpreter-inl.h Tested with patch in comment 64.

evalcx("const x = l; with([]) { var x; }", newGlobal()); Run with --fuzzing-safe --no-threads --no-ion --no-baseline on the patch in comment 64 applied on m-c rev 2c1fb007137d and get: Assertion failure: CheckVarNameConflict(cx, lexicalScope, dn), at vm/Interpreter-inl.h === (lldb) bt 5 * thread #1: tid = 0x6a498, 0x00000001003239b0 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::DefVarOperation(cx=0x0000000102b45400, varobj=<unavailable>, dn=<unavailable>, attrs=5) + 960 at Interpreter-inl.h:503, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x00000001003239b0 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::DefVarOperation(cx=0x0000000102b45400, varobj=<unavailable>, dn=<unavailable>, attrs=5) + 960 at Interpreter-inl.h:503 frame #1: 0x00000001002d278c js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`Interpret(cx=0x0000000102b45400, state=0x00007fff5fbfe108) + 65324 at Interpreter.cpp:3503 frame #2: 0x00000001002c27e9 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::RunScript(cx=0x0000000102b45400, state=0x00007fff5fbfe108) + 441 at Interpreter.cpp:709 frame #3: 0x00000001002db194 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::ExecuteKernel(cx=0x0000000102b45400, script=<unavailable>, scopeChainArg=0x0000000102c7c070, thisv=0x00007fff5fbfe220, newTargetValue=0x00007fff5fbfe218, type=EXECUTE_GLOBAL, evalInFrame=<unavailable>, result=<unavailable>) + 1444 at Interpreter.cpp:984 frame #4: 0x00000001002db5dd js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::Execute(cx=0x0000000102b45400, script=<unavailable>, scopeChainArg=<unavailable>, rval=0x0000000102e140a8) + 557 at Interpreter.cpp:1018 (lldb)

// Adapted from js/src/tests/js1_5/Regress/regress-396684.js eval("function f(){}; function f(){};"); Run with --fuzzing-safe --no-threads --no-ion --no-baseline on the patch in comment 64 applied on m-c rev 2c1fb007137d and get: Assertion failure: isKind(PNK_FUNCTION) || isKind(PNK_NAME), at frontend/ParseNode-inl.h === (lldb) bt 5 * thread #1: tid = 0x71ddb, 0x0000000100188426 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::frontend::ParseNode::name(this=<unavailable>) const + 230 at ParseNode-inl.h:20, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x0000000100188426 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::frontend::ParseNode::name(this=<unavailable>) const + 230 at ParseNode-inl.h:20 frame #1: 0x0000000100041556 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`void js::frontend::AppendPackedBindings<js::frontend::FullParseHandler>(pc=0x00007fff5fbfcac8, vec=0x00007fff5fbfcc08, dst=0x0000000102d32170, numUnaliased=0x0000000000000000) + 470 at Parser.cpp:387 frame #2: 0x0000000100041269 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::frontend::ParseContext<js::frontend::FullParseHandler>::drainGlobalOrEvalBindings(this=0x00007fff5fbfcac8, cx=<unavailable>, vars=<unavailable>, lexicals=<unavailable>) + 121 at Parser.cpp:486 frame #3: 0x00000001001cb552 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`BytecodeCompiler::compileScript(this=0x00007fff5fbfcf78, scopeChain=JS::HandleObject @ 0x00007fff5fbfca90, evalCaller=<unavailable>) + 2114 at BytecodeCompiler.cpp:611 frame #4: 0x00000001001cd4fd js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::frontend::CompileScript(cx=<unavailable>, alloc=<unavailable>, scopeChain=JS::HandleObject @ r14, enclosingStaticScope=<unavailable>, evalCaller=<unavailable>, options=<unavailable>, srcBuf=<unavailable>, source_=0x0000000103e75328, extraSct=0x0000000000000000, sourceObjectOut=0x0000000000000000) + 189 at BytecodeCompiler.cpp:848 (lldb)

var x = 0; // Adapted from randomly chosen test: js/src/jit-test/tests/debug/execution-observability-03.js var p = new Proxy({}, { "deleteProperty": function() { var g = newGlobal(); g.parent = {}; g.eval("\ var dbg = new Debugger(parent);\ dbg.onEnterFrame = function() {};\ "); } }); delete p.foo; // Adapted from randomly chosen test: js/src/jit-test/tests/debug/Script-source-02.js evaluate("let x = 0;") Run with --fuzzing-safe --no-threads --baseline-eager --no-ion on the patch in comment 64 applied on m-c rev 2c1fb007137d and get: Assertion failure: frame.isDebuggee(), at vm/Debugger-inl.h === (lldb) bt 5 * thread #1: tid = 0x799ab, 0x0000000100321088 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::Debugger::onLeaveFrame(cx=<unavailable>, frame=<unavailable>, ok=<unavailable>) + 760 at Debugger-inl.h:18, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x0000000100321088 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::Debugger::onLeaveFrame(cx=<unavailable>, frame=<unavailable>, ok=<unavailable>) + 760 at Debugger-inl.h:18 frame #1: 0x00000001005de19f js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::jit::HandleException(js::jit::ResumeFromException*) [inlined] js::jit::HandleExceptionBaseline(cx=<unavailable>, frame=<unavailable>, pc=<unavailable>) + 187 at JitFrames.cpp:753 frame #2: 0x00000001005de0e4 js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js::jit::HandleException(rfe=0x00007fff5fbfd688) + 532 at JitFrames.cpp:899 frame #3: 0x0000000101dd163d frame #4: 0x0000000101dd1dc4 (lldb)

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #67) > Created attachment 8669270 [details] > stack for Assertion failure: CheckVarNameConflict(cx, lexicalScope, dn) > > evalcx("const x = l; with([]) { var x; }", newGlobal()); > > Run with --fuzzing-safe --no-threads --no-ion --no-baseline on the patch in > comment 64 applied on m-c rev 2c1fb007137d and get: > > Assertion failure: CheckVarNameConflict(cx, lexicalScope, dn), at > vm/Interpreter-inl.h > > === > > (lldb) bt 5 > * thread #1: tid = 0x6a498, 0x00000001003239b0 > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js:: > DefVarOperation(cx=0x0000000102b45400, varobj=<unavailable>, > dn=<unavailable>, attrs=5) + 960 at Interpreter-inl.h:503, queue = > 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) > * frame #0: 0x00000001003239b0 > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js:: > DefVarOperation(cx=0x0000000102b45400, varobj=<unavailable>, > dn=<unavailable>, attrs=5) + 960 at Interpreter-inl.h:503 > frame #1: 0x00000001002d278c > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b- > 2c1fb007137d`Interpret(cx=0x0000000102b45400, state=0x00007fff5fbfe108) + > 65324 at Interpreter.cpp:3503 > frame #2: 0x00000001002c27e9 > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js:: > RunScript(cx=0x0000000102b45400, state=0x00007fff5fbfe108) + 441 at > Interpreter.cpp:709 > frame #3: 0x00000001002db194 > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js:: > ExecuteKernel(cx=0x0000000102b45400, script=<unavailable>, > scopeChainArg=0x0000000102c7c070, thisv=0x00007fff5fbfe220, > newTargetValue=0x00007fff5fbfe218, type=EXECUTE_GLOBAL, > evalInFrame=<unavailable>, result=<unavailable>) + 1444 at > Interpreter.cpp:984 > frame #4: 0x00000001002db5dd > js-dbg-64-dm-darwin-589199-c64-38a4cd2b2c6b-2c1fb007137d`js:: > Execute(cx=0x0000000102b45400, script=<unavailable>, > scopeChainArg=<unavailable>, rval=0x0000000102e140a8) + 557 at > Interpreter.cpp:1018 > (lldb) Wow this is bad. Apparently we've always been horribly broken when dealing with redeclarations inside of with scopes. :(

I stopped fuzzing the patch for now because of the bug in comment 68, which keeps triggering all the time. Please add a new feedback? when updating the patch. Thanks!

Gaia fix landed in https://github.com/mozilla-b2g/gaia/commit/5fa0051b2c097f940d4a1be3109c53a925a20da8

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed936e06df7b https://hg.mozilla.org/integration/mozilla-inbound/rev/af4bccd688b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcc3881b95d https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f647fe2d10 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d8f1cd3afc https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4a8943d496 https://hg.mozilla.org/integration/mozilla-inbound/rev/26fc971a24c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/3228ac384c92 https://hg.mozilla.org/integration/mozilla-inbound/rev/04eb8f524122 https://hg.mozilla.org/integration/mozilla-inbound/rev/6917045ffcf5 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0aa2c21379

Apparently this introduced a 21% regression in a test from Dromaeo (https://arewefastyet.com/#machine=17&view=single&suite=dromaeo&subtest=dom-query) - it's either this bug or bug 1202902

> Apparently this introduced a 21% regression in a test from Dromaeo Tracked in bug 1212183.

https://hg.mozilla.org/mozilla-central/rev/ed936e06df7b https://hg.mozilla.org/mozilla-central/rev/af4bccd688b2 https://hg.mozilla.org/mozilla-central/rev/3bcc3881b95d https://hg.mozilla.org/mozilla-central/rev/b9f647fe2d10 https://hg.mozilla.org/mozilla-central/rev/b2d8f1cd3afc https://hg.mozilla.org/mozilla-central/rev/6e4a8943d496 https://hg.mozilla.org/mozilla-central/rev/26fc971a24c4 https://hg.mozilla.org/mozilla-central/rev/3228ac384c92 https://hg.mozilla.org/mozilla-central/rev/04eb8f524122 https://hg.mozilla.org/mozilla-central/rev/6917045ffcf5 https://hg.mozilla.org/mozilla-central/rev/ac0aa2c21379

(In reply to Guilherme Lima from comment #75) > Apparently this introduced a 21% regression in a test from Dromaeo > (https://arewefastyet.com/#machine=17&view=single&suite=dromaeo&subtest=dom- > query) - it's either this bug or bug 1202902 Yep, already has a fix and will push today.

Looks like all reviewed changes are landed and persisted on m-c. What's left in this bug? I think we should file new bug and block this bug for any remaining issues.

Good call, clearing leave-open.

Sheriff will not close the bug unless a new patch is merged.

Added the site compatibility doc, though I don't think this change affects production sites. Reviews welcome. https://www.fxsitecompat.com/en-US/docs/2015/variables-defined-with-const-and-let-are-no-longer-properties-on-window/

(In reply to Kohei Yoshino [:kohei] from comment #82) > variables defined with those statements *global* variables defined with those statements Some other notes: 1. This change may introduce redeclaration errors. For example, <script type="application/javascript;version=1.8"> let a = 1; alert(a); let a = 2; alert(a); </script> will no longer work. <script type="application/javascript;version=1.8"> let a = 1; alert(a); </script> <script type="application/javascript;version=1.8"> let a = 2; alert(a); </script> will not work either. Even worse, <script src="let.js" type="application/javascript;version=1.8"></script> <script type="application/javascript;version=1.8"> let a = 2; alert(a); </script> -- let.js -- let a = 1; alert(a); ------------ will not work. 2. An eval()'ed script will no longer propagate global let/const variables to the outer context. Example: <script type="application/javascript;version=1.8"> eval('let a = 1;'); alert(a); // ReferenceError </script>

Thanks for your clarification. Updated the doc accordingly: https://www.fxsitecompat.com/en-US/docs/2015/variables-defined-with-const-and-let-are-no-longer-properties-on-window-redeclaration-with-let-will-throw/

https://developer.mozilla.org/en-US/Firefox/Releases/44#JavaScript https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let https://blog.mozilla.org/addons/2015/10/14/breaking-changes-let-const-firefox-nightly-44/

(In reply to Florian Scholz [:fscholz] (MDN) from comment #85) > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/ > let I believe the MDN page is still outdated. Specifically this section: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Scoping_rules