This is interesting, thanks for filing it.

Blink already has some code to try prevent compositor from producing frames with SetDeferCommits() during page transitions. But that only works if you haven't yet done a commit. In this case you already have an active tree, so you produce the wrong thing.

This is also interesting where if you have a compositor animation happening, and a page navigation starts, we'll keep doing the work of that animation and showing frames which gets in the way of the navigation.

This also relates to poor behaviour during navigation where if you stop navigation mid-way, you can end up on the new site, but with the old site visible, and you can still interact with it via scroll because there is an active tree in the renderer, but not with clicks because the main thread is already gone for that site. I encounter this regularly on android.


My first thought was that we could tie this to visibility in the renderer with something like SetVisible(visible, has_url_changed) which would cover your use case here. But I think we should be doing something in other cases where the main thread is gone. In short, while your repro involves being in the background, I think similar things exist in the foreground too, and become persistent when you cancel navigation mid-way.

Doing (3) as stated is problematic because it's not taking into consideration the background color of the page. We don't want to introduce yet another way to flash a color of high contrast to the page background. If nothing else we should be using the background color of the previous page as a placeholder. 

Doing this does require code in the browser process, but we want the aura window to show solid color instead of the previous renderer frame. When the RWHVI becomes visible if the URL has changed, it could evict the previous frame from the renderer (re DelegatedFrameHost::EvictDelegatedFrame()).

But there comes the question of the renderer continuing to produce frames that replace it. It seems there's 2 things that are needed here:

1. Making the renderer stop producing frames when a navigation happens seems good, regardless of visible or not. This would require a method on LayerTreeHost that ends up telling the scheduler to stop making frames until the next BeginMainFrame (and perhaps setting NeedsBeginMainFrame then). I'm not sure where navigation code lives, maybe chrishtr@ has some thoughts re: this.

2. This still races at best and frames can arrive in the browser after the last frame was evicted. We would want to deny these frames. For that we need some communication. I'm not entirely sure how navigation works - it's always controlled by the browser now I think? If so it can pass a monotonically increasing integer to the renderer, and reject frames that don't include that same number back. This is very similar to resize where we pass a frame id to the renderer. +piman@ and +fsamuel@ might have some thoughts here.

If you want to go the route of calling RWHV->Show when the new URL is ready to draw, you can use the visual state callback that was built for android webview. The visual state callback is called on the next activation or on commit aborted. Inside the compositor it becomes a (inaptly named) swap promise that goes through commit / activation / swap. Android webview clients use this to prevent loading a page until it's ready to draw.

https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host.h?rcl=0&l=169

> 1. Making the renderer stop producing frames when a navigation happens seems good, regardless of visible or not. This would require a method on LayerTreeHost that ends up telling the scheduler to stop making frames until the next BeginMainFrame (and perhaps setting NeedsBeginMainFrame then). I'm not sure where navigation code lives, maybe chrishtr@ has some thoughts re: this.

I disagree with this because a lot of times I accidentally click the wrong link and then want to correct my decision by clicking the right one. Doesn't that require a commit?

> I disagree with this because a lot of times I accidentally click the wrong link
> and then want to correct my decision by clicking the right one. Doesn't that
> require a commit?

I'm suggesting this once the main thread has navigated. At that point you can no longer click links because the page is gone. But we end up in a weird inbetween where the active tree still has the old page, but the main thread navigated but stopped loading and never committed a new page.

Maybe we can coax this into the surface id logic, where a navigation would cause somehow a new local id to be created - in the refactored world where local ids are created ahead of time (either in the client or in the browser, but *before* the frame is sent rather than after). For example the FrameMsg_Navigate message could include the new local id, that the renderer would start using once the navigation commits to the new URL.
We'll still need something custom in OP's step (4) to discard the old surface id (and only use the new one from that point on), but I think that would correctly track the state that we want.

At that point, once the browser starts using the new surface id, extra frames for the old one would be essentially discarded (effectively solving both (1) and (2) in danakj's comment #1). I'm not sure whether or not we want a local id change in the LTH to immediately stop generating frames - thinking about it, it may actually be a good idea for resize performance, but I wonder if there are edge cases where it could cause jank. We could leave it as an option - but it's only an optimization at this point.

Thanks for the prompt and detailed feedback on this issue, much appreciated.

sunnyps: That was indeed a very handy function that I had overlooked, I adapted my workaround to use that instead of OnFirstPaintAfterLoad and it appears to work.

When it comes to solving the more generic issue I'm not sure I have the insight into all the details to be able to pull it off in a reasonable time frame, i.e. doing something like what piman suggested.

Are you interested in the solution/workaround I've made so I should create a CL for it or will you attempt a more generic fix? 

I think this remains:

> Doing (3) as stated is problematic because it's not taking into consideration
> the background color of the page. We don't want to introduce yet another way to
> flash a color of high contrast to the page background.

So I don't think we should land that as it changes into a flashing problem instead. It's pretty vacation mode right now but I think if you were to work thru the details with fsamuel an I we could guide you thru making a robust solution. The relevant code is mostly in DelegatedFrameHost (which is the browser side) and RendererCompositorFrameSink (which is the renderer side producer of frames). The part I'm not sure about is where the renderer hears about navigation from the browser that we could stash an ID to send with the next swap, I can connect with people on the loading team if you're interested in poking at this.

It looks to me that DelegatedFrameHost is still generating LocalIds when it receives a frame from the renderer (https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_frame_host.cc?rcl=0&l=464) when the size changes, so we probably want some other monotonic number that we could remove when the local id is coming from the renderer.

In my current version I use the visual state callback so that when RWHVA::Show is called I show only the host and request a visual state callback, and only when the callback is received do I show the aura::Window. That does not cause a flashing background in my setup, since the window is either shown on top of another window or on top of a black background/nothingness. What it does however is introduce an extra delay before the window is shown, I'm currently planning on measuring what the real performance impact of this approach is.

I also realized that I don't only have a problem with loading a new page in a hidden window. The same page may also change while being hidden which also can cause a flash when the window is shown. This is also solved in my current version by not limiting the workaround to new URLs being loaded.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

fsamuel/samans: how do we stand on this wrt Surface Synchronization? We would change the surface id on navigation, right? Do we evict the old frame as well?

I think this should be pretty easy to fix. In RenderWidgetHostImpl::WasShown, add:

if (new_content_rendering_timeout_ && new_content_rendering_timeout_->IsRunning()) {
  new_content_rendering_timeout_->Stop();
  ClearDisplayedGraphics();
}

I'll send out a CL later.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3a2996acf79195ec3b201d9f7a2685b561499e9

commit c3a2996acf79195ec3b201d9f7a2685b561499e9
Author: Saman Sami <samans@chromium.org>
Date: Fri Feb 23 15:42:39 2018

content: Show blank when switching to a tab that navigated in background

If a background tab navigated, don't flash the contents of the old page
when showing it. Instead, show blank until we have content to show.

Bug:  671188 
Change-Id: I774549428d34ba19c133c780b0c76d623e1b7a25
Reviewed-on: https://chromium-review.googlesource.com/929284
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538787}
[modify] https://crrev.com/c3a2996acf79195ec3b201d9f7a2685b561499e9/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/c3a2996acf79195ec3b201d9f7a2685b561499e9/content/browser/renderer_host/render_widget_host_unittest.cc