REGRESSION(r279104): [GPU Process] Canvas layout tests became flaky

• Component: Canvas
• Status: RESOLVED DUPLICATE

[Said Abou-Hallawa]

The change r279104 tried to fix some issues in RemoteResourceCacheProxy::didFinalizeRenderingUpdate().

It fixed the early return 

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

    // which is equivalent to the following code because unusedFontCount and maximumUnusedFontCountToSkipRemoval are both unsigned and maximumUnusedFontCountToSkipRemoval = 0.
    // if (!unusedFontCount)
    //    return;

to:
    if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) {
        prepareForNextRenderingUpdate();
        return;
    }

And it fixed the synchronization of the cached fonts between WebProcess and GPUP:

from:

    // This code removes the font object from GPUP but keeps the font identifier in WebProcess
    for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) {
        if (m_currentRenderingUpdateVersion - item.value >= minimumRenderingUpdateCountToKeepFontAlive)
            m_remoteRenderingBackendProxy.releaseRemoteResource(item.key);
    }

to:

    m_fontIdentifierToLastRenderingUpdateVersionMap.removeIf([&] (const auto& item) {
        if (m_currentRenderingUpdateVersion - item.value < minimumRenderingUpdateCountToKeepFontAlive)
            return false;
        m_remoteRenderingBackendProxy.releaseRemoteResource(item.key);
        return true;
    });


But when I looked closely at the original RemoteResourceCacheProxy::didFinalizeRenderingUpdate() before r279104, I found it was always making an early return and basically it was doing nothing. Here is the explanation.

1. When the fonts are cached for the first RenderingUpdate to a canvas, all the entries in m_fontIdentifierToLastRenderingUpdateVersionMap will have value = 1 since m_currentRenderingUpdateVersion is initialized with 1. 
2. m_numberOfFontsUsedInCurrentRenderingUpdate will be equal to the number of fonts cached during the first RenderingUpdate.
3. RemoteResourceCacheProxy::didFinalizeRenderingUpdate(), will find unusedFontCount == 0 because m_numberOfFontsUsedInCurrentRenderingUpdate == m_fontIdentifierToLastRenderingUpdateVersionMap.size(). So it will return without changing m_numberOfFontsUsedInCurrentRenderingUpdate or m_currentRenderingUpdateVersion.
4. If in the next RenderingUpdate a new font is cached, it will inherit m_currentRenderingUpdateVersion which is still 1. And m_numberOfFontsUsedInCurrentRenderingUpdate will be incremented by 1.
5. Caching more fonts will increment m_numberOfFontsUsedInCurrentRenderingUpdate as if all the cached fonts were used for m_currentRenderingUpdateVersion = 1.
6. RemoteResourceCacheProxy::didFinalizeRenderingUpdate() will make an early return again because unusedFontCount is always zero.

So the change r279104 exercises a new code path which was never exercised before. Removing the font from the cache in GPUP and recaching it causes RemoteRenderingBackend to wakeUpAndApplyDisplayList() more often if the Font was missing in the GPUP cache.

I think the solution is to remove RemoteResourceCacheProxy::didFinalizeRenderingUpdate() since it has been doing nothing since it was written.

[Said Abou-Hallawa]

Created attachment 432017 [details]
Patch

[Said Abou-Hallawa]

Created attachment 432019 [details]
Patch

[Said Abou-Hallawa]

Created attachment 432020 [details]
Patch

[Said Abou-Hallawa]

Created attachment 432021 [details]
Patch

[Said Abou-Hallawa]

Created attachment 432022 [details]
Patch

[Said Abou-Hallawa]

*** This bug has been marked as a duplicate of bug 227229 ***