Bug 227229 - [GPU Process] RELEASE_ASSERT in RemoteResourceCacheProxy::didFinalizeRenderingUpdate() may fire if GPUP is relaunched
Summary: [GPU Process] RELEASE_ASSERT in RemoteResourceCacheProxy::didFinalizeRenderin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 227277 (view as bug list)
Depends on: 227292
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-21 11:32 PDT by Said Abou-Hallawa
Modified: 2021-06-24 16:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2021-06-21 13:09 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (7.07 KB, patch)
2021-06-21 20:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (6.95 KB, patch)
2021-06-23 10:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (6.98 KB, patch)
2021-06-23 16:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2021-06-24 13:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-06-21 11:32:40 PDT
The member RemoteResourceCacheProxy::m_numberOfFontsUsedInCurrentRenderingUpdate is used to keep track the number of fonts which are used in the current RenderingUpdate. We keep a HashMap for all fonts in m_fontIdentifierToLastRenderingUpdateVersionMap. This HashMap contains the used fonts, whose count is represented by m_numberOfFontsUsedInCurrentRenderingUpdate, and other unused fonts. The goal of RemoteResourceCacheProxy::didFinalizeRenderingUpdate() is to remove some of the unused fonts if they are really old and if they take more than 25% of m_fontIdentifierToLastRenderingUpdateVersionMap.

There are two bugs in the font caching scenario:
1. When relaunching then GPUP process, RemoteResourceCacheProxy::remoteResourceCacheWasDestroyed() gets called. In this function we clear m_fontIdentifierToLastRenderingUpdateVersionMap but we do not set m_numberOfFontsUsedInCurrentRenderingUpdate to zero. This will make us hit the RELEASE_ASSERT RemoteResourceCacheProxy::didFinalizeRenderingUpdate() once we keep incrementing m_numberOfFontsUsedInCurrentRenderingUpdate.
2. When removing a font from GPUP process by calling RemoteRenderingBackendProxy::releaseRemoteResource(), we do not remove the corresponding entry from m_fontIdentifierToLastRenderingUpdateVersionMap. RemoteResourceCacheProxy::cacheFont() makes the assumption: if the font is found in m_fontIdentifierToLastRenderingUpdateVersionMap, it is also cached in GPUP.
Comment 1 Said Abou-Hallawa 2021-06-21 11:33:27 PDT
<rdar://79147947>
Comment 2 Said Abou-Hallawa 2021-06-21 13:09:11 PDT
Created attachment 431899 [details]
Patch
Comment 3 Myles C. Maxfield 2021-06-21 16:09:52 PDT
Comment on attachment 431899 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +           corresponding entries form m_fontIdentifierToLastRenderingUpdateVersionMap.

typo: "form"

> Source/WebKit/ChangeLog:21
> +        3) We need to prepareForNextRenderingUpdate() even if no font will be
> +           removed from m_fontIdentifierToLastRenderingUpdateVersionMap.

prepareForNextRenderingUpdate() is a new function that's added by this patch, so I don't quite understand why this list in the ChangeLog includes a detail of its calling pattern.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:137
> +    m_currentRenderingUpdateVersion = 0;

RemoteResourceCacheProxy::cacheFont() has:

    auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] {
        return 0;
    });
    auto& lastVersion = result.iterator->value;
    if (lastVersion != m_currentRenderingUpdateVersion) {
        lastVersion = m_currentRenderingUpdateVersion;
        ++m_numberOfFontsUsedInCurrentRenderingUpdate;
    }

If we set m_currentRenderingUpdateVersion to 0 here, then this "if" block will not execute for new fonts that enter cacheFont() after clearFontMap() runs, so we would end up telling the GPUP to cache the new font without incrementing m_numberOfFontsUsedInCurrentRenderingUpdate.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:149
> +    if (!unusedFontCount || unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) {

It looks like you changed the && to ||. Was that intentional?

If || is intentional, I think we can just simplify the entire check to "if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount)"

If not, and it's supposed to be && instead, then I think the check can be just "if (!unusedFontCount)"

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:150
> +        prepareForNextRenderingUpdate();

I don't know if the ChangeLog mentions this, but this seems pretty important. Previously, we weren't incrementing m_currentRenderingUpdateVersion in this branch, which means we were treating adjacent rendering updates as not-distinct if we felt that we didn't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap. Now, with this patch, even if we feel that we don't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap, we're treating adjacent rendering updates as distinct. I think this is important and valuable, and probably worth mentioning in the ChangeLog.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154
> +    Vector<RenderingResourceIdentifier> toBeRemoved;

What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:164
> +    for (auto& renderingResourceIdentifier : toBeRemoved)

Identifiers are just single scalar values, so I think "auto" is better than "auto&"
Comment 4 Said Abou-Hallawa 2021-06-21 20:03:44 PDT
Comment on attachment 431899 [details]
Patch

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

>> Source/WebKit/ChangeLog:18
>> +           corresponding entries form m_fontIdentifierToLastRenderingUpdateVersionMap.
> 
> typo: "form"

Fixed.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:137
>> +    m_currentRenderingUpdateVersion = 0;
> 
> RemoteResourceCacheProxy::cacheFont() has:
> 
>     auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] {
>         return 0;
>     });
>     auto& lastVersion = result.iterator->value;
>     if (lastVersion != m_currentRenderingUpdateVersion) {
>         lastVersion = m_currentRenderingUpdateVersion;
>         ++m_numberOfFontsUsedInCurrentRenderingUpdate;
>     }
> 
> If we set m_currentRenderingUpdateVersion to 0 here, then this "if" block will not execute for new fonts that enter cacheFont() after clearFontMap() runs, so we would end up telling the GPUP to cache the new font without incrementing m_numberOfFontsUsedInCurrentRenderingUpdate.

You are right. This can be fixed by changing the ensure functor to be:

    auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] {
        m_remoteRenderingBackendProxy.cacheFont(makeRef(font));
        ++m_numberOfFontsUsedInCurrentRenderingUpdate;
        return m_currentRenderingUpdateVersion;
    });

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:149
>> +    if (!unusedFontCount || unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) {
> 
> It looks like you changed the && to ||. Was that intentional?
> 
> If || is intentional, I think we can just simplify the entire check to "if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount)"
> 
> If not, and it's supposed to be && instead, then I think the check can be just "if (!unusedFontCount)"

It was intentional because, as you said, the above if-statement is equivalent to "if (!unusedFontCount)". I added "!unusedFontCount" to avoid the multiplication in the clause "minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount". But I will change it as you suggested since you think it will be clearer.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:150
>> +        prepareForNextRenderingUpdate();
> 
> I don't know if the ChangeLog mentions this, but this seems pretty important. Previously, we weren't incrementing m_currentRenderingUpdateVersion in this branch, which means we were treating adjacent rendering updates as not-distinct if we felt that we didn't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap. Now, with this patch, even if we feel that we don't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap, we're treating adjacent rendering updates as distinct. I think this is important and valuable, and probably worth mentioning in the ChangeLog.

A comment will be added to the ChangeLog.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154
>> +    Vector<RenderingResourceIdentifier> toBeRemoved;
> 
> What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes.

I was using it because we loop through the items of m_fontIdentifierToLastRenderingUpdateVersionMap using the modern C++ for-loop which, I think, is not suitable for deleting items while looping. I will replace it by calling HashMap::removeIf() instead.
Comment 5 Said Abou-Hallawa 2021-06-21 20:05:43 PDT
Created attachment 431941 [details]
Patch
Comment 6 Myles C. Maxfield 2021-06-21 20:14:12 PDT
Comment on attachment 431899 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154
>>> +    Vector<RenderingResourceIdentifier> toBeRemoved;
>> 
>> What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes.
> 
> I was using it because we loop through the items of m_fontIdentifierToLastRenderingUpdateVersionMap using the modern C++ for-loop which, I think, is not suitable for deleting items while looping. I will replace it by calling HashMap::removeIf() instead.

Cooooool!
Comment 7 EWS 2021-06-21 23:08:36 PDT
Committed r279104 (239021@main): <https://commits.webkit.org/239021@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431941 [details].
Comment 8 Aakash Jain 2021-06-22 09:31:10 PDT
(In reply to EWS from comment #7)
> Committed r279104 (239021@main): <https://commits.webkit.org/239021@main>
This seems to have made this test very flaky on iOS and mac wk2: fast/canvas/canvas-overloads-strokeText.html

History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fcanvas%2Fcanvas-overloads-strokeText.html
Comment 9 Said Abou-Hallawa 2021-06-22 11:45:29 PDT
It looks like the test is failing because the GPUProcess terminates the WebProcess. I replaced the macro TERMINATE_WEB_PROCESS_WITH_MESSAGE() with Assert(false) and I ran the following command:

run-webkit-tests --debug --no-retry LayoutTests/fast/canvas 

I got the following result:

fast/canvas/canvas-overloads-strokeText.html [ Crash ]

And I got the following call stack when the assertion fired in the GPUProcess:

ASSERTION FAILED: false
/Volumes/Data/WebKit/OpenSource/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp(304) : void WebKit::RemoteRenderingBackend::wakeUpAndApplyDisplayList(const WebKit::GPUProcessWakeupMessageArguments &)
1   0x1438c45b9 WTFCrash
2   0x117a3bf3b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1185aa181 WebKit::RemoteRenderingBackend::wakeUpAndApplyDisplayList(WebKit::GPUProcessWakeupMessageArguments const&)
4   0x1185add11 WebKit::RemoteRenderingBackend::cacheFont(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&)
5   0x11856aee0 void IPC::callMemberFunctionImpl<WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >, 0ul>(WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >&&, std::__1::integer_sequence<unsigned long, 0ul>)
6   0x11856a390 void IPC::callMemberFunction<WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >&&, WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&))
7   0x11855debf void IPC::handleMessage<Messages::RemoteRenderingBackend::CacheFont, WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&)>(IPC::Decoder&, WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&))
8   0x11855d8de WebKit::RemoteRenderingBackend::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
9   0x117abdce2 IPC::Connection::dispatchMessageReceiverMessage(IPC::MessageReceiver&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)
10  0x117ac721a IPC::WorkQueueMessageReceiverQueue::enqueueMessage(IPC::Connection&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)::'lambda'()::operator()()
11  0x117ac6f7e WTF::Detail::CallableWrapper<IPC::WorkQueueMessageReceiverQueue::enqueueMessage(IPC::Connection&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)::'lambda'(), void>::call()
12  0x1438ee6f2 WTF::Function<void ()>::operator()() const
13  0x143a0101e WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::operator()() const
14  0x143a01222 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::operator()(void*) const
15  0x143a011f5 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::__invoke(void*)
16  0x7fff204c0603 _dispatch_call_block_and_release
17  0x7fff204c17e6 _dispatch_client_callout
18  0x7fff204c75ca _dispatch_lane_serial_drain
19  0x7fff204c808d _dispatch_lane_invoke
20  0x7fff204d1bed _dispatch_workloop_worker_thread
21  0x7fff206684c0 _pthread_wqthread
22  0x7fff20667493 start_wqthread
LEAK: 1 WebPageProxy
Comment 11 Said Abou-Hallawa 2021-06-22 21:47:14 PDT
I filed bug 227277 to track in the new flatness in the canvas layout tests.
Comment 13 WebKit Commit Bot 2021-06-23 08:18:53 PDT
Re-opened since this is blocked by bug 227292
Comment 14 Said Abou-Hallawa 2021-06-23 10:35:40 PDT
Created attachment 432069 [details]
Patch
Comment 15 Said Abou-Hallawa 2021-06-23 10:36:21 PDT
*** Bug 227277 has been marked as a duplicate of this bug. ***
Comment 16 Simon Fraser (smfr) 2021-06-23 16:02:50 PDT
Comment on attachment 432069 [details]
Patch

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

> Source/WebKit/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=227229
> +

Radar number here please.
Comment 17 Said Abou-Hallawa 2021-06-23 16:38:26 PDT
Created attachment 432106 [details]
Patch
Comment 18 Said Abou-Hallawa 2021-06-24 13:13:08 PDT
I talked to Myles and we agreed on fixing the RELEASE_ASSERT without removing didFinalizeRenderingUpdate(). Fixing or removing this function will addressed in a separate bug.
Comment 19 Said Abou-Hallawa 2021-06-24 13:37:58 PDT
Created attachment 432207 [details]
Patch
Comment 20 Said Abou-Hallawa 2021-06-24 14:04:31 PDT
I filed bug 227376 to track fixing/removing RemoteResourceCacheProxy::didFinalizeRenderingUpdate().
Comment 21 Myles C. Maxfield 2021-06-24 15:48:00 PDT
Comment on attachment 432207 [details]
Patch

Kind of a shame we don't have an API test for this, that can actually kill the web process to test this codepath...
Comment 22 EWS 2021-06-24 16:12:30 PDT
Committed r279252 (239136@main): <https://commits.webkit.org/239136@main>

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