Bug 236695 – CrashTracer: com.apple.WebKit.WebContent at JavaScriptCore: bmalloc_allocate_impl_impl_slow

• Component: New Bugs
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

Under some circumstances, we run out of memory

Thread 0 Crashed ↩::   Dispatch queue: com.apple.main-thread
0   JavaScriptCore                	       0x18ccfef34 bmalloc_allocate_impl_impl_slow + 60 (/Library/Caches/com.apple.xbs/Sources/bmalloc/WebKit-7613.1.16.0.4/Source/bmalloc/libpas/src/libpas/pas_utils.h:186)
1   JavaScriptCore                	       0x18ccf7f78 bmalloc_allocate_impl_casual_case + 272 (/Library/Caches/com.apple.xbs/Sources/bmalloc/WebKit-7613.1.16.0.4/Source/bmalloc/libpas/src/libpas/pas_try_allocate_intrinsic.h:173)
2   WebCore                       	       0x191a75728 WebCore::combineSegmentsData(WTF::Vector<WebCore::FragmentedSharedBuffer::DataSegmentVectorEntry, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, unsigned long) + 72 (/System/Volumes/Data/SWE/iOS/BuildRoots/BuildRoot949/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS15.4.Internal.sdk/usr/local/include/wtf/FastMalloc.h:219)
3   WebCore                       	       0x191a7566c WebCore::FragmentedSharedBuffer::makeContiguous() const + 280 (/Library/Caches/com.apple.xbs/Sources/WebCore/WebKit-7613.1.16.0.4/Source/WebCore/./platform/SharedBuffer.cpp:120)
4   WebCore                       	       0x1918c3e34 WebCore::CachedRawResource::updateBuffer(WebCore::FragmentedSharedBuffer const&) + 140 (/Library/Caches/com.apple.xbs/Sources/WebCore/WebKit-7613.1.16.0.4/Source/WebCore/./loader/cache/CachedRawResource.cpp:71)
5   WebCore                       	       0x191890b78 WebCore::SubresourceLoader::didReceiveBuffer(WebCore::FragmentedSharedBuffer const&, long long, WebCore::DataPayloadType) + 252 (/Library/Caches/com.apple.xbs/Sources/WebCore/WebKit-7613.1.16.0.4/Source/WebCore/./loader/SubresourceLoader.cpp:545)
6   WebKit                        	       0x18fb7e344 WebKit::WebResourceLoader::didReceiveData(IPC::SharedBufferCopy const&, long long) + 296 (/Library/Caches/com.apple.xbs/Sources/WebKit/WebKit-7613.1.16.0.4/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:221)
7   WebKit                        	       0x18fd1e340 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) + 380 (/Library/Caches/com.apple.xbs/Sources/WebKit/WebKit-7613.1.16.0.4/Source/WebKit/Platform/IPC/HandleMessage.h:125)
8   WebKit                        	       0x18f7856d0 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 788 (/Library/Caches/com.apple.xbs/Sources/WebKit/WebKit-7613.1.16.0.4/Source/WebKit/Platform/IPC/Connection.cpp:1080)
9   WebKit                        	       0x18f787e74 WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_13, void>::call() + 252 (/Library/Caches/com.apple.xbs/Sources/WebKit/WebKit-7613.1.16.0.4/Source/WebKit/Platform/IPC/Connection.cpp:1194)
10  JavaScriptCore                	       0x18cc9bb94 WTF::RunLoop::performWork() + 200 (/Library/Caches/com.apple.xbs/Binaries/WTF/install/Root/usr/local/include/wtf/Function.h:82)
11  JavaScriptCore                	       0x18cc9c930 WTF::RunLoop::performWork(void*) + 36 (/Library/Caches/com.apple.xbs/Sources/WTF/WebKit-7613.1.16.0.4/Source/WTF/wtf/cf/RunLoopCF.cpp:46)

While we can't tell which sites this is occurring with, when using Speedtest.net we get very close to the 1GB memory limit and sometimes ever going over it.

One possible explanation for this high memory usage is that whenever we receive a new partial segment for a XHRRequest, the FragmentedSharedBuffer is flattened by CachedRawResource before being passed to the various listener. Prior bug 233030 this flattening was also occurring (though silently) when SharedBuffer::data() method was called. Bug 233030 changed the signature of the crash.

But not flattening repeatedly the SharedBuffer and only doing it for listener actually needed it we can lower the memory usage (though it may be at the expense of small speed loss as flattening of the SharedBuffer could be done multiple times for the same SharedBuffer).

[Jean-Yves Avenard [:jya]]

rdar://87596724

[Jean-Yves Avenard [:jya]]

Created attachment 452161 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 452169 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 452172 [details]
WIP

[Jer Noble]

Created attachment 452222 [details]
Patch

[Jean-Yves Avenard [:jya]]

Comment on attachment 452222 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        FragmentedSharedBuffer is receieved from the network process, it is proactively coalesced

received

> Source/WebCore/loader/cache/CachedRawResource.cpp:118
> +            if (auto incrementalData = calculateIncrementalDataChunk(contiguousData)) {

this should be using data rather than contiguousData.

Otherwise the client would have received each individual segment, and then a SharedBufferDataView restricting the content to just the last segment received but that actually takes a reference to the entire content. This could potentially means we keep a reference for longer than needed.

> Source/WebCore/loader/cache/CachedScript.cpp:87
>              m_scriptHash = m_script.impl()->hash();

If the CachedScript received fragmented content, finishLoading would have been called. This version has dropped the makeContiguous to m_data in finishLoading, this could potentially lead to multiple call to makeContiguous on the same buffer

[Jer Noble]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 452222 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452222&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        FragmentedSharedBuffer is receieved from the network process, it is proactively coalesced
> 
> received
> 
> > Source/WebCore/loader/cache/CachedRawResource.cpp:118
> > +            if (auto incrementalData = calculateIncrementalDataChunk(contiguousData)) {
> 
> this should be using data rather than contiguousData.
> 
> Otherwise the client would have received each individual segment, and then a
> SharedBufferDataView restricting the content to just the last segment
> received but that actually takes a reference to the entire content. This
> could potentially means we keep a reference for longer than needed.

Okay. Can we fix that in a future patch? This one keeps the original behavior of coalescing and storing in m_data, then passing m_data in.

> > Source/WebCore/loader/cache/CachedScript.cpp:87
> >              m_scriptHash = m_script.impl()->hash();
> 
> If the CachedScript received fragmented content, finishLoading would have
> been called. This version has dropped the makeContiguous to m_data in
> finishLoading, this could potentially lead to multiple call to
> makeContiguous on the same buffer

Not sure I understand here; CachedScript::finishLoading() still coalesces the incoming buffer.

[Jean-Yves Avenard [:jya]]

Comment on attachment 452222 [details]
Patch

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

> Source/WebCore/loader/cache/CachedRawResource.cpp:70
> +    m_data = data.copy();

forgot to mention, if you take a copy rather than a reference to data ; you must keep a strong ref to data above, as a call to notifyClientsDataWasReceived will result in a re-entrant call to the CachedRawResource parent that will destroy data.

[Jean-Yves Avenard [:jya]]

Created attachment 452294 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 452295 [details]
Patch

[EWS]

Committed r290005 (247391@main): <https://commits.webkit.org/247391@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452295 [details].