New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 909903 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression

Blocking:
issue 906840
issue 909689



Sign in to add a comment

SurfaceSync w/out OOP-D causes blank screen when restarting Chrome on Android

Project Member Reported by ericrk@chromium.org, Nov 28

Issue description

When running SurfaceSync w/out OOP-D, Chrome no longer shows any content when restarting + reloading a prev. URL. This is fixed on refresh or orientation change.

Bisected to "Don't Allocate new LocalsurfaceId at Frame Eviction": https://chromium-review.googlesource.com/c/chromium/src/+/1336438

Steps to repro:
1) Run Android with VizDisplayCompositor disabled + SurfaceSynchronization enabled.
2) Load any page (wikipedia works).
3) Close Chrome by opening app switcher and swiping it away.
4) Re-open Chrome from icon.
5) Screen is blank.

Jonathan, can you take a look and fix or revert the CL for now?

Alternately we could just disable SurfaceSync w/out Viz on Android, as we aren't planning on Launching it alone, but it would still be good to understand what went wrong, in case there are other bugs.

Let me know what approach you'd like to take.
 
So what I see happening, upon relaunch

Without OOP-D
RWHVAndroid::ctor
RWHVAndroid::WasEvicted
RWHVAndroid::DidNavigate

With OOP-D
RWHVAndroid::ctor
RWHVAndroid::Show
RWHVAndroid::DidNavigate


All while marked as visible. So I can detect this, and ensure that DidNavigate generates a new LocalSurfaceId if there is no valid one.

However I'm surprised that we are being evicted at startup. I haven't dug into why yet.
Nevermind that will not be sufficient. Apparently the Evicts can arrive after the Navigation. So this is racy.

Will dig further. samans@ any insight into why non-OOP-D would be evicting at startup?
Blocking: 906840
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 1

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

commit 7ed1160a8a192ae36ee1f5b0c1fee9fd2ca70d15
Author: Jonathan Ross <jonross@chromium.org>
Date: Sat Dec 01 01:32:11 2018

Fix Blank Screens When Restarting on Android

Recently on Android, we moved to invalidating the LocalSurfaceId upon frame
eviction. This way one can be allocated closer to usage.

This has a side effect when Viz Display Compositor is not active. When not
active, the browser itself has different logic for connecting to its
CompositorFrameSink. Where upon any connection, current frames are evicted. If
this occurs while a RenderWidgetHostViewAndroid is showing, then there is no
subsequent actions which update the LocalSurfaceId.

This leads to us evicting the active frame, without generating a new one.

This change updateds RenderWidgetHostViewAndroid::WasEvicted to only invalidate
when not showing. Which addresses the frame eviction of background tabs. While
returning to generating new ids when showing. Which supports the restarting
case.

Bug:  909903 
Change-Id: I5d5da55a8072f71773e2ceb4331fed4fd3402bf0
Reviewed-on: https://chromium-review.googlesource.com/c/1355845
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612894}
[modify] https://crrev.com/7ed1160a8a192ae36ee1f5b0c1fee9fd2ca70d15/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/7ed1160a8a192ae36ee1f5b0c1fee9fd2ca70d15/content/browser/renderer_host/render_widget_host_view_browsertest.cc

Status: Fixed (was: Assigned)
Blocking: 909689
Labels: Merge-Request-72
Status: Assigned (was: Fixed)
A side-effect of this bug has been seen in issue 909689

Requesting merge #4 back into M-72
Labels: ReleaseBlock-Beta
Have confirmed that this is the root cause of issue 909689, and that the fix in #4 does solve it.

Updated with ReleaseBlock-Beta accordingly
Labels: -Merge-Request-72 Merge-Approved-72
Approving merge to M72 branch 3626 based on comments #7 and #9. Please merge ASAP and mark bug as fixed after the merge. Thank you.
Pls merge you change to M72 branch 3626 ASAP so we can pick it up for next  Dev/Beta release, RC cut on Monday, 12/10 @ 1:00 PM PT. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 10

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7dc0b33420a543516f2693d1b748d05e4618ac1

commit e7dc0b33420a543516f2693d1b748d05e4618ac1
Author: jonross <jonross@chromium.org>
Date: Mon Dec 10 13:19:10 2018

Merge Fix Blank Screens When Restarting on Android into M-72
Fix Blank Screens When Restarting on Android

Recently on Android, we moved to invalidating the LocalSurfaceId upon frame
eviction. This way one can be allocated closer to usage.

This has a side effect when Viz Display Compositor is not active. When not
active, the browser itself has different logic for connecting to its
CompositorFrameSink. Where upon any connection, current frames are evicted. If
this occurs while a RenderWidgetHostViewAndroid is showing, then there is no
subsequent actions which update the LocalSurfaceId.

This leads to us evicting the active frame, without generating a new one.

This change updateds RenderWidgetHostViewAndroid::WasEvicted to only invalidate
when not showing. Which addresses the frame eviction of background tabs. While
returning to generating new ids when showing. Which supports the restarting
case.

TBR=jonross@chromium.org

(cherry picked from commit 7ed1160a8a192ae36ee1f5b0c1fee9fd2ca70d15)

Bug:  909903 
Change-Id: I5d5da55a8072f71773e2ceb4331fed4fd3402bf0
Reviewed-on: https://chromium-review.googlesource.com/c/1355845
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612894}
Reviewed-on: https://chromium-review.googlesource.com/c/1369715
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#194}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/e7dc0b33420a543516f2693d1b748d05e4618ac1/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e7dc0b33420a543516f2693d1b748d05e4618ac1/content/browser/renderer_host/render_widget_host_view_browsertest.cc

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/e7dc0b33420a543516f2693d1b748d05e4618ac1

Commit: e7dc0b33420a543516f2693d1b748d05e4618ac1
Author: jonross@chromium.org
Commiter: jonross@chromium.org
Date: 2018-12-10 13:19:10 +0000 UTC

Merge Fix Blank Screens When Restarting on Android into M-72
Fix Blank Screens When Restarting on Android

Recently on Android, we moved to invalidating the LocalSurfaceId upon frame
eviction. This way one can be allocated closer to usage.

This has a side effect when Viz Display Compositor is not active. When not
active, the browser itself has different logic for connecting to its
CompositorFrameSink. Where upon any connection, current frames are evicted. If
this occurs while a RenderWidgetHostViewAndroid is showing, then there is no
subsequent actions which update the LocalSurfaceId.

This leads to us evicting the active frame, without generating a new one.

This change updateds RenderWidgetHostViewAndroid::WasEvicted to only invalidate
when not showing. Which addresses the frame eviction of background tabs. While
returning to generating new ids when showing. Which supports the restarting
case.

TBR=jonross@chromium.org

(cherry picked from commit 7ed1160a8a192ae36ee1f5b0c1fee9fd2ca70d15)

Bug:  909903 
Change-Id: I5d5da55a8072f71773e2ceb4331fed4fd3402bf0
Reviewed-on: https://chromium-review.googlesource.com/c/1355845
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612894}
Reviewed-on: https://chromium-review.googlesource.com/c/1369715
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#194}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment