Summary: | Scopes that are not under TDZ should still push their variables onto the TDZ stack so that lifting TDZ doesn't bypass that scope | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, ddkilzer, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Filip Pizlo
2016-06-30 23:21:06 PDT
Created attachment 282514 [details]
the patch
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 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 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. (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. Created attachment 282634 [details]
the patch
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. (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. Landed in https://trac.webkit.org/changeset/202778 (In reply to comment #10) > Landed in https://trac.webkit.org/changeset/202778 Follow-up fix in r202781: <https://trac.webkit.org/changeset/202781> |