Bug 187160 – Disable IsoHeaps when Gigacage is off

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

If Gigacage is disabled, it may be due to lack of address space.  Therefore we should also turn off IsoHeaps.

[Michael Saboff]

<rdar://problem/36029149>

[Michael Saboff]

Created attachment 343858 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

(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

[Saam Barati]

(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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Michael Saboff]

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

[Ryan Haddad]

(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

[Ryan Haddad]

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

[Michael Saboff]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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?

[Mark Lam]

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

[Michael Saboff]

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

[Michael Saboff]

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

[Michael Saboff]

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.

[Saam Barati]

Comment on attachment 344365 [details]
Updated patch

r=me

[EWS Watchlist]

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.

[Mark Lam]

Comment on attachment 344365 [details]
Updated patch

r=me too.

[WebKit Commit Bot]

Comment on attachment 344365 [details]
Updated patch

Clearing flags on attachment: 344365

Committed r233547: <https://trac.webkit.org/changeset/233547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187497

[Ryan Haddad]

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

[Ryan Haddad]

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

[Michael Saboff]

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

[Michael Saboff]

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.

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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