Summary: | [JSC] Remove reifyPropertyNameIfNeeded | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, ryanhaddad, saam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 179734, 180644 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2018-05-05 06:18:45 PDT
Note that reifyPropertyNameIfNeeded is only used by JSFunction. Created attachment 340081 [details]
Patch
Created attachment 340082 [details]
Patch
Created attachment 340083 [details]
Patch
Comment on attachment 340083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340083&action=review > Source/JavaScriptCore/ChangeLog:8 > + reifyPropertyNameIfNeeded is in the middle of putDirectInternal, which is super critical path. What was the original bug we were fixing with this again? Wasn't it just with setters/getters? Maybe let's just call it there? I'm really not a fan of this MethodTable method since it's super half baked. Comment on attachment 340083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340083&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + reifyPropertyNameIfNeeded is in the middle of putDirectInternal, which is super critical path. > > What was the original bug we were fixing with this again? Wasn't it just with setters/getters? Maybe let's just call it there? I'm really not a fan of this MethodTable method since it's super half baked. Hah. I see your patch does that. Comment on attachment 340083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340083&action=review > Source/JavaScriptCore/runtime/CommonSlowPaths.h:228 > +enum class RequireCacheStructreMode { Yes, No }; Since you're inlining this already, and you have the ability to templatize it, it feels like a more canonical way to do this would be to take a Structure** as a final parameter. And just default that to Structure**=nullptr. Since it's inlined, the compiler should remove the branch. > Source/JavaScriptCore/runtime/CommonSlowPaths.h:232 > + if (baseObject->inherits<JSFunction>(vm)) { Do we really not use reification in other classes? Comment on attachment 340083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340083&action=review >> Source/JavaScriptCore/runtime/CommonSlowPaths.h:228 >> +enum class RequireCacheStructreMode { Yes, No }; > > Since you're inlining this already, and you have the ability to templatize it, it feels like a more canonical way to do this would be to take a Structure** as a final parameter. And just default that to Structure**=nullptr. Since it's inlined, the compiler should remove the branch. Sounds nice. Fixed. >> Source/JavaScriptCore/runtime/CommonSlowPaths.h:232 >> + if (baseObject->inherits<JSFunction>(vm)) { > > Do we really not use reification in other classes? Right now, we do not have any `reifyPropertyNameIfNeeded` functions except for JSFunction. So this patch does not break the things. If we have another class which should needs reification, I think we should make this JSTypeInfo flag checking here (And add type flag). Created attachment 340170 [details]
Patch
It seems like ErrorInstance also reifies stuff. (In reply to Saam Barati from comment #10) > It seems like ErrorInstance also reifies stuff. I'm not sure it can run into whatever bug JF found originally or not. (In reply to Saam Barati from comment #11) > (In reply to Saam Barati from comment #10) > > It seems like ErrorInstance also reifies stuff. > > I'm not sure it can run into whatever bug JF found originally or not. I think this is OK. These functions are used for operations, which are invoked by bytecodes. And these bytecodes should be used for - object literals - class literals In object literals, the target is JSFinalObject. And in class literals with static functions, the target is a constructor, which is JSFunction. When we use putDirect for the other types of objects from operations, we do not accept arbitrary values as its inputs and property names. So, here, I think handling JSFunction is enough. Ping? Created attachment 340482 [details]
Patch
Comment on attachment 340482 [details] Patch Attachment 340482 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7699886 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html http/tests/preload/onload_event.html Created attachment 340515 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Ping? Comment on attachment 340482 [details]
Patch
r=me
Committed r231902: <https://trac.webkit.org/changeset/231902> This change introduced crashes on JSC Debug bots due to an unchecked exception: ERROR: Unchecked JS exception: This scope can throw a JS exception: putDirectAccessorWithReify @ /Volumes/Data/slave/highsierra-32bitJSC-debug/build/Source/JavaScriptCore/runtime/CommonSlowPaths.h:243 (ExceptionScope::m_recursionDepth was 9) But the exception was unchecked as of this scope: defineOwnProperty @ ./runtime/JSFunction.cpp:554 (ExceptionScope::m_recursionDepth was 9) https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1931 https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1024 (In reply to Ryan Haddad from comment #21) > This change introduced crashes on JSC Debug bots due to an unchecked > exception: > > ERROR: Unchecked JS exception: > This scope can throw a JS exception: putDirectAccessorWithReify @ > /Volumes/Data/slave/highsierra-32bitJSC-debug/build/Source/JavaScriptCore/ > runtime/CommonSlowPaths.h:243 > (ExceptionScope::m_recursionDepth was 9) > But the exception was unchecked as of this scope: defineOwnProperty @ > ./runtime/JSFunction.cpp:554 > (ExceptionScope::m_recursionDepth was 9) > > https://build.webkit.org/builders/Apple%20High%20Sierra%2032- > bit%20JSC%20%28BuildAndTest%29/builds/1931 > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1024 I'll check this soon. Committed r231957: <https://trac.webkit.org/changeset/231957> Committed r231976: <https://trac.webkit.org/changeset/231976> |