New issue
Advanced search Search tips

Issue 889498 link

Starred by 2 users
Status: Fixed
Owner:
ericrk@chromium.org
Closed: Oct 12
Cc:
benmason@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 787086



Sign in to add a comment

OOP-D does not handle ContextResult failures

Project Member Reported by boliu@chromium.org, Sep 26 
Found out while implementing https://chromium-review.googlesource.com/1244106

With OOP-D on-screen context is created entirely in the GPU process. The ContextResult for that context is never returned back to the browser process. This is a problem, especially after the above CL, because it's still the browser process that creates the underlying android surface, and it's possible for that surface to be persistently stuck in a bad state.

What currently happens right now is the RootCompositorFrameSinkImpl is deleted in the GPU process, which causes a request for a new frame sink in the browser compositor. There is no retry limit on recreating that frame sink. So a kFatalFailure that crashes the browser process in CompositorImpl::OnGpuChannelEstablished without OOP-D, will probably lead to unbounded retry with OOP-D.

Comment 1 by ericrk@chromium.org, Oct 8 

The following revision *should* have referred to this bug.
  https://chromium.googlesource.com/chromium/src.git/+/7733742ec0041a96de40c887ed1899e7f5dc9216

commit 7733742ec0041a96de40c887ed1899e7f5dc9216
Author: Eric Karl <ericrk@chromium.org>
Date: Fri Oct 05 00:41:07 2018

Android OOP-D: Handle fatal context results

As OOP-D doesn't create the surface-backed context in the Browser
process, it doesn't receive context creation failures. This patch adds
a new function, OnFatalOrSurfaceContextCreationFailure to DisplayClient
which notifies the browser on such errors.

If the error is fatal, we now crash, matching our previous behavior.

Bug:  787086 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I1feeae493291a0c4ab5a4fb966311a1ee334a449
Reviewed-on: https://chromium-review.googlesource.com/c/1249572
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596943}
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/components/viz/test/DEPS
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/components/viz/test/mock_display_client.h
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/content/browser/renderer_host/compositor_impl_android.h
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/gpu/ipc/common/BUILD.gn
[add] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/gpu/ipc/common/context_result.mojom
[add] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/gpu/ipc/common/context_result.typemap
[add] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/gpu/ipc/common/context_result_struct_traits.h
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/gpu/ipc/common/typemaps.gni
[modify] https://crrev.com/7733742ec0041a96de40c887ed1899e7f5dc9216/services/viz/privileged/interfaces/compositing/display_private.mojom

Comment 2 by ericrk@chromium.org, Oct 8

Labels: -Pri-3 Merge-Request-70 Pri-1
Failing to handle these failures in Viz is high priority, as a user will get stuck on a blank screen in chrome with no way to progress other than to restart chrome. Crashing would be much preferred.

Requesting a merge for this based on the above. The code has baked in canary for 4 days and the codepaths modified are exclusive to the Viz Finch experiment, which can be disabled if any issues are discovered.
Project Member

Comment 3 by sheriffbot@chromium.org, Oct 8

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

Comment 4 by benmason@chromium.org, Oct 9

Labels: -Hotlist-Merge-Review -Merge-Review-70 Merge-Approved-70
Approved for merge to 70, branch 3538.
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 12

Cc: benmason@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 6 by ericrk@chromium.org, Oct 12

Labels: -Merge-Approved-70
Status: Fixed (was: Assigned)
Not merging at this point, as we're too close to stable.
►

Sign in to add a comment