Bug 187160

<rdar://problem/36029149>

Created attachment 343858 [details] Patch

Comment on attachment 343858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343858&action=review > Source/bmalloc/ChangeLog:7 > + nit: The description you put in bugzilla for this bug should go here.

(In reply to Michael Saboff from comment #0) > If Gigacage is disabled, it may be due to lack of address space. Therefore > we should also turn off IsoHeaps. Also probably worth stating that we expect IsoHeaps to use more virtual memory than not using IsoHeaps and why

(In reply to Michael Saboff from comment #0) > If Gigacage is disabled, it may be due to lack of address space. Therefore > we should also turn off IsoHeaps. Also probably worth stating that we expect IsoHeaps to use more virtual memory than not using IsoHeaps and why

Comment on attachment 343858 [details] Patch Attachment 343858 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8376450 Number of test failures exceeded the failure limit.

Created attachment 343879 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6

Committed r233347: <https://trac.webkit.org/changeset/233347>

(In reply to Michael Saboff from comment #8) > Committed r233347: <https://trac.webkit.org/changeset/233347> This change causes crashes on WK1 (seen with LayoutTests and API tests) https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/builds/4479 Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010f7af3e3 bmalloc::PerThread<bmalloc::PerHeapKind<bmalloc::Cache> >::getSlowCase() + 35 (PerThread.h:143) 1 com.apple.JavaScriptCore 0x000000010f7af384 bmalloc::Cache::tryAllocateSlowCaseNullCache(bmalloc::HeapKind, unsigned long) + 20 (Cache.cpp:53) 2 com.apple.WebCore 0x000000011dd45c0f bmalloc::Cache::tryAllocate(bmalloc::HeapKind, unsigned long) + 47 (Cache.h:70) 3 com.apple.WebCore 0x000000011dd45abb bmalloc::api::tryMalloc(unsigned long, bmalloc::HeapKind) + 27 (bmalloc.h:43) 4 com.apple.WebCore 0x00000001202065fb void* bmalloc::IsoTLS::allocateSlow<bmalloc::IsoConfig<1320u>, WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 107 (IsoTLSInlines.h:90) 5 com.apple.WebCore 0x0000000120206545 void* bmalloc::IsoTLS::allocateImpl<bmalloc::IsoConfig<1320u>, WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 85 (IsoTLSInlines.h:71) 6 com.apple.WebCore 0x00000001202064e5 void* bmalloc::IsoTLS::allocate<WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 37 (IsoTLSInlines.h:37) 7 com.apple.WebCore 0x00000001201db271 bmalloc::api::IsoHeap<WebCore::FrameView>::allocate() + 33 (IsoHeapInlines.h:50) 8 com.apple.WebCore 0x00000001201db245 WebCore::FrameView::operator new(unsigned long) + 69 (FrameView.cpp:125) 9 com.apple.WebCore 0x00000001201d681a WebCore::FrameView::create(WebCore::Frame&) + 42 (FrameView.cpp:225) 10 com.apple.WebKitLegacy 0x000000011a45bfc5 WebFrameLoaderClient::transitionToCommittedForNewPage() + 869 (WebFrameLoaderClient.mm:1476) 11 com.apple.WebCore 0x000000012001c92f WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) + 1151 (FrameLoader.cpp:2135) 12 com.apple.WebCore 0x000000012001b979 WebCore::FrameLoader::commitProvisionalLoad() + 2153 (FrameLoader.cpp:1954) 13 com.apple.WebCore 0x000000011ffc440c WebCore::DocumentLoader::commitIfReady() + 60 (DocumentLoader.cpp:359) 14 com.apple.WebCore 0x000000011ffc4772 WebCore::DocumentLoader::finishedLoading() + 306 (DocumentLoader.cpp:422) 15 com.apple.WebCore 0x000000011ffce799 WebCore::DocumentLoader::maybeLoadEmpty() + 1241 (DocumentLoader.cpp:1672) 16 com.apple.WebCore 0x000000011ffce958 WebCore::DocumentLoader::startLoadingMainResource(WebCore::ShouldContinue) + 360 (DocumentLoader.cpp:1688) 17 com.apple.WebCore 0x000000012000ea7e WebCore::FrameLoader::init() + 302 (FrameLoader.cpp:312) 18 com.apple.WebCore 0x00000001201d2284 WebCore::Frame::init() + 36 (Frame.cpp:204) 19 com.apple.WebKitLegacy 0x000000011a44705e +[WebFrame(WebInternal) _createMainFrameWithPage:frameName:frameView:] + 270 (WebFrame.mm:332) 20 com.apple.WebKitLegacy 0x000000011a55f50b -[WebView(WebPrivate) _commonInitializationWithFrameName:groupName:] + 6443 (WebView.mm:1511) 21 com.apple.WebKitLegacy 0x000000011a5608c9 -[WebView(WebPrivate) _initWithFrame:frameName:groupName:] + 297 (WebView.mm:1602) 22 com.apple.WebKitLegacy 0x000000011a573779 -[WebView initWithFrame:frameName:groupName:] + 313 (WebView.mm:5835) 23 DumpRenderTree 0x000000010edb39af createWebViewAndOffscreenWindow() + 255 (DumpRenderTree.mm:705) 24 DumpRenderTree 0x000000010edb6150 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2368 (DumpRenderTree.mm:1950) 25 DumpRenderTree 0x000000010edb5761 runTestingServerLoop() + 417 (DumpRenderTree.mm:1166) 26 DumpRenderTree 0x000000010edb4c1e dumpRenderTree(int, char const**) + 1646 (DumpRenderTree.mm:1268) 27 DumpRenderTree 0x000000010edb7eaf DumpRenderTreeMain(int, char const**) + 111 (DumpRenderTree.mm:1387) 28 DumpRenderTree 0x000000010ee3d012 main + 34 (DumpRenderTreeMain.mm:34) 29 libdyld.dylib 0x00007fff7b4ef015 start + 1

Rolled out in https://trac.webkit.org/r233358 to get WK1 tests (and EWS) running again.

Created attachment 344241 [details] Updated patch to work around compiler issue.

Comment on attachment 344241 [details] Updated patch to work around compiler issue. Attachment 344241 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/8430655 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate apiTests

Comment on attachment 344241 [details] Updated patch to work around compiler issue. Attachment 344241 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8432136 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html

Created attachment 344260 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 344241 [details] Updated patch to work around compiler issue. View in context: https://bugs.webkit.org/attachment.cgi?id=344241&action=review > Source/bmalloc/bmalloc/BCompiler.h:42 > +// Clang versions before 10.00 have a bug where static member variables of an inlined > +// templated class gets implicitely instantiated in each linked object where used. Why not just move s_mallocFallbackState out of the class to a global static variable?

Comment on attachment 344241 [details] Updated patch to work around compiler issue. View in context: https://bugs.webkit.org/attachment.cgi?id=344241&action=review >> Source/bmalloc/bmalloc/BCompiler.h:42 >> +// templated class gets implicitely instantiated in each linked object where used. > > Why not just move s_mallocFallbackState out of the class to a global static variable? I agree with Saam. Also, fwiw, there's a typo: /implicitely/implicitly/.

(In reply to Saam Barati from comment #15) > Comment on attachment 344241 [details] > Updated patch to work around compiler issue. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344241&action=review > > > Source/bmalloc/bmalloc/BCompiler.h:42 > > +// Clang versions before 10.00 have a bug where static member variables of an inlined > > +// templated class gets implicitely instantiated in each linked object where used. > > Why not just move s_mallocFallbackState out of the class to a global static > variable? That is not the variable(s) in question. The issue is with PerThreadStorage in PerThread.h and its three statics: static bool s_didInitialize; static pthread_key_t s_key; static std::once_flag s_onceFlag; There needs to be one (and only one) instance of these variables for each specialized instance of the templated class. There are currently two types used to specialize, PerHeapKind<Cache> and PerHeapKind<Heap>. It seemed easiest to make the change for clang >= v10. This bug exists in the current code with clang < v10, but isn't triggered through normal usage. If you run DumpRenderTree with bmalloc_IsoHeap=false with a clang v9.x Debug build, you'll get the crash.

(In reply to Mark Lam from comment #16) > Comment on attachment 344241 [details] > Updated patch to work around compiler issue. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344241&action=review > > >> Source/bmalloc/bmalloc/BCompiler.h:42 > >> +// templated class gets implicitely instantiated in each linked object where used. > > > > Why not just move s_mallocFallbackState out of the class to a global static variable? > > I agree with Saam. Also, fwiw, there's a typo: /implicitely/implicitly/. Fixed locally.

Created attachment 344365 [details] Updated patch Working with Mark, we were able to come up with template magic to instantiate and export one instance of each specialized PerThread class static and ensure that all modules link to those variables.

Comment on attachment 344365 [details] Updated patch r=me

Attachment 344365 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/PerThread.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 344365 [details] Updated patch r=me too.

Comment on attachment 344365 [details] Updated patch Clearing flags on attachment: 344365 Committed r233547: <https://trac.webkit.org/changeset/233547>

All reviewed patches have been landed. Closing bug.

(In reply to WebKit Commit Bot from comment #23) > Comment on attachment 344365 [details] > Updated patch > > Clearing flags on attachment: 344365 > > Committed r233547: <https://trac.webkit.org/changeset/233547> Build fix for iOS Simulator Debug (and probably other ports): Committed r233550: <https://trac.webkit.org/changeset/233550>

Re-opened since this is blocked by bug 187497

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

This change was rolled out because it introduced flakiness with fullscreen media tests on macOS WK1. See https://bugs.webkit.org/show_bug.cgi?id=187387

Committed r233773: <https://trac.webkit.org/changeset/233773>

I remanded the patch with the additional change that DumpRenderTree by default uses Gigacages. This allows the flaky media tests to run without timing out. I filed <https://bugs.webkit.org/show_bug.cgi?id=187609> to track the issue with DumpRenderTree and these tests that time out.

The commit-queue just saw media/media-fullscreen-loop-inline.html flake (DumpRenderTree crashed) while processing attachment 377039 [details] on bug 201050. Bot: webkit-cq-01 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6

Created attachment 377045 [details] Archive of layout-test-results from webkit-cq-01