Closed Bug 1144743 Opened 5 years ago Closed 5 years ago

Add a hasPollutedScope flag to scripts

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files, 1 obsolete file)

We want this to drop compile-and-go.
Summary: Add a hasNonSyntacticGlobalScope flag to scripts → Add a hasPollutedScope flag to scripts
Blocks: 1144802
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8579824 - Flags: review?(luke) → review+
Comment on attachment 8579825 [details] [diff] [review]
part 2.  Add an option to JS::CompileOptions for hasPollutedScope

Review of attachment 8579825 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +3491,4 @@
>      OwningCompileOptions &setUTF8(bool u) { utf8 = u; return *this; }
>      OwningCompileOptions &setColumn(unsigned c) { column = c; return *this; }
>      OwningCompileOptions &setCompileAndGo(bool cng) { compileAndGo = cng; return *this; }
> +    OwningCompileOptions &setHasPollutedScope(bool p) { hasPollutedScope = p; return *this; }

Ah, so, iiuc, JSAPI users *can* now preemptively compile their scripts as having polluted scopes to avoid an inevitable clone.  Importantly, while it will affect what bytecode gets emitted, it's purely an optimization and does not affect correctness.
Attachment #8579825 - Flags: review?(luke) → review+
Attachment #8579826 - Flags: review?(luke) → review+
> Ah, so, iiuc, JSAPI users *can* now preemptively compile their scripts as having polluted
> scopes to avoid an inevitable clone.  Importantly, while it will affect what bytecode
> gets emitted, it's purely an optimization and does not affect correctness.

Correct on all counts.
Comment on attachment 8579827 [details] [diff] [review]
part 4.  Set the hasPollutedScope flag correctly when compiling functions

Review of attachment 8579827 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +3999,5 @@
>      if (!fun)
>          return false;
>  
> +    // Make sure to handle cases when we have a polluted scopechain.
> +    // Avoid doing the copy if we can.

I expect the time spent unconditionally copying the CompileOptions would be fully amortized by the massive expense of parsing (even for tiny bodies) and so it's not worth doing the Maybe dance.
Attachment #8579827 - Flags: review?(luke) → review+
Though actually, js::ExecuteInGlobalAndReturnScope will release-assert that the incoming script has a polluted scope, since there is absolutely no case in which it makes sense to do that to a script with an unpolluted scope.  I think that's OK; this API is pretty special snowflake.
Hmm.  I was looking at the PersistentRooteds in the OwningCompileOptions and worrying about their cost.  I don't have a good feel for how much that is nowadays....
Comment on attachment 8579828 [details] [diff] [review]
part 5.  Set the hasPollutedScope flag correctly when cloning functions

Review of attachment 8579828 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +2098,5 @@
> +    // chain terminator, since in that case we have some actual scope objects on
> +    // our scope chain and whatnot; whoever put them there should be responsible
> +    // for setting our script's flags appropriately.
> +    if (!IsValidTerminatingScope(newParent))
> +        return true;

Maybe this will go away with the new patch, but I don't understand why this test is necessary.

::: js/src/jsobj.cpp
@@ +2151,4 @@
>          value = srcArray->getDenseElement(i);
>          MOZ_ASSERT_IF(value.isMarkable(),
>                        value.toGCThing()->isTenured() &&
> +                      cx->runtime()->isAtomsZone(value.toGCThing()->asTenured().zoneFromAnyThread()));

I'm curious why this patch requires this.

::: js/src/jsscript.h
@@ +753,4 @@
>  
>  JSScript *
>  CloneScript(JSContext *cx, HandleObject enclosingScope, HandleFunction fun, HandleScript script,
> +            bool hasPollutedScope = false, NewObjectKind newKind = GenericObject);

Could you instead create a PollutedScopeOption enum?  This would replace all the /* hasPollutedScope = */ comments.
> but I don't understand why this test is necessary.

Two things going on there:

1)  Without that test, JSOP_LAMBDA was leading to script-cloning, because it does a CloneFunctionObject with a parent that's a Call object, which is clearly not a global.  This seemed highly undesirable to me.

2)  I discovered that JSOP_LAMBDA was leading to script cloning because the array path of CloneObjectLiteral is totally broken and caused crashes/asserts in automation when invoked during said script-cloning.  See also bug 1143269.

I will add comments about item 1 here, I guess.

> I'm curious why this patch requires this.

It doesn't.  I was looking at this code because of said asserts in automation, and the code as written is obviously wrong: if the value _is_ from the atoms zone, then the zone() call will immediately assert.  I can split this one hunk into a separate bug if you'd like.

> Could you instead create a PollutedScopeOption enum?

OK.
Comment on attachment 8579828 [details] [diff] [review]
part 5.  Set the hasPollutedScope flag correctly when cloning functions

Review of attachment 8579828 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +2103,5 @@
> +
> +    // We need to clone the script if we're interpreted and not
> +    // already marked as having a polluted scope.
> +    return !fun->isInterpreted() ||
> +           (fun->hasScript() && fun->nonLazyScript()->hasPollutedScope());

So, documenting IRC discussion, we need to:
 1. have CanLazilyParse() return false when hasPollutedScope
 2. assert !lazyScript && !isRelazifiable() for a cloned script if clone->hasPollutedScript w/ comment explaining why
Comment on attachment 8579829 [details] [diff] [review]
part 6.  Set the hasPollutedScope flag correctly when executing scripts

Review of attachment 8579829 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter.cpp
@@ +640,5 @@
>          AutoSuppressGC nogc(cx);
>          MOZ_ASSERT(GetOuterObject(cx, thisObj) == thisObj);
>      }
> +    RootedObject terminatingScope(cx, &scopeChainArg);
> +    while (!IsValidTerminatingScope(terminatingScope))

