Bug 227376 – [GPU Process] Canvas drawing never releases any fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

In this function 'unusedFontCount' is always equal to zero because m_fontIdentifierToLastRenderingUpdateVersionMap.size() is always equal to m_numberOfFontsUsedInCurrentRenderingUpdate. In other words all the cached fonts are always considered be used by the current RenderingUpdate.

So this function makes an early return through the if-statement:

    if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval)
        return;

except in one case which is totalFontCount == 0. In this case, the body of this function will be executed but it will do nothing because m_fontIdentifierToLastRenderingUpdateVersionMap is empty.

This means all the fonts are cached always in GPUP. They are removed only under memory pressure through WebPage::releaseMemory() which calls RemoteResourceCacheProxy::releaseMemory().

[Radar WebKit Bug Importer]

<rdar://problem/79741186>

[Myles C. Maxfield]

This came from https://bugs.webkit.org/show_bug.cgi?id=227229

[Myles C. Maxfield]

Created attachment 432314 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 432314 [details]
Patch

Getting the same crashes that appeared in https://bugs.webkit.org/show_bug.cgi?id=227229

[Myles C. Maxfield]

Can reproduce the crash with:

run-webkit-tests --debug --no-retry --child-processes=1 --verbose fast/canvas/2d.fillText.gradient.html fast/canvas/2d.getPath.modification.html

[Myles C. Maxfield]

I can reproduce a crash (which may be a different crash) with this: DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug ~/Build/Products/Debug/WebKitTestRunner "file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/canvas/2d.fillText.gradient.html" "file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/canvas/2d.getPath.modification.html"

[Myles C. Maxfield]

My theory is that the web process is sending the releaseRemoteResource() message, and then quitting, then a new web process starts up (for the second test, because it has <!-- webkit-test-runner [ InspectorAdditionsEnabled=true ] -->) it creates a new context, and somehow the old message is being delivered to the new context. The new context says "well I don't have any fonts cached with ID 19" and hits the ASSERT_NOT_REACHED().

If this theory is correct, this is real bad.

[Myles C. Maxfield]

I think my theory is confirmed. From the point of view of the GPU Process, we see a RemoteRenderingBackend being created with destinationID 16, then it's destroyed, then we see another RemoteRenderingBackend being created with destinationID 16.

[Myles C. Maxfield]

Created attachment 433007 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433007 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152
> +    if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval) {
> +        prepareForNextRenderingUpdate();

It would be clearer to move all the font releasing code into a new function, so that this early return is less confusing. It would be too easy to add code below that is not font related.

[Myles C. Maxfield]

Created attachment 433083 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 433083 [details]
Patch

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

Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider when m_currentRenderingUpdateVersion is zero?

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152
>      if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval)
>          return;

Do we need to change this if-statement or change maximumUnusedFontCountToSkipRemoval from being zero?

[Myles C. Maxfield]

(In reply to Said Abou-Hallawa from comment #12)
> Comment on attachment 433083 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433083&action=review
> 
> Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment
> about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider
> when m_currentRenderingUpdateVersion is zero?

Nope, because this patch doesn't reset m_currentRenderingUpdateVersion to zero. m_currentRenderingUpdateVersion is a uint64_t so we aren't in danger of it overflowing.

[Myles C. Maxfield]

Comment on attachment 433083 [details]
Patch

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

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152
>>          return;
> 
> Do we need to change this if-statement or change maximumUnusedFontCountToSkipRemoval from being zero?

Good idea. I'll upload a new patch.

[Myles C. Maxfield]

Created attachment 433236 [details]
Patch

[Said Abou-Hallawa]

(In reply to Myles C. Maxfield from comment #13)
> (In reply to Said Abou-Hallawa from comment #12)
> > Comment on attachment 433083 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433083&action=review
> > 
> > Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment
> > about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider
> > when m_currentRenderingUpdateVersion is zero?
> 
> Nope, because this patch doesn't reset m_currentRenderingUpdateVersion to
> zero. m_currentRenderingUpdateVersion is a uint64_t so we aren't in danger
> of it overflowing.

I just realized we initialize m_currentRenderingUpdateVersion with { 1 }. My question was based on the assumption that it is initialized with { 0 }.

[EWS]

Committed r279806 (239570@main): <https://commits.webkit.org/239570@main>

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

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/279806/webkit

Broke two tests:
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/requestClientNodes-css.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=inspector%2Fcanvas%2Frecording-webgl-snapshots.html&test=inspector%2Fcanvas%2FrequestClientNodes-css.html

tracking in https://bugs.webkit.org/show_bug.cgi?id=227929