commit 8af34d702f0ddf935b97070e0b01b31d0e4d1b2c
author Christopher Cameron <ccameron@chromium.org> Fri Jun 22 02:35:29 2018
committer Kenneth Russell <kbr@chromium.org> Fri Jun 22 02:35:29 2018
tree 7832467dac627022767cc5d75a7e92aadc34f991
parent 7f13c14b2e3e74479f2a1f031f174423c4eb594e

Fix flakey SnapshotBrowserTest.SyncMultiWindowTest on Mac

This is a race between navigation and surfaces on their way to the
browser.

Here is the way that things work when the test passes:
 1. Renderer process navigates
    a. Renderer calls RenderFrameImpl::DidCommitNavigationInternal
       - This will go more-or-less directly to the browser process
    b. Renderer calls SubmitCompositorFrame with the current surface id
       - This goes off to the viz process, and eventually will land
         back in the browser
 2. The browser process gets the message from [1.a.]
    a. Comes in as RWHI::DidNavigate
    b. Calls RWHV::DidNavigate
       - This will skip allocating a new surface id, because this is the
         first navigation
    c. STARTS the NewContentRenderTimeout (at the end of
       RHWI::DidNavigate)
 3. The browser process gets notified of the frame sent in [1.b.]
    a. Comes in as DelegatedFrameHost::OnFirstSurfaceActivation
    b. STOPS the NewContentRenderTimeout (in
       RHWI::DidReceiveFirstFrameAfterNavigation).
So we stop the timer, and so we don't evict our frame.

But notice how we're relying on [1.a.] arriving at the browser before
[1.b.], even though they take completely independent routes?

What ends up happening to us when we get a flake is:
 1. Renderer process navigates
    a. Renderer calls RenderFrameImpl::DidCommitNavigationInternal
    b. Renderer calls SubmitCompositorFrame with the current surface id
 2. The browser process gets notified of the frame sent in [1.b.]
    a. Comes in as DelegatedFrameHost::OnFirstSurfaceActivation
    b. In RHWI::DidReceiveFirstFrameAfterNavigation, we don't stop the
       NewContentRenderTimeout, because we never started it.
 3. The browser process gets the message from [1.a.]
    a. Comes in as RWHI::DidNavigate
    b. Calls RWHV::DidNavigate
       - This will skip allocating a new surface id, because this is the
         first navigation
    c. STARTS the new NewContentRenderTimeout (at the end of
       RHWI::DidNavigate)
We'll never hit OnFirstSurfaceActivation for this surface id again
(because we already hit it), and so the timeout will always fire.

The fix is to change RHWI::DidNavigate to only start the timer when
RWHV::DidNavigate changes the surface id. Two reasons:
  - If we didn't change the surface id, it was because this was the
    first navigation, and so we don't need to clear the frame.
  - If we allocate a new surface id, then that will kick the renderer
    to trigger OnFirstSurfaceActivation that will tell us to stop the
    timer (and if we don't allocate a new id, we won't necessarily hit
    OnFirstSurfaceActivation, and we won't stop the timer).

R=samans, kbr
TBR=piman (for content/ OWNERship)

Bug: 853651
Change-Id: I25a8fbf5f6b618b64d764546056fd342e8fc50f9
Reviewed-on: https://chromium-review.googlesource.com/1109448
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569513}