Bug 238030 – [JSC][MSVC] custom getter creation needs to include classInfo since MSVC ICF is not "safe" variant

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Pavel Feldman]

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

[Fujii Hironori]

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)

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/90639246>

[Yusuke Suzuki]

Created attachment 455477 [details]
Patch

[Yusuke Suzuki]

Created attachment 455478 [details]
Patch

[Alexey Shvayka]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

*** Bug 238180 has been marked as a duplicate of this bug. ***

[EWS]

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].

[Fujii Hironori]

*** Bug 236097 has been marked as a duplicate of this bug. ***