In preparation for ES6 global lexicals, I'd like to account for polluting global scopes on the static scope chain. AFAICT, there are currently 2 ways to pollute the global scope: 1. 0+ non-syntactic WithObjects. This is used by various JSAPI functions that take an |AutoObjectVector& scopeChain|. 2. A PlainObject that acts as unqualified/qualified varobj that's supposed to capture all global vars. This is only used by ExecuteInGlobalAndReturnScope for frame scripts. I propose to add a StaticPollutingGlobalObject static scope that accounts for both these cases. With this change, we can remove the HasPollutedGlobal flag in favor of checking if a JSScript's enclosingStaticScope_ is a StaticPollutingGlobalObject.

Rock on. Also, if you'd like to give the PlainObject from (2) it's own new js::Class and then add the asserts suggested in bug 1143794 that would be most welcome :)

CloneFunctionObject is split into the following: - CloneFunctionAndScript, which deep clones the function and its script, giving the cloned script a new static scope chain. This is used for cloning singleton lambdas and JSAPI cloning. For singleton lambdas, the original and the clone script have the same static scope chain. For JSAPI cloning, a new static scope is provided (either null, for a clean global, or StaticPollutingGlobalObject, for a polluted global). - CloneFunctionReuseScript, which clones the function but reuses the script, and thus keeps the same static scope chain. CloneScript is split into the following: - CloneGlobalScript, which clones a script with and gives it a new static scope. - CloneScriptIntoFunction, which clones a script into a JSFunction and gives it a new static scope. Cloning a script into a new function container requires slightly different logic to hook up the static scope chain before cloning inner scripts.

(In reply to Luke Wagner [:luke] from comment #1) > Rock on. Also, if you'd like to give the PlainObject from (2) it's own new > js::Class and then add the asserts suggested in bug 1143794 that would be > most welcome :) I'll have to leave this for another time. Just adding a static scope was much harder than I anticipated.

Comment on attachment 8613193 [details] [diff] [review] Add StaticPollutingGlobalObject and teach scope iterators about it. Review of attachment 8613193 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +773,5 @@ > /* > * DebuggerEnv should only wrap a debug scope chain obtained (transitively) > * from GetDebugScopeFor(Frame|Function). > */ > + MOZ_ASSERT(!env->is<ScopeObject>()); Why all these changes for IsSyntacticScope to !is<ScopeObject>()? The former had some semantic meaning, the latter is an impl detail (and ambiguous, e.g., wrt DebugScopeProxy). ::: js/src/vm/ScopeObject-inl.h @@ +100,5 @@ > } > > template <AllowGC allowGC> > inline bool > +StaticScopeIter<allowGC>::definitelyHasDynamicScopeObject() const I see what you mean by "definitely", but it's a bit confusing to read in the context of a specific iteration. How about having this function, as a precondition, not be valid to call in a polluting scope and then handle this in callers (which have to do something special anyways? ::: js/src/vm/ScopeObject.cpp @@ +563,5 @@ > + obj->setReservedSlot(SCOPE_CHAIN_SLOT, ObjectOrNullValue(enclosing)); > + return obj; > +} > + > +const Class StaticPollutingGlobalObject::class_ = { How about also making a new class to replace the PlainObject used for the ExecuteInGlobalAndReturnScope case? That could allow even stronger assertions everywhere. @@ +1010,5 @@ > + // > + // Only increment ssi_ once we've iterated through all the non-syntactic > + // DynamicWithObjects. > + if (ssi_.type() != StaticScopeIter<CanGC>::PollutingGlobal || !hasPollutingGlobalScopeObject()) > + ssi_++; Could this be instead expressed as: if (type() == PollutingScope && scope_ && scope_->is<DynamicWithObject>()) ssi_++; ? ::: js/src/vm/ScopeObject.h @@ +21,5 @@ > namespace frontend { struct Definition; } > > class StaticWithObject; > class StaticEvalObject; > +class StaticPollutingGlobalObject; I'm wondering if 'Global' makes sense here, since this static scope object doesn't represent the *actual* global object, just some object on the scope chain before the global object. How about 'StaticPollutingScopeObject'? @@ +217,5 @@ > * > + * NB2: StaticPollutingGlobalObjects notify of 0+ non-syntactic > + * DynamicWithObjects on the dynamic scope chain. Additionally, scripts whose > + * enclosing static scope is a StaticPollutingGlobalObjects may have their > + * dynamic scope chain terminated by a non-GlobalObject varobj. 'terminated' makes me think "the last non-null obj on the scope chain" which is necessarily a GlobalObject. I think you mean "...have the syntactic portion of their scope chain...". @@ +407,5 @@ > +// 1. 0+ non-syntactic DynamicWithObjects. This static scope helps ScopeIter > +// iterate these DynamicWithObjects. > +// > +// 2. 1 PlainObject that is a both a QualifiedVarObj and an UnqualifiedVarObj, > +// used exclusively in js::ExecuteInGlobalAndReturnScope. s/used/created/ @@ +411,5 @@ > +// used exclusively in js::ExecuteInGlobalAndReturnScope. > +// > +// Since this PlainObject is not a ScopeObject, ScopeIter cannot iterate > +// through it. Instead, this PlainObject always terminates the dynamic > +// scope chain in lieu of a GlobalObject. Technically, I don't think this is correct: - ScopeIter *can* iterate through the PlainObject (it does so by iterating to obj->global()) - the PlainObject doesn't terminate the scope chain, the global still does. @@ +1103,5 @@ > +{ > + if (ssi_.type() == StaticScopeIter<CanGC>::PollutingGlobal) { > + MOZ_ASSERT_IF(scope_->is<DynamicWithObject>(), > + !scope_->as<DynamicWithObject>().isSyntactic()); > + return scope_->is<DynamicWithObject>(); What about the PlainObject, doesn't it hasPollutingGlobalScopeObject? @@ +1128,5 @@ > + return staticEval().isStrict(); > + > + // Polluting global scopes are not syntactic scopes and cannot be > + // synthesized. > + return type() != PollutingGlobal; This doesn't seem right, given the name of this function. Seems like either the function needs to be renamed ('canHaveSyntacticScopeObject') or the polluting case should return 'true' and the callers should deal. ::: js/src/vm/Stack.cpp @@ +145,5 @@ > { > #ifdef DEBUG > RootedObject enclosingScope(cx, script->enclosingStaticScope()); > for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) { > + if (i.definitelyHasDynamicScopeObject()) { Instead of adding this method which is specific to this case, what about having control flow: if (i.type() == PollutingScope) { while ... } else if (i.hasDynamicScopeObject()) { switch ... }

