1144743 - Add a hasPollutedScope flag to scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We want this to drop compile-and-go.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a hasPollutedScope flag to scripts

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add an option to JS::CompileOptions for hasPollutedScope

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Set hasPollutedScope when we're compiling scripts we know will be executed with a non-global scope without cloning them

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Set the hasPollutedScope flag correctly when compiling functions

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Set the hasPollutedScope flag correctly when cloning functions

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Set the hasPollutedScope flag correctly when executing scripts

Comment 7

[Luke Wagner [:luke]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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 9

[Luke Wagner [:luke]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Luke Wagner [:luke]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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 14

[Luke Wagner [:luke]]

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 15

[Luke Wagner [:luke]]

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.

Comment 16

[Luke Wagner [:luke]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Set the hasPollutedGlobalScope flag correctly when cloning functions

Comment 20

[Luke Wagner [: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.

Comment 21

[Luke Wagner [:luke]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Luke Wagner [:luke]]

(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 :)

Comment 24

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df51f5c349f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d67adea5f54e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f6eef1a0cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1962bdd6fb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbc8769b1e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e1e34538124

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/df51f5c349f0
https://hg.mozilla.org/mozilla-central/rev/d67adea5f54e
https://hg.mozilla.org/mozilla-central/rev/f3f6eef1a0cc
https://hg.mozilla.org/mozilla-central/rev/f1962bdd6fb8
https://hg.mozilla.org/mozilla-central/rev/4fbc8769b1e5
https://hg.mozilla.org/mozilla-central/rev/0e1e34538124