Bug 159332 - Scopes that are not under TDZ should still push their variables onto the TDZ stack so that lifting TDZ doesn't bypass that scope
Summary: Scopes that are not under TDZ should still push their variables onto the TDZ ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-30 23:21 PDT by Filip Pizlo
Modified: 2016-07-02 19:23 PDT (History)
7 users (show)

See Also:


Attachments
the patch (13.37 KB, patch)
2016-06-30 23:26 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff
the patch (15.70 KB, patch)
2016-07-01 20:13 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-06-30 23:21:06 PDT
This fixes an instacrash in this code:

function foo () {
try{}catch(e){}print(e);let e;
}

foo();

We lift TDZ for "e" in "catch (e){}", but since that scope doesn't push anything onto the TDZ stack, we lift TDZ from "let e".
Comment 1 Filip Pizlo 2016-06-30 23:26:19 PDT
Created attachment 282514 [details]
the patch
Comment 2 Saam Barati 2016-07-01 07:59:21 PDT
Comment on attachment 282514 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282514&action=review

r=me
Oh wow, that's a huge bug. Nice find.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2019
> +void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment, TDZRequirement)

I'd either remove this last argument from the function or add an assert that it matches what's being removed.
Comment 3 Saam Barati 2016-07-01 08:30:42 PDT
Comment on attachment 282514 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282514&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2775
>      for (UniquedStringImpl* function : functionsToRemove)
>          environment.remove(function);
>  
> -    m_TDZStack.append(std::make_pair(WTFMove(environment), optimization));
> +    TDZStackEntry entry;
> +    entry.environment = WTFMove(environment);
> +    entry.optimization = optimization;
> +    entry.requirement = requirement;
> +    m_TDZStack.append(WTFMove(entry));

For added safety along the same lines of this patch, I think  we should also change the code here that removes functions from the environment.
And instead, we should not emit a TDZ check or lift a TDZ check if the entry in the environment is a function.

I can't quite get this to crash, I think because of how we emit block scoped functions, but I think that this provides better safety in general.
It also future proofs changes to how we emit block scoped functions.
Comment 4 Mark Lam 2016-07-01 09:24:29 PDT
Comment on attachment 282514 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282514&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2176
> -
> +    

Please undo this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2774
> +    TDZStackEntry entry;
> +    entry.environment = WTFMove(environment);
> +    entry.optimization = optimization;
> +    entry.requirement = requirement;

Can you use a constructor here instead?  That guarantees that all fields get initialized.
Comment 5 David Kilzer (:ddkilzer) 2016-07-01 11:32:23 PDT
<rdar://problem/27018958>
Comment 6 Filip Pizlo 2016-07-01 19:30:02 PDT
(In reply to comment #3)
> Comment on attachment 282514 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282514&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2775
> >      for (UniquedStringImpl* function : functionsToRemove)
> >          environment.remove(function);
> >  
> > -    m_TDZStack.append(std::make_pair(WTFMove(environment), optimization));
> > +    TDZStackEntry entry;
> > +    entry.environment = WTFMove(environment);
> > +    entry.optimization = optimization;
> > +    entry.requirement = requirement;
> > +    m_TDZStack.append(WTFMove(entry));
> 
> For added safety along the same lines of this patch, I think  we should also
> change the code here that removes functions from the environment.
> And instead, we should not emit a TDZ check or lift a TDZ check if the entry
> in the environment is a function.
> 
> I can't quite get this to crash, I think because of how we emit block scoped
> functions, but I think that this provides better safety in general.
> It also future proofs changes to how we emit block scoped functions.

Interesting.  I think I know how to fix all of these issues.  I'll post a new patch soon.
Comment 7 Filip Pizlo 2016-07-01 20:13:37 PDT
Created attachment 282634 [details]
the patch
Comment 8 Saam Barati 2016-07-01 23:06:39 PDT
Comment on attachment 282634 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282634&action=review

r=me with a suggestion

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2767
> +        map.add(entry.key, entry.value.isFunction() ? TDZNecessityLevel::NotNeeded: level);

Style: missing a space before :

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2774
> +    // NOTE: This is conservative. If called at "...", it will report "x" as being under TDZ:

This is indeed a bit unfortunate. Can you just write it so that you have a SmallPtrSet<UniquedStringImpl*> that represents things we've seen that don't need TDZ so we can make it optimal?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:892
> +        Vector<TDZMap> m_TDZStack;

Nice. I like this.
Comment 9 Filip Pizlo 2016-07-02 10:40:49 PDT
(In reply to comment #8)
> Comment on attachment 282634 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282634&action=review
> 
> r=me with a suggestion
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2767
> > +        map.add(entry.key, entry.value.isFunction() ? TDZNecessityLevel::NotNeeded: level);
> 
> Style: missing a space before :

Fixed.

> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2774
> > +    // NOTE: This is conservative. If called at "...", it will report "x" as being under TDZ:
> 
> This is indeed a bit unfortunate. Can you just write it so that you have a
> SmallPtrSet<UniquedStringImpl*> that represents things we've seen that don't
> need TDZ so we can make it optimal?

This isn't a regression, so I think we should fix this issue in a different patch.
Filed: https://bugs.webkit.org/show_bug.cgi?id=159387

> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:892
> > +        Vector<TDZMap> m_TDZStack;
> 
> Nice. I like this.
Comment 10 Filip Pizlo 2016-07-02 10:47:01 PDT
Landed in https://trac.webkit.org/changeset/202778
Comment 11 David Kilzer (:ddkilzer) 2016-07-02 19:23:46 PDT
(In reply to comment #10)
> Landed in https://trac.webkit.org/changeset/202778

Follow-up fix in r202781:  <https://trac.webkit.org/changeset/202781>