WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
https://bugs.webkit.org/show_bug.cgi?id=159332
Summary
Scopes that are not under TDZ should still push their variables onto the TDZ ...
Filip Pizlo
Reported
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".
Attachments
the patch
(13.37 KB, patch)
2016-06-30 23:26 PDT
,
Filip Pizlo
saam
: review+
Details
Formatted Diff
Diff
the patch
(15.70 KB, patch)
2016-07-01 20:13 PDT
,
Filip Pizlo
saam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Filip Pizlo
Comment 1
2016-06-30 23:26:19 PDT
Created
attachment 282514
[details]
the patch
Saam Barati
Comment 2
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.
Saam Barati
Comment 3
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.
Mark Lam
Comment 4
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.
David Kilzer (:ddkilzer)
Comment 5
2016-07-01 11:32:23 PDT
<
rdar://problem/27018958
>
Filip Pizlo
Comment 6
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.
Filip Pizlo
Comment 7
2016-07-01 20:13:37 PDT
Created
attachment 282634
[details]
the patch
Saam Barati
Comment 8
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.
Filip Pizlo
Comment 9
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.
Filip Pizlo
Comment 10
2016-07-02 10:47:01 PDT
Landed in
https://trac.webkit.org/changeset/202778
David Kilzer (:ddkilzer)
Comment 11
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug