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".
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.
<rdar://problem/27018958>
(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>