Comment on attachment 8613192 [details] [diff] [review] Cleanup: use standard object allocation functions when allocating scope objects. Review of attachment 8613192 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup! ::: js/src/vm/ScopeObject.h @@ +582,5 @@ > * A static block object is cloned (when entering the block) iff some > * variable of the block isAliased. > */ > bool needsClone() { > + return numVariables() > 0 && !getSlot(RESERVED_SLOTS).isFalse(); I guess this hunk leaked in from another patch? I'm not really competent to review this change.

(In reply to Terrence Cole [:terrence] from comment #10) > Comment on attachment 8613192 [details] [diff] [review] > Cleanup: use standard object allocation functions when allocating scope > objects. > > Review of attachment 8613192 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice cleanup! > > ::: js/src/vm/ScopeObject.h > @@ +582,5 @@ > > * A static block object is cloned (when entering the block) iff some > > * variable of the block isAliased. > > */ > > bool needsClone() { > > + return numVariables() > 0 && !getSlot(RESERVED_SLOTS).isFalse(); > > I guess this hunk leaked in from another patch? I'm not really competent to > review this change. Surprisingly it is in the right patch. Previously, StaticBlockObjects were manually constructed as OBJECT4s, so it was always fine to getSlot(RESERVED_SLOTS). Now that it's switched over to standard allocation paths, with only 2 reserved slots, it's allocated as OBJECT2 if numVariables() == ). So it's only legal to getSlot(RESERVED_SLOTS) if numVariables() > 0.

Above, numVariables() == ) is supposed to be numVariables() == 0.

(In reply to Luke Wagner [:luke] from comment #9) > Comment on attachment 8613193 [details] [diff] [review] > Add StaticPollutingGlobalObject and teach scope iterators about it. > > Review of attachment 8613193 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Debugger.cpp > @@ +773,5 @@ > > /* > > * DebuggerEnv should only wrap a debug scope chain obtained (transitively) > > * from GetDebugScopeFor(Frame|Function). > > */ > > + MOZ_ASSERT(!env->is<ScopeObject>()); > > Why all these changes for IsSyntacticScope to !is<ScopeObject>()? The > former had some semantic meaning, the latter is an impl detail (and > ambiguous, e.g., wrt DebugScopeProxy). > I can change this back. I was intending for Debugger to actually be able to wrap the non-syntactic DynamicWithObjects in DebugScopeProxies, since they are, after all, ScopeObjects. Not a big deal either way, since nothing depends on this behavior one way or another right now. > ::: js/src/vm/ScopeObject-inl.h > @@ +100,5 @@ > > } > > > > template <AllowGC allowGC> > > inline bool > > +StaticScopeIter<allowGC>::definitelyHasDynamicScopeObject() const > > I see what you mean by "definitely", but it's a bit confusing to read in the > context of a specific iteration. How about having this function, as a > precondition, not be valid to call in a polluting scope and then handle this > in callers (which have to do something special anyways? How about the current behavior of returning false for when settled on PollutingGlobal, and rename it 'hasSyntacticDynamicScopeObject'? Almost all uses of the predicate are for computing hops in the frontend, and they don't need to do something special for the PollutedGlobal static scope, just not consider it to have a syntactic scope object. > > ::: js/src/vm/ScopeObject.cpp > @@ +563,5 @@ > > + obj->setReservedSlot(SCOPE_CHAIN_SLOT, ObjectOrNullValue(enclosing)); > > + return obj; > > +} > > + > > +const Class StaticPollutingGlobalObject::class_ = { > > How about also making a new class to replace the PlainObject used for the > ExecuteInGlobalAndReturnScope case? That could allow even stronger > assertions everywhere. > I'll try -- not in the mood for this yak shaving to get even more hairy though. If tests fail I'll wait on this. > @@ +1010,5 @@ > > + // > > + // Only increment ssi_ once we've iterated through all the non-syntactic > > + // DynamicWithObjects. > > + if (ssi_.type() != StaticScopeIter<CanGC>::PollutingGlobal || !hasPollutingGlobalScopeObject()) > > + ssi_++; > > Could this be instead expressed as: > if (type() == PollutingScope && scope_ && scope_->is<DynamicWithObject>()) > ssi_++; > ? > No, that case is handling both the normal case and the case you wrote above. Could rewrite as if (type() == PollutingScope) { if (hasPollutingScopeObject()) ssi_++; } else { ssi_++; } > ::: js/src/vm/ScopeObject.h > @@ +21,5 @@ > > namespace frontend { struct Definition; } > > > > class StaticWithObject; > > class StaticEvalObject; > > +class StaticPollutingGlobalObject; > > I'm wondering if 'Global' makes sense here, since this static scope object > doesn't represent the *actual* global object, just some object on the scope > chain before the global object. How about 'StaticPollutingScopeObject'? Well, definitely not 'StaticPollutingScopeObject', that's too generic IMO. It occurs in a very specific place: where the global otherwise would be. Everywhere else in the code already refers to 'hasPollutedGlobalScope'. Is the problem 'GlobalObject'? I was just following convention there with the 'Object' suffix. What about 'StaticPollutingGlobalScope'? > > @@ +217,5 @@ > > * > > + * NB2: StaticPollutingGlobalObjects notify of 0+ non-syntactic > > + * DynamicWithObjects on the dynamic scope chain. Additionally, scripts whose > > + * enclosing static scope is a StaticPollutingGlobalObjects may have their > > + * dynamic scope chain terminated by a non-GlobalObject varobj. > > 'terminated' makes me think "the last non-null obj on the scope chain" which > is necessarily a GlobalObject. I think you mean "...have the syntactic > portion of their scope chain...". Good point, will change wording. > > @@ +407,5 @@ > > +// 1. 0+ non-syntactic DynamicWithObjects. This static scope helps ScopeIter > > +// iterate these DynamicWithObjects. > > +// > > +// 2. 1 PlainObject that is a both a QualifiedVarObj and an UnqualifiedVarObj, > > +// used exclusively in js::ExecuteInGlobalAndReturnScope. > > s/used/created/ > Fixed. > @@ +411,5 @@ > > +// used exclusively in js::ExecuteInGlobalAndReturnScope. > > +// > > +// Since this PlainObject is not a ScopeObject, ScopeIter cannot iterate > > +// through it. Instead, this PlainObject always terminates the dynamic > > +// scope chain in lieu of a GlobalObject. > > Technically, I don't think this is correct: > - ScopeIter *can* iterate through the PlainObject (it does so by iterating > to obj->global()) > - the PlainObject doesn't terminate the scope chain, the global still does. > Since the ScopeIter rewrite, ScopeIter can no longer iterate through the PlainObject. It only calls enclosingScope() on things that is<ScopeObject>(). For the termination wording, I'll change it to "terminates the syntactic portion of the dynamic scope chain in lieu of a GlobalObject". > @@ +1103,5 @@ > > +{ > > + if (ssi_.type() == StaticScopeIter<CanGC>::PollutingGlobal) { > > + MOZ_ASSERT_IF(scope_->is<DynamicWithObject>(), > > + !scope_->as<DynamicWithObject>().isSyntactic()); > > + return scope_->is<DynamicWithObject>(); > > What about the PlainObject, doesn't it hasPollutingGlobalScopeObject? > No, the intention of hasPollutingGlobalScopeObject is if it has a |ScopeObject| (i.e. the non-syntactic Withs) on the dynamic scope chain, as ScopeIter iterates through ScopeObjects. Those Withs are still ScopeObjects, just not syntactic ones. This is setting up so that when the time comes, ScopeIter can go past them to get to the global lexical scope object. > @@ +1128,5 @@ > > + return staticEval().isStrict(); > > + > > + // Polluting global scopes are not syntactic scopes and cannot be > > + // synthesized. > > + return type() != PollutingGlobal; > > This doesn't seem right, given the name of this function. Seems like either > the function needs to be renamed ('canHaveSyntacticScopeObject') or the > polluting case should return 'true' and the callers should deal. > Like I said above, I don't think it's natural for most of the callees to deal. This method really means "can have synthesizable ScopeObject". How about: hasPollutingGlobalScopeObject -> hasNonSyntacticScopeObject hasScopeObject -> hasAnyScopeObject canHaveScopeObject -> canHaveSyntacticScopeObject and introduce a new hasSyntacticScopeObject that most users should use? > ::: js/src/vm/Stack.cpp > @@ +145,5 @@ > > { > > #ifdef DEBUG > > RootedObject enclosingScope(cx, script->enclosingStaticScope()); > > for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) { > > + if (i.definitelyHasDynamicScopeObject()) { > > Instead of adding this method which is specific to this case, what about > having control flow: > if (i.type() == PollutingScope) { > while ... > } else if (i.hasDynamicScopeObject()) { > switch ... > } Sure, can do.

Oh, how about PollutedToplevel?

(In reply to Shu-yu Guo [:shu] from comment #14) > Oh, how about PollutedToplevel? Nope, I take this back. I think I'll just stick with StaticPollutingGlobalObject and hasPollutedGlobalScope. I don't think it's that confusing.

(In reply to Shu-yu Guo [:shu] from comment #13) > (In reply to Luke Wagner [:luke] from comment #9) > > Comment on attachment 8613193 [details] [diff] [review] > > Add StaticPollutingGlobalObject and teach scope iterators about it. > > > > Review of attachment 8613193 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: js/src/vm/Debugger.cpp > > @@ +773,5 @@ > > > /* > > > * DebuggerEnv should only wrap a debug scope chain obtained (transitively) > > > * from GetDebugScopeFor(Frame|Function). > > > */ > > > + MOZ_ASSERT(!env->is<ScopeObject>()); > > > > Why all these changes for IsSyntacticScope to !is<ScopeObject>()? The > > former had some semantic meaning, the latter is an impl detail (and > > ambiguous, e.g., wrt DebugScopeProxy). > > > > I can change this back. I was intending for Debugger to actually be able to > wrap the non-syntactic DynamicWithObjects in DebugScopeProxies, since they > are, after all, ScopeObjects. > Nope, I take this back too. I remembered why I changed this. The Debugger needs to start wrapping even non-syntactic ScopeObjects as DebugScopeProxies in preparation for the the global lexical scope, which *will* be a syntactic ScopeObject, but may come after the non-syntactic DynamicWithObjects with a polluting global scope: GlobalObject | ExtensibleBlockObject (global lexical) | non-syntactic DynamicWithObject | ... For correctness, Debugger.Environment needs to reflect the global lexical scope, so it can't just give up when it encounters the first non-syntactic scope on the scope chain.

Oops, squashed it wrong.

Renamed according to discussion with luke on IRC.

Renamed stuff.

Renamed stuff. CloneFunctionObject is split into the following: - CloneFunctionAndScript, which deep clones the function and its script, giving the cloned script a new static scope chain. This is used for cloning singleton lambdas and JSAPI cloning. For singleton lambdas, the original and the clone script have the same static scope chain. For JSAPI cloning, a new static scope is provided (either null, for a clean global, or StaticPollutingGlobalObject, for a polluted global). - CloneFunctionReuseScript, which clones the function but reuses the script, and thus keeps the same static scope chain. CloneScript is split into the following: - CloneGlobalScript, which clones a script with and gives it a new static scope. - CloneScriptIntoFunction, which clones a script into a JSFunction and gives it a new static scope. Cloning a script into a new function container requires slightly different logic to hook up the static scope chain before cloning inner scripts.

Comment on attachment 8615029 [details] [diff] [review] Use JS::CompileForNonSyntacticScope in Gecko where we used to set polluted global scope. Flipping review to bz, since he dealt with this stuff recently.

Comment on attachment 8615029 [details] [diff] [review] Use JS::CompileForNonSyntacticScope in Gecko where we used to set polluted global scope. >+ if(JS_IsGlobalObject(targetObj)) Space after "if", please. r=me

Comment on attachment 8615024 [details] [diff] [review] Add StaticNonSyntacticScopeObjects and teach scope iterators about it. Review of attachment 8615024 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks for all the renaming. ::: js/src/vm/ScopeObject.cpp @@ +1008,5 @@ > + // scope chain for them, we need to manually iterate when ssi_ is settled > + // on the polluting global. > + // > + // Only increment ssi_ once we've iterated through all the non-syntactic > + // ScopeObjects. IIUC, this nice, concise comment subsumes the whole prior para so it could be removed. ::: js/src/vm/ScopeObject.h @@ +101,5 @@ > + // Return whether this static scope will have a syntactic (i.e. a > + // ScopeObject that isn't a non-syntactic With) on the dynamic scope > + // chain. The polluting global scope may have 0+ scope objects on the > + // dynamic scope chain, and thus is not definitely known to have any > + // dynamic scope objects. I'd leave off the last sentence since, being a non-syntactic scope object, it isn't relevant. @@ +410,5 @@ > +// created exclusively in js::ExecuteInGlobalAndReturnScope. > +// > +// Since this PlainObject is not a ScopeObject, ScopeIter cannot iterate > +// through it. Instead, this PlainObject always comes after the syntactic > +// portion of the dynamic scope chain in lieu of a GlobalObject. s/in lieu of/in front of/ since name lookup will still reach the GlobalObject (viz., GETNAME). @@ +1109,5 @@ > { > + if (ssi_.type() == StaticScopeIter<CanGC>::NonSyntactic) { > + MOZ_ASSERT_IF(scope_->is<DynamicWithObject>(), > + !scope_->as<DynamicWithObject>().isSyntactic()); > + return scope_->is<DynamicWithObject>(); Could you add: "The case we're worrying about here is a NonSyntactic static scope which has zero corresponding non-syntactic DynamicWithObject scopes." Also, since you said the goal is to eventually iterate through the PlainObject scope, perhaps you can add "|| scope_->is<PlainObject>()"? @@ +1130,5 @@ > + // Only strict eval scopes can have dynamic scope objects. > + if (type() == Eval) > + return staticEval().isStrict(); > + > + // Non-syntactic scopes cannot be synthesized. Synthesizability doesn't seem relevant, so I'd leave off the comment. Also, perhaps you could 'switch (type()) { ... }' (sucking in the above Eval case) so that it would be quite clear to the reader the cases involved (rather than assuming !NonSyntactic implies Syntactic).

Comment on attachment 8615025 [details] [diff] [review] Remove PollutedGlobalScopeOption in favor of using the static scope chain to detect non-syntactic scopes. Review of attachment 8615025 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +4086,3 @@ > if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf, > enclosingStaticScope)) > + { IIUC, you can fit the CompileFunctionBody call in one line and remove the curlies.

Comment on attachment 8615027 [details] [diff] [review] Replace the PlainObj varobj with NonSyntacticVariablesObject. Review of attachment 8615027 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jsfun.cpp @@ +1969,5 @@ > atom, nullptr, allocKind, newKind); > } > > +static bool > +NewFunctionScopeIsWellFormed(ExclusiveContext* cx, HandleObject parent) You might to wrap this in #ifdef DEBUG to avoid "static function is unused" warning in opt builds. @@ +1971,5 @@ > > +static bool > +NewFunctionScopeIsWellFormed(ExclusiveContext* cx, HandleObject parent) > +{ > + RootedObject realParent(cx, SkipScopeParent(parent)); I think it'd look a bit nicer to put 'realParent' after the comment. ::: js/src/jsobjinlines.h @@ +223,5 @@ > JSObject::isQualifiedVarObj() > { > if (is<js::DebugScopeObject>()) > return as<js::DebugScopeObject>().scope().isQualifiedVarObj(); > + // FIXMEshu: We would like to assert that only GlobalObject or Can you replace "FIXMEshu" with "TODO" if this lands? ::: js/src/vm/ScopeObject.h @@ +429,5 @@ > +// bindings. That is, a scope object that captures both qualified var > +// assignments and unqualified bareword assignments. Its parent is always the > +// real global. > +// > +// This is used in ExecuteInGlobalAndReturnScope as a fake global scope. Similar to my comment in the previous patch; I wouldn't think of this as a fake global: the global object is still the root of the scope chain (and is reached by GETNAME); this is just an object in front that happens to receive new vars b/c magic. @@ +1090,5 @@ > IsSyntacticScope(JSObject* scope) > { > return scope->is<ScopeObject>() && > (!scope->is<DynamicWithObject>() || > + scope->as<DynamicWithObject>().isSyntactic()) && Perhaps put the two disjuncts on the same line to make the associativity clear?

Missed a case for detecting 'with' scopes. Going to efaust since he touched this most recently.

Rebased and fixed XDR. See comment 22 for explanation of API split.

Rebased on top of new.target stuff.

Comment on attachment 8615643 [details] [diff] [review] Detect with scopes at parse time using the static scope chain for non-function scripts. Also cache static scope properties on SharedGlobalContext. Review of attachment 8615643 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/frontend/SharedContext.h @@ +287,5 @@ > + Directives directives, Handle<ScopeObject*> topStaticScope, > + bool extraWarnings) > + : SharedContext(cx, directives, extraWarnings), > + topStaticScope_(topStaticScope), > + allowNewTarget_(computeAllowSyntax(AllowedSyntax::NewTarget)), This is really cute. Nice.

Comment on attachment 8615029 [details] [diff] [review] Use JS::CompileForNonSyntacticScope in Gecko where we used to set polluted global scope. Review of attachment 8615029 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Might as well land this for other people.

Comment on attachment 8621405 [details] [diff] [review] Debug function to dump static scope chain of scripts. Review of attachment 8621405 [details] [diff] [review]: ----------------------------------------------------------------- Yes please. I wanted this the last time I poked some of this stuff. Thanks for adding this.

Comment on attachment 8615627 [details] [diff] [review] Restructure function and script cloning in light of PollutingGlobal scope changes. Review of attachment 8615627 [details] [diff] [review]: ----------------------------------------------------------------- This review is neither punctual nor particularly punctilious. :-( Wish I could have done better on either front, but this is just byzantine. ::: js/src/jsapi.cpp @@ +3281,5 @@ > > // If a function was compiled to be lexically nested inside some other > // script, we cannot clone it without breaking the compiler's assumptions. > JSObject* scope = fun->nonLazyScript()->enclosingStaticScope(); > + if (scope) { if (JSObject* scope = ...) { @@ +3346,5 @@ > > + if (CanReuseScriptForClone(cx->compartment(), fun, dynamicScope)) { > + // If the script is to be reused, either the script can already handle > + // non-syntactic scopes, or there is no new static scope. > + MOZ_ASSERT(!staticScope || fun->getOrCreateScript(cx)->hasNonSyntacticScope()); This getOrCreateScript call is fallible, right? Can you write a LazyScript-aware method that isn't fallible to do this? Some OOM tests or other are going to trip this, as written. @@ +3366,5 @@ > CloneFunctionObject(JSContext* cx, HandleObject funobj, AutoObjectVector& scopeChain) > { > RootedObject dynamicScope(cx); > + Rooted<ScopeObject*> staticScope(cx); > + if (!CreateNonSyntacticScopeChain(cx, scopeChain, &dynamicScope, &staticScope)) |unusedStaticScope| before, eh? That looks like it was a bad sign for proper semantics, probably. ::: js/src/jsscript.cpp @@ +1205,4 @@ > lazy.set(LazyScript::Create(cx, fun, nullptr, enclosingScope, enclosingScript, > packedFields, begin, end, lineno, column)); > + if (!lazy) > + return false; Minor preference for LazyScript* ls = LazyScript::Create(...); if (!ls) return false; lazy.set(ls); fun->initLazyScript(lazy); rather than passing a possibly-null to .set. Don't use potentially-errorful return values before checking them, and all that. @@ +3129,4 @@ > > clone = CloneNestedScopeObject(cx, enclosingScope, innerBlock); > } else if (obj->is<JSFunction>()) { > RootedFunction innerFun(cx, &obj->as<JSFunction>()); There's an extraordinary amount of pushing/popping of Rooted in here. Might be worth (separate patch) moving them all out of the loop, if we can do it without obscuring lifetimes too much -- which seems doable to me. If there's concern about cross-loop dependencies occurring, we can always initialize them all to nullptr (or crash-on-touch?) at the starts of the loops in debug builds, so that interloop dependencies get caught in testing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb11e655d223 https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd90228daf6 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c11d517e18 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e35269a8062 https://hg.mozilla.org/integration/mozilla-inbound/rev/63bd369e5349 https://hg.mozilla.org/integration/mozilla-inbound/rev/53fcddbe4cfb https://hg.mozilla.org/integration/mozilla-inbound/rev/b04ab7bd78af https://hg.mozilla.org/integration/mozilla-inbound/rev/502ddf7d2268 https://hg.mozilla.org/integration/mozilla-inbound/rev/52772afc023e https://hg.mozilla.org/integration/mozilla-inbound/rev/06cc09729bf4 https://hg.mozilla.org/integration/mozilla-inbound/rev/f69852001c61 https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3c302864bf

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e3bb16f653

Backed out for Windows Spidermonkey bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=10805090&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/0379d120132a https://hg.mozilla.org/integration/mozilla-inbound/rev/710aabd4e4ab https://hg.mozilla.org/integration/mozilla-inbound/rev/cb18fdf212cc https://hg.mozilla.org/integration/mozilla-inbound/rev/1d26c357fea5 https://hg.mozilla.org/integration/mozilla-inbound/rev/77283cc3e7e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/db16ffa16c96 https://hg.mozilla.org/integration/mozilla-inbound/rev/fba7bb481879 https://hg.mozilla.org/integration/mozilla-inbound/rev/96207d707430 https://hg.mozilla.org/integration/mozilla-inbound/rev/0cfeee56564b https://hg.mozilla.org/integration/mozilla-inbound/rev/8031242fe5e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec0a501209a https://hg.mozilla.org/integration/mozilla-inbound/rev/42fcb204b544

I'm pretty sure something here triggered these build warnings (treated as an error on clang 3.6) -- just hitting these on m-i locally, for the first time: { js/src/frontend/SharedContext.h:296:16: error: 'toObjectBox' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] ObjectBox* toObjectBox() { return nullptr; } ^ js/src/frontend/SharedContext.h:198:24: note: overridden virtual function is here virtual ObjectBox* toObjectBox() = 0; ^ ...and: js/src/frontend/SharedContext.h:341:16: error: 'toObjectBox' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] ObjectBox* toObjectBox() { return this; } ^ js/src/frontend/SharedContext.h:198:24: note: overridden virtual function is here virtual ObjectBox* toObjectBox() = 0; ^ } We just need to label the toObjectBox impls as 'override'. I'll push a followup to do that.

Added the 'override' annotation, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6c99f92485

sorry had to back this out since something in this push caused https://treeherder.mozilla.org/logviewer.html#?job_id=10818914&repo=mozilla-inbound to fail permanently

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/efd38e59dbc8 https://hg.mozilla.org/integration/mozilla-inbound/rev/f1930da6025e https://hg.mozilla.org/integration/mozilla-inbound/rev/4f335a15cf90 https://hg.mozilla.org/integration/mozilla-inbound/rev/62c1baace9f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/00ced26ae3b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/773597ed1360 https://hg.mozilla.org/integration/mozilla-inbound/rev/8fff20d67e1f https://hg.mozilla.org/integration/mozilla-inbound/rev/a5006bb25be0 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a41852dfabb https://hg.mozilla.org/integration/mozilla-inbound/rev/19d458098a1f https://hg.mozilla.org/integration/mozilla-inbound/rev/2d778cfa2f37 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae258031fc0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd9a9b94341

What was causing the Gu failures. Such sadness.

Comment on attachment 8623331 [details] [diff] [review] Rebase yield offsets when cloning scripts. Review of attachment 8623331 [details] [diff] [review]: ----------------------------------------------------------------- Ugh good catch. We should backport right?

https://hg.mozilla.org/integration/mozilla-inbound/rev/381003ca79a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc0d6adf6ed https://hg.mozilla.org/integration/mozilla-inbound/rev/7a404929c74c https://hg.mozilla.org/integration/mozilla-inbound/rev/64fa28f0255a https://hg.mozilla.org/integration/mozilla-inbound/rev/513bbca6fb6c https://hg.mozilla.org/integration/mozilla-inbound/rev/5e88d482f5e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e00d8a3ca1d https://hg.mozilla.org/integration/mozilla-inbound/rev/5c01ab1d9790 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b157964c572 https://hg.mozilla.org/integration/mozilla-inbound/rev/5abb37cb0495 https://hg.mozilla.org/integration/mozilla-inbound/rev/4143cca42cc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/97352c48fb98 https://hg.mozilla.org/integration/mozilla-inbound/rev/662ec8b0561e

Unsurprisingly, this re-landing caused the issues noted in comment 43 again. I'll push the followup again.

(In reply to Daniel Holbert [:dholbert] from comment #50) > Unsurprisingly, this re-landing caused the issues noted in comment 43 again. > I'll push the followup again. Doh, I'm sorry! I missed your comment the first time around because I was too busy being angry.

https://hg.mozilla.org/mozilla-central/rev/381003ca79a3 https://hg.mozilla.org/mozilla-central/rev/1dc0d6adf6ed https://hg.mozilla.org/mozilla-central/rev/7a404929c74c https://hg.mozilla.org/mozilla-central/rev/64fa28f0255a https://hg.mozilla.org/mozilla-central/rev/513bbca6fb6c https://hg.mozilla.org/mozilla-central/rev/5e88d482f5e8 https://hg.mozilla.org/mozilla-central/rev/0e00d8a3ca1d https://hg.mozilla.org/mozilla-central/rev/5c01ab1d9790 https://hg.mozilla.org/mozilla-central/rev/7b157964c572 https://hg.mozilla.org/mozilla-central/rev/5abb37cb0495 https://hg.mozilla.org/mozilla-central/rev/4143cca42cc7 https://hg.mozilla.org/mozilla-central/rev/97352c48fb98 https://hg.mozilla.org/mozilla-central/rev/662ec8b0561e https://hg.mozilla.org/mozilla-central/rev/1db9723f1617

Backed out for frequent crashes/asserts in automation. https://treeherder.mozilla.org/logviewer.html#?job_id=10935850&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=10933526&repo=mozilla-inbound etc... https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e617011c42

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/b4e617011c42

https://hg.mozilla.org/integration/mozilla-inbound/rev/b32fcdc115b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/18c025c16bed https://hg.mozilla.org/integration/mozilla-inbound/rev/a103caa36183 https://hg.mozilla.org/integration/mozilla-inbound/rev/6318eba2d3fe https://hg.mozilla.org/integration/mozilla-inbound/rev/17d21020a786 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5df87d2ee5 https://hg.mozilla.org/integration/mozilla-inbound/rev/e74163801eef https://hg.mozilla.org/integration/mozilla-inbound/rev/91a3217d4cd0 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9561cbcd29 https://hg.mozilla.org/integration/mozilla-inbound/rev/fea908d8a836 https://hg.mozilla.org/integration/mozilla-inbound/rev/c5301aa7a2ae https://hg.mozilla.org/integration/mozilla-inbound/rev/4a65eeea4d9a https://hg.mozilla.org/integration/mozilla-inbound/rev/24966b67d232

Backed out for the same crashes as last time. Please verify that they are fixed on Try before pushing this again. https://treeherder.mozilla.org/logviewer.html#?job_id=10966733&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc988bd3c77

Going to try to land again. Try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36d678cef99 The ASAN J is fixed, other oranges look like known intermittents.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5037e0c47c70 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7fed87e4cb https://hg.mozilla.org/integration/mozilla-inbound/rev/8a416fedec44 https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f099600f57 https://hg.mozilla.org/integration/mozilla-inbound/rev/cf29e5d83bcd https://hg.mozilla.org/integration/mozilla-inbound/rev/924f41548f1a https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9252925e26 https://hg.mozilla.org/integration/mozilla-inbound/rev/569be66a0c9a https://hg.mozilla.org/integration/mozilla-inbound/rev/f19fec531e71 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfca45fd1f11 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9b3671def4 https://hg.mozilla.org/integration/mozilla-inbound/rev/63b4dd8b552e https://hg.mozilla.org/integration/mozilla-inbound/rev/43b82c9c8a9f

https://hg.mozilla.org/mozilla-central/rev/5037e0c47c70 https://hg.mozilla.org/mozilla-central/rev/ec7fed87e4cb https://hg.mozilla.org/mozilla-central/rev/8a416fedec44 https://hg.mozilla.org/mozilla-central/rev/f2f099600f57 https://hg.mozilla.org/mozilla-central/rev/cf29e5d83bcd https://hg.mozilla.org/mozilla-central/rev/924f41548f1a https://hg.mozilla.org/mozilla-central/rev/7f9252925e26 https://hg.mozilla.org/mozilla-central/rev/569be66a0c9a https://hg.mozilla.org/mozilla-central/rev/f19fec531e71 https://hg.mozilla.org/mozilla-central/rev/bfca45fd1f11 https://hg.mozilla.org/mozilla-central/rev/1c9b3671def4 https://hg.mozilla.org/mozilla-central/rev/63b4dd8b552e https://hg.mozilla.org/mozilla-central/rev/43b82c9c8a9f