<rdar://problem/55134229>
Created attachment 378259 [details] proposed patch.
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review > Source/bmalloc/ChangeLog:23 > + 3. Use a storeStore fence in freezeGigacageConfig() and permanentlyFreezeGigacageConfig() > + to ensure that values in the Config has been written out before we freeze it. this doesn't sound necessary. The CPU would be wrong if it moved such a store below whatever the kernel is doing
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review > Source/bmalloc/bmalloc/Gigacage.cpp:86 > + std::atomic_thread_fence(std::memory_order_seq_cst); yeah this seems dubious and unneeded.
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review > Source/bmalloc/bmalloc/Gigacage.cpp:121 > + result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, true, VM_PROT_READ); can you name "true" here?
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review >> Source/bmalloc/ChangeLog:23 >> + to ensure that values in the Config has been written out before we freeze it. > > this doesn't sound necessary. The CPU would be wrong if it moved such a store below whatever the kernel is doing I was thinking that, but wanted to be get 2nd opinions to be doubly sure. Will remove. >> Source/bmalloc/bmalloc/Gigacage.cpp:121 >> + result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, true, VM_PROT_READ); > > can you name "true" here? Sure. I'll use an enum with descriptive names like AllowPermissionChangesAfterThis and DisallowPermissionChangesAfterThis.
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review > Source/bmalloc/ChangeLog:20 > + On OS(DARWIN), this is made possible by using vm_protect with a true > + set_maximum argument. We also add a g_gigacageConfig.isPermanentlyFrozen flag > + that we assert. you should explain what this is for DARWIN > Source/bmalloc/bmalloc/Gigacage.cpp:90 > + result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, false, VM_PROT_READ); let's name "false" here > Source/bmalloc/bmalloc/Gigacage.cpp:116 > + std::atomic_thread_fence(std::memory_order_seq_cst); also is unneeded. Is is not expected that multiple threads can call this at the same time, right?
Comment on attachment 378259 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=378259&action=review Thanks for the review. >> Source/bmalloc/ChangeLog:20 >> + that we assert. > > you should explain what this is for DARWIN Will do. >> Source/bmalloc/bmalloc/Gigacage.cpp:90 >> + result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, false, VM_PROT_READ); > > let's name "false" here Done. >> Source/bmalloc/bmalloc/Gigacage.cpp:116 >> + std::atomic_thread_fence(std::memory_order_seq_cst); > > also is unneeded. Is is not expected that multiple threads can call this at the same time, right? Removed. It's expected that only the main thread should call forbidDisablingPrimitiveGigacage() (and therefore, permanentlyFreezeGigacageConfig()). We're not expecting other threads to call this.
Created attachment 378264 [details] patch for landing.
Created attachment 378265 [details] patch for landing.
Created attachment 378276 [details] patch for landing.
Landed in r249608: <http://trac.webkit.org/r249608>.
FYI, fixed disabled-gigacage-* failures in https://bugs.webkit.org/show_bug.cgi?id=201577.
(In reply to Mark Lam from comment #12) > FYI, fixed disabled-gigacage-* failures in > https://bugs.webkit.org/show_bug.cgi?id=201577. Correction: fixed disabled-gigacage-* test failures in https://bugs.webkit.org/show_bug.cgi?id=201579.
I am getting a lot of crashes running the layout tests: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [9131] Application Specific Information: This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000740256780 Gigacage::forbidDisablingPrimitiveGigacage() + 32 1 com.apple.WebKit 0x000000010a5c987e WebKit::WebProcess::WebProcess() + 1246 (WebProcess.cpp:224) 2 com.apple.WebKit 0x000000010a5c9395 WebKit::WebProcess::WebProcess() + 21 (WebProcess.cpp:224) 3 com.apple.WebKit 0x000000010a5c933b WebKit::WebProcess::singleton() + 43 (WebProcess.cpp:176) 4 com.apple.WebKit 0x000000010a39c811 void WebKit::initializeAuxiliaryProcess<WebKit::WebProcess>(WebKit::AuxiliaryProcessInitializationParameters&&) + 17 (XPCServiceEntryPoint.h:73) 5 com.apple.WebKit 0x000000010a399689 void WebKit::XPCServiceInitializer<WebKit::WebProcess, WebKit::XPCServiceInitializerDelegate>(WTF::OSObjectPtr<NSObject<OS_xpc_object>*>, NSObject<OS_xpc_object>*, NSObject<OS_xpc_object>*) + 921 (XPCServiceEntryPoint.h:128) 6 com.apple.WebKit 0x000000010a3992b1 WebContentServiceInitializer + 81 (WebContentServiceEntryPoint.mm:53) 7 com.apple.WebKit 0x0000000109d3d00a invocation function for block in WebKit::XPCServiceEventHandler(NSObject<OS_xpc_object>*) + 650 (XPCServiceMain.mm:79) 8 libxpc.dylib 0x00007fff650d9078 _xpc_connection_call_event_handler + 56 9 libxpc.dylib 0x00007fff650d7268 _xpc_connection_mach_event + 927 10 libdispatch.dylib 0x00007fff64e3e57e _dispatch_client_callout4 + 9 11 libdispatch.dylib 0x00007fff64e53aeb _dispatch_mach_msg_invoke + 435 12 libdispatch.dylib 0x00007fff64e43950 _dispatch_lane_serial_drain + 263 13 libdispatch.dylib 0x00007fff64e5463e _dispatch_mach_invoke + 481 14 libdispatch.dylib 0x00007fff64e49a91 _dispatch_main_queue_callback_4CF + 783 15 com.apple.CoreFoundation 0x00007fff2db5bd00 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9 16 com.apple.CoreFoundation 0x00007fff2db5b44a __CFRunLoopRun + 2370 17 com.apple.CoreFoundation 0x00007fff2db5a883 CFRunLoopRunSpecific + 499 18 com.apple.Foundation 0x00007fff301e9e2d -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212 19 com.apple.Foundation 0x00007fff301e9d46 -[NSRunLoop(NSRunLoop) run] + 76 20 libxpc.dylib 0x00007fff650f49d6 _xpc_objc_main.cold.4 + 49 21 libxpc.dylib 0x00007fff650dd7e1 _xpc_objc_main + 559 22 libxpc.dylib 0x00007fff650dd2fc xpc_main + 377 23 com.apple.WebKit 0x0000000109d32cea WebKit::XPCServiceMain(int, char const**) + 1322 (XPCServiceMain.mm:147) 24 com.apple.WebKit 0x000000010accc12b WKXPCServiceMain + 27 (WKMain.mm:33) 25 com.apple.WebKit.WebContent 0x00000001095baeb2 main + 34 (AuxiliaryProcessMain.cpp:30) 26 libdyld.dylib 0x00007fff64e8e2a5 start + 1
Unfortunately, EWS does not run WK2 debug.
(In reply to Chris Dumez from comment #14) > I am getting a lot of crashes running the layout tests: ... > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x0000000740256780 > Gigacage::forbidDisablingPrimitiveGigacage() + 32 > 1 com.apple.WebKit 0x000000010a5c987e > WebKit::WebProcess::WebProcess() + 1246 (WebProcess.cpp:224) ... I'm investigating.
Created attachment 378306 [details] follow up fix patch.
Thanks for the review. Follow up fix landed in r249621: <http://trac.webkit.org/r249621>.