Do you suppose, in this or a followup bug, we could rename "terminating scope" (everywhere)?  It confuses me every time until I remember "first non-syntactic scope" (considering the global object as non-syntactic).  Ideally, I think, we'd define it right below JSObject::enclosingScope(), invert it, and call it 'IsSynctacticScope' and rename JSObject::enclosingScope() to JSObject::syntacticEnclosingScope() (and have it assert(IsSyntacticScope(*this)).  Ideally we'd actually have a SyntacticScopeObject so we could is<SyntacticScopeObject>(), but I think the fact that DebugScopeProxy is a proxy makes that hard.  So weak typing.

Also, JSObject::enclosingScope() returns global() for function objects (you were right, first introduced by http://hg.mozilla.org/mozilla-central/rev/37d8d0362318#l5.12) which seems so wrong that I can't imagine anyone depends on this so we could just remove the function case.
(In reply to Not doing reviews right now from comment #13)
Ah, yes.  The fact that we reuse the same path for both the totally-normal-an-must-be-fast lambda case and the totally-rare-and-kinda-expensive JSAPI case has always bothered me.  Do you see any clean way to cleave the two?  Also, what happens in the top-level JSOP_LAMBDA case (where there is no syntactic scope object)?

> It doesn't.  I was looking at this code because of said asserts in
> automation, and the code as written is obviously wrong: if the value _is_
> from the atoms zone, then the zone() call will immediately assert.  I can
> split this one hunk into a separate bug if you'd like.

Oh, not necessary to split out, just wanted to understand.  Your existing patch separation is already just beautiful.
> Could you instead create a PollutedScopeOption enum?  

enum PollutedGlobalScopeOption {
    HasPollutedGlobalScope,
    HasCleanGlobalScope
};

in namespace js in jsscript.h, with the /* = ... */ bits now using /* = HasCleanGlobalScope */.

Let me know if you have better naming suggestions.
> Do you suppose, in this or a followup bug, we could rename "terminating scope" 

Bug 1145282.

> rename JSObject::enclosingScope() to JSObject::syntacticEnclosingScope() 

enclosingScope will walk up the non-syntactic scope chain too.


> Also, JSObject::enclosingScope() returns global() for function objects 

Imo, it's not reached at all for function objects.  It's just asserting something about them in case it is; a leftover from parent removal....  As discussed, we'll deal with this in bug 1143794.

> Do you see any clean way to cleave the two?

Apart from Yet Another Argument?  :(

> Also, what happens in the top-level JSOP_LAMBDA case (where there is no syntactic scope
> object)?

Then newParent->is<GlobalObject>() tests true, no?  And we take that early return.
Flags: needinfo?(luke)
Blocks: 1145294
Attachment #8579828 - Attachment is obsolete: true
Attachment #8579828 - Flags: review?(luke)
Comment on attachment 8580276 [details] [diff] [review]
part 5.  Set the hasPollutedGlobalScope flag correctly when cloning functions

Review of attachment 8580276 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsscript.cpp
@@ +3199,5 @@
>  
>      clone->mutableScript().init(nullptr);
>  
> +    JSScript *cscript = CloneScript(cx, scope, clone, script,
> +                                    pollutedGlobalScopeOption, newKind);

I think it'd be fine to use 'polluted' as the arg name everywhere (between the type and enumerator names, we're already super-readable) so we can wrap less here and in other places.
Attachment #8580276 - Flags: review?(luke) → review+
Comment on attachment 8579829 [details] [diff] [review]
part 6.  Set the hasPollutedScope flag correctly when executing scripts

Review of attachment 8579829 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +4159,4 @@
>      assertSameCompartment(cx, obj);
>      RootedScript script(cx, scriptArg);
>      if (script->compartment() != cx->compartment()) {
> +        script = CloneScript(cx, NullPtr(), NullPtr(), script, !obj->is<GlobalObject>());

I assume these callsites will be updated to the enumerator names?

::: js/src/vm/Interpreter.cpp
@@ +640,5 @@
>          AutoSuppressGC nogc(cx);
>          MOZ_ASSERT(GetOuterObject(cx, thisObj) == thisObj);
>      }
> +    RootedObject terminatingScope(cx, &scopeChainArg);
> +    while (!IsValidTerminatingScope(terminatingScope))

Do you suppose we could rename "terminating scope" (everywhere)?  It confuses me every time until I remember "first non-syntactic scope" (considering the global object as non-syntactic which).
Attachment #8579829 - Flags: review?(luke) → review+
Flags: needinfo?(luke)
> I think it'd be fine to use 'polluted' as the arg name everywhere

OK, will do.

> I assume these callsites will be updated to the enumerator names?

Yes.

> Do you suppose we could rename "terminating scope" (everywhere)? 

You asked that once already.  ;) I really will.  Bug 1145282.
(In reply to Not doing reviews right now from comment #22)
> > Do you suppose we could rename "terminating scope" (everywhere)? 
> You asked that once already.  ;) I really will.  Bug 1145282.

Oh, hah, that was a lingering comment from when I started reviewing this bug before switching to the other bug that I forgot to delete.  I trust you :)
Blocks: 1145488
Blocks: 1145491
Comment on attachment 8580276 [details] [diff] [review]
part 5.  Set the hasPollutedGlobalScope flag correctly when cloning functions

Turns out, this part was somewhat buggy because it did not propagate the polluted state to the scripts of the lambdas in the script being cloned...

That's getting fixed in bug 1145491.
You need to log in before you can comment on or make changes to this bug.