Closed Bug 1165486 Opened 9 years ago Closed 9 years ago

Add a static scope for dynamic polluting global scopes

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(14 files, 7 obsolete files)

Cleanup: use standard object allocation functions when allocating scope objects.
24.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Detect with scopes at parse time using the static scope chain instead of treating it as a polluted global.
2.31 KB, patch
luke
: review+
Details | Diff | Splinter Review
Cleanup: rename staticEvalScope to topStaticScope to prepare for the static top-level lexical scope.
10.73 KB, patch
luke
: review+
Details | Diff | Splinter Review
Rename hasPollutedGlobalScope to hasNonSyntacticScope.
34.50 KB, patch
luke
: review+
Details | Diff | Splinter Review
Add StaticNonSyntacticScopeObjects and teach scope iterators about it.
36.09 KB, patch
luke
: review+
Details | Diff | Splinter Review
Remove PollutedGlobalScopeOption in favor of using the static scope chain to detect non-syntactic scopes.
39.03 KB, patch
luke
: review+
Details | Diff | Splinter Review
Replace the PlainObj varobj with NonSyntacticVariablesObject.
17.46 KB, patch
luke
: review+
Details | Diff | Splinter Review
Split JS::Compile into JS::Compile and JS::CompileForNonSyntacticScope.
9.78 KB, patch
luke
: review+
Details | Diff | Splinter Review
Use JS::CompileForNonSyntacticScope in Gecko where we used to set polluted global scope.
3.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Restructure function and script cloning in light of PollutingGlobal scope changes.
51.88 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Detect with scopes at parse time using the static scope chain for non-function scripts. Also cache static scope properties on SharedGlobalContext.
9.65 KB, patch
efaust
: review+
Details | Diff | Splinter Review
Debug function to dump static scope chain of scripts.
4.65 KB, patch
efaust
: review+
Details | Diff | Splinter Review
Rebase yield offsets when cloning scripts.
4.21 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Fixup: use an RAII class to set the script during function script emission in case of error.
5.55 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
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.
Attachment #8613197 - Flags: review?(luke)
Attachment #8613193 - Flags: review?(luke)
Attachment #8613194 - Flags: review?(luke)
Attachment #8613195 - Flags: review?(luke)
Attachment #8613196 - Flags: review?(jwalden+bmo)
Assignee: nobody → shu
Status: NEW → ASSIGNED
(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 ...
  }
Attachment #8613194 - Flags: review?(luke) → review+
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.
Attachment #8613192 - Flags: review?(terrence) → review+
(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.
Attachment #8613826 - Attachment is obsolete: true
Attachment #8613826 - Flags: review?(luke)
Attachment #8613828 - Flags: review?(luke)
Renamed according to discussion with luke on IRC.
Attachment #8613193 - Attachment is obsolete: true
Attachment #8613193 - Flags: review?(luke)
Attachment #8615024 - Flags: review?(luke)
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.
Attachment #8613196 - Attachment is obsolete: true
Attachment #8613196 - Flags: review?(jwalden+bmo)
Attachment #8615026 - Flags: review?(jwalden+bmo)

        Attachment #8613828 -
        Attachment is obsolete: true

        Attachment #8613828 -
        Flags: review?(luke)

        Attachment #8615027 -
        Flags: review?(luke)

    
  

        Attachment #8615025 -
        Flags: review?(luke)

    
  

        Attachment #8615028 -
        Flags: review?(luke)

    
  

        Attachment #8615029 -
        Flags: review?(bobbyholley)

    
    

    
  
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.

        Attachment #8615029 -
        Flags: review?(bobbyholley) → review?(bzbarsky)

    
    

    
  
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

        Attachment #8615029 -
        Flags: review?(bzbarsky) → review+

    
  

        Attachment #8613197 -
        Flags: review?(luke) → review+

    
  

        Attachment #8615023 -
        Flags: review?(luke) → review+

    
    

    
  
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).

        Attachment #8615024 -
        Flags: review?(luke) → review+

    
    

    
  
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.

        Attachment #8615025 -
        Flags: review?(luke) → review+

    
    

    
  
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?

        Attachment #8615027 -
        Flags: review?(luke) → review+

    
  

        Attachment #8615028 -
        Flags: review?(luke) → review+

    
    

    
  


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

        Attachment #8615625 -
        Flags: review?(efaustbmo)

    
    

    
  


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

        Attachment #8615026 -
        Attachment is obsolete: true

        Attachment #8615026 -
        Flags: review?(jwalden+bmo)

        Attachment #8615627 -
        Flags: review?(jwalden+bmo)

    
    

    
  


  
Rebased on top of new.target stuff.

        Attachment #8615625 -
        Attachment is obsolete: true

        Attachment #8615625 -
        Flags: review?(efaustbmo)

        Attachment #8615643 -
        Flags: review?(efaustbmo)

    
    

    
  
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.

        Attachment #8615643 -
        Flags: review?(efaustbmo) → review+

    
    

    
  
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.

        Attachment #8621405 -
        Flags: review?(efaustbmo)

    
    

    
  
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.

        Attachment #8621405 -
        Flags: review?(efaustbmo) → review+

    
    

    
  
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.

        Attachment #8615627 -
        Flags: review?(jwalden+bmo) → review+

    
    

    
  
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

    
    

    
  


  
What was causing the Gu failures. Such sadness.

        Attachment #8623331 -
        Flags: review?(jdemooij)

    
  
Flags: needinfo?(shu)

    
  
Depends on: 1175397

    
    

    
  
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?

        Attachment #8623331 -
        Flags: review?(jdemooij) → review+

    
    

    
  
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.

    
    

    
  
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

    
  
Depends on: 1176430

        Attachment #8625117 -
        Flags: review?(jdemooij) → review+

    
    

    
  
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.
Depends on: 1183191

    
  
Depends on: 1199764

    
  
See Also:  → 1387719

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: