[GPU Process] RELEASE_ASSERT in RemoteResourceCacheProxy::didFinalizeRenderingUpdate() may fire if GPUP is relaunched

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

<rdar://79147947>

[Said Abou-Hallawa]

Created attachment 431899 [details]
Patch

[Myles C. Maxfield]

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&"

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 431941 [details]
Patch

[Myles C. Maxfield]

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!

[EWS]

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

[Aakash Jain]

(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

[Said Abou-Hallawa]

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

[Truitt Savell]

I am seeing 4 new test failures from the changes in https://trac.webkit.org/changeset/279104/webkit

inspector/canvas/recording-webgl-snapshots.html
svg/as-image/svg-image-with-data-uri-from-canvas.html
fast/canvas/canvas-path-addPath.html
inspector/canvas/requestClientNodes-css.html

results: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r279119%20(3463)/results.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=inspector%2Fcanvas%2Frecording-webgl-snapshots.html&test=svg%2Fas-image%2Fsvg-image-with-data-uri-from-canvas.html&test=fast%2Fcanvas%2Fcanvas-path-addPath.html&test=inspector%2Fcanvas%2FrequestClientNodes-css.html

[Said Abou-Hallawa]

I filed bug 227277 to track in the new flatness in the canvas layout tests.

[Aakash Jain]

These flaky tests are causing false positive on EWS and also slowing down EWS.

e.g.:
False positives on EWS:
https://ews-build.webkit.org/#/builders/51/builds/16145
https://ews-build.webkit.org/#/builders/51/builds/16121
https://ews-build.webkit.org/#/builders/51/builds/16117
https://ews-build.webkit.org/#/builders/51/builds/16111
https://ews-build.webkit.org/#/builders/51/builds/16101
https://ews-build.webkit.org/#/builders/51/builds/16096

Flakiness:
https://ews-build.webkit.org/#/builders/51/builds/16143
https://ews-build.webkit.org/#/builders/51/builds/16132
https://ews-build.webkit.org/#/builders/51/builds/16131
https://ews-build.webkit.org/#/builders/51/builds/16119
https://ews-build.webkit.org/#/builders/51/builds/16116
https://ews-build.webkit.org/#/builders/51/builds/16112
https://ews-build.webkit.org/#/builders/51/builds/16109
https://ews-build.webkit.org/#/builders/51/builds/16103

This wastes so much of engineering time because many engineers would have to look into false positives (because of flaky failures). I am going to revert it for now. You can re-land this along-with patch from Bug 227277.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227292

[Said Abou-Hallawa]

Created attachment 432069 [details]
Patch

[Said Abou-Hallawa]

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

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 432106 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 432207 [details]
Patch

[Said Abou-Hallawa]

I filed bug 227376 to track fixing/removing RemoteResourceCacheProxy::didFinalizeRenderingUpdate().

[Myles C. Maxfield]

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

[EWS]

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