Bug 238030

Summary: [JSC][MSVC] custom getter creation needs to include classInfo since MSVC ICF is not "safe" variant
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ashvayka, ews-watchlist, Hironori.Fujii, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Windows 10   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236097
Attachments:
Description Flags
Patch
none
Patch none

Description Pavel Feldman 2022-03-17 11:05:32 PDT
This is windows-specific

1. Navigate to https://console.cloudmanagementtest.apteco.com/authentication/login
2. Observe blank page

Bisect results:
First bad: c888e174bc83 Reland StructureID overhaul https://bugs.webkit.org/show_bug.cgi?id=235720
Last good: ef9e9e97b731 Support additional WPEToolingBackend types https://bugs.webkit.org/show_bug.cgi?id=235745

Chane:
https://bugs.webkit.org/show_bug.cgi?id=235720

Error in console:
[Error] ERROR – Error: Uncaught (in promise): TypeError: The Document.onerror getter can only be used on instances of Document
onerror@[native code]
@https://console.cloudmanagementtest.apteco.com/polyfills.573e766b24bdf168.js:1:12802
@https://console.cloudmanagementtest.apteco.com/runtime.14d64c90b81ec588.js:1:1605
@https://console.cloudmanagementtest.apteco.com/runtime.14d64c90b81ec588.js:1:2298
@https://console.cloudmanagementtest.apteco.com/runtime.14d64c90b81ec588.js:1:693
reduce@[native code]
@https://console.cloudmanagementtest.apteco.com/runtime.14d64c90b81ec588.js:1:678
loadChildren@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:6685
loadModuleFactory@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:684262
load@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:683935
@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:676270
te@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:44320
@https://console.cloudmanagementtest.apteco.com/main.a57817f372c44157.js:1:41836
Comment 1 Fujii Hironori 2022-03-17 13:28:40 PDT
Setting a env var JSC_useJIT to 0 has no effect for this issue.
WinCairo Debug (64bit, compiled by MSVC) can load the page successfully. (See also Bug 236097)
Comment 2 Fujii Hironori 2022-03-17 16:47:26 PDT
I confirmed WinCairo (64bit, Release) compiled by clang-cl also has the same issue.
I used attachment#455038 [details] patch to compile with clang-cl.
Comment 3 Fujii Hironori 2022-03-17 16:48:14 PDT
I confirmed the same error without VirtualAlloc2.

diff --git a/Source/WTF/wtf/win/OSAllocatorWin.cpp b/Source/WTF/wtf/win/OSAllocatorWin.cpp
index b7f4eddd288d..96133a38f5ae 100644
--- a/Source/WTF/wtf/win/OSAllocatorWin.cpp
+++ b/Source/WTF/wtf/win/OSAllocatorWin.cpp
@@ -60,6 +60,7 @@ void* OSAllocator::tryReserveUncommittedAligned(size_t bytes, size_t alignment,
 {
     ASSERT(hasOneBitSet(alignment) && alignment >= pageSize());
 
+#if 0    
     if (VirtualAlloc2Ptr()) {
         MEM_ADDRESS_REQUIREMENTS addressReqs = { };
         MEM_EXTENDED_PARAMETER param = { };
@@ -69,6 +70,7 @@ void* OSAllocator::tryReserveUncommittedAligned(size_t bytes, size_t alignment,
         void* result = VirtualAlloc2Ptr()(nullptr, nullptr, bytes, MEM_RESERVE, protection(writable, executable), &param, 1);
         return result;
     }
+#endif
 
     void* result = tryReserveUncommitted(bytes + alignment);
     // There's no way to release the reserved memory on Windows, from what I can tell as the whole segment has to be released at once.
Comment 4 Fujii Hironori 2022-03-18 00:21:20 PDT
(In reply to Fujii Hironori from comment #2)
> I confirmed WinCairo (64bit, Release) compiled by clang-cl also has the same
> issue.
> I used attachment#455038 [details] patch to compile with clang-cl.

I confirmed WinCairo (64bit, Debug) compiled by clang-cl can load the page successfully.
Comment 5 Radar WebKit Bug Importer 2022-03-22 09:08:12 PDT
<rdar://problem/90639246>
Comment 6 Yusuke Suzuki 2022-03-23 02:38:57 PDT
Created attachment 455477 [details]
Patch
Comment 7 Yusuke Suzuki 2022-03-23 02:57:19 PDT
Created attachment 455478 [details]
Patch
Comment 8 Alexey Shvayka 2022-03-23 10:57:19 PDT
Comment on attachment 455478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455478&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        Since JSCustomGetterFunction does separate thing based on attached DOMAttribute, we need to include const ClassInfo*

There is no way we can use slotBase() pointer instead, right? To fix the issue for all possibly accessors rather than only DOMAttribute?
Comment 9 Yusuke Suzuki 2022-03-23 11:00:20 PDT
Comment on attachment 455478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455478&action=review

>> Source/JavaScriptCore/ChangeLog:12
>> +        Since JSCustomGetterFunction does separate thing based on attached DOMAttribute, we need to include const ClassInfo*
> 
> There is no way we can use slotBase() pointer instead, right? To fix the issue for all possibly accessors rather than only DOMAttribute?

I think this is fine: if DOMAttribute is not attached, the function itself includes ClassInfo check. This means that each function is not identical.
Only case we get identical code for function is, DOMAttribute is attached and ClassInfo check is delegated into JSCustomGetterFunction side.
Comment 10 Yusuke Suzuki 2022-03-23 11:43:32 PDT
*** Bug 238180 has been marked as a duplicate of this bug. ***
Comment 11 EWS 2022-03-23 11:48:32 PDT
Committed r291756 (248787@main): <https://commits.webkit.org/248787@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455478 [details].
Comment 12 Fujii Hironori 2022-03-23 12:44:06 PDT
*** Bug 236097 has been marked as a duplicate of this bug. ***