Summary: | [JSC] Check NullSetterFunction under strict-mode context since structure / PropertyCondition are unaware of this | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2020-06-16 16:02:31 PDT
Created attachment 402056 [details]
Patch
Comment on attachment 402056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402056&action=review r=me > Source/JavaScriptCore/ChangeLog:26 > + we replace the calling target with special function which throws an error. In DFG / FTL, we emit `CheckNotJSCast` DFT node which typo: /DFT node/DFG node/ > Source/JavaScriptCore/bytecode/AccessCase.cpp:1606 > + // Note that GetterSetter alwyas has cells for both. If it is not set (like, getter exits, but setter is not set), Null{Getter,Setter}Function is stored. typo: /alwyas/always/ > Source/JavaScriptCore/bytecode/AccessCase.cpp:1614 > + // We replace getter with this AccessCase's JSGlobalObject::nullSetterStrictFunction, which will throw an error with the right JSGlobalObject. You're replacing the "setter" here, not the ""getter", right? > Source/JavaScriptCore/dfg/DFGStructureAbstractValue.cpp:358 > + // Note taht this function returns true if the structure set is empty. typo: /taht/that/. Same for StructureAbstractValue::isSubClassOf above. > Source/JavaScriptCore/runtime/NullSetterFunction.cpp:93 > + constexpr bool useCurrentFrame = false; At first I thought this is wrong, but then, I realized that this function is an InternalFunction, and therefore should be skipped. Can you write a short test to verify that error.stack's top frame entry is that of the JS function exercising the IC? Or augment one of your existing tests? Comment on attachment 402056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402056&action=review >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1606 >> + // Note that GetterSetter alwyas has cells for both. If it is not set (like, getter exits, but setter is not set), Null{Getter,Setter}Function is stored. > > typo: /alwyas/always/ Fixed. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1614 >> + // We replace getter with this AccessCase's JSGlobalObject::nullSetterStrictFunction, which will throw an error with the right JSGlobalObject. > > You're replacing the "setter" here, not the ""getter", right? Fixed. >> Source/JavaScriptCore/dfg/DFGStructureAbstractValue.cpp:358 >> + // Note taht this function returns true if the structure set is empty. > > typo: /taht/that/. Same for StructureAbstractValue::isSubClassOf above. Fixed. >> Source/JavaScriptCore/runtime/NullSetterFunction.cpp:93 >> + constexpr bool useCurrentFrame = false; > > At first I thought this is wrong, but then, I realized that this function is an InternalFunction, and therefore should be skipped. Can you write a short test to verify that error.stack's top frame entry is that of the JS function exercising the IC? Or augment one of your existing tests? Added. Committed r263134: <https://trac.webkit.org/changeset/263134> Committed r263165: <https://trac.webkit.org/changeset/263165> |