Investigate/work around eglCreateWindowSurface already connected to another API |
|||||||||
Issue descriptionContext crbug.com/785279#c23. Seems like a consistent failure, which is not good. Either increase the retry limit, or maybe try to recreate the entire surface if we can detect this happening..
,
Nov 20 2017
Oh it's pretty bad.. https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20OMIT%20RECORD%20IF%20SUM(REGEXP(special_protos.android_info.logcat_errors%2C%20%27.*already%20connected%20to%20another%20API.*%27))%20%3D%200&sql_dialect=dremelsql&ignore_case=false&enable_rewrite=false&omit_field_name=special_protos.android_info.logcat_errors&omit_field_value=libEGL%20%20%3A%20eglCreateWindowSurface%3A%20native_window_api_connect%20(win%3D0xeb6ad008)%20failed%20(0xffffffea)%20(already%20connected%20to%20another%20API%3F)&omit_field_opt=%3D#magicsignature:50,devicemodel,gpuvendor,gpurenderer,-systemmodified,-country,-magicsignaturesorted And this is real real sad: 1 Pixel 10.89% 10642 2 Nexus 6P 10.07% 9833 3 Pixel XL 8.79% 8588 4 Nexus 5X 5.90% 5768 5 ONEPLUS A5000 3.88% 3792 6 SM-G955U 2.63% 2571 7 SM-G950U 2.01% 1967 8 SM-G935F 1.84% 1798 9 SM-N950U 1.63% 1597 10 ONEPLUS A3003 1.33% 1304
,
Nov 20 2017
,
Nov 20 2017
~60% of content::CompositorImpl::OnGpuChannelEstablished signature contains this log. and since collecting log is a pretty precarious, that's probably a low estimate. Pretty big deal here..
,
Nov 21 2017
Well, killing gpu process or doing a chrome://gpuclean doesn't reproduce. Then looking further, looks like this is something lower is hosed. A lot of these reports broke with 11-20 18:19:30.965 992 1037 I Adreno : DequeueBuffer: dequeueBuffer failed 11-20 18:19:30.979 992 1037 E chromium: [ERROR:gles2_cmd_decoder.cc(15905)] Context lost because SwapBuffers failed. 11-20 18:19:30.980 992 1037 E chromium: [ERROR:gles2_cmd_decoder.cc(5414)] Error: 5 for Command kSwapBuffers So it's something a lot lower that's broken. Gonna add in an artificial swap buffer error tomorrow, and see if the underlying egl surface is actually destroyed. Maybe the problem is trying to create to egl surface from the same buffer queue. If not, maybe the fallback workaround should be to recreate the entire surface instead..
,
Nov 21 2017
Well, I can get the "already connected to another API" failure if I call eglCreateWindowSurface twice in a row. But inserting a failure in SwapBuffers never has problems because the egl surface is always destroyed before the new one is created. +piman, there shouldn't be corner cases where the old egl surface hangs around after a context loss, until after a new egl surface is created with the same window, right? The O-time bug is b/36561071, which is related to picture in picture. I guess that's a feature that can also mess around with our surface as well. But that looks to be fixed, at least I can't reproduce the issue following the same steps
,
Nov 27 2017
@#6: I don't think there are such corner cases, but it's worth checking. The previous ContextProvider should be destroyed before we request the new one, and as a side effect, the previous gpu-side context should be destroyed too. There's refcounting in various places though, so it's worth checking. One thing I'm just thinking of is when we use virtualized contexts (do we here?) we may leave the EGLSurface current as we switch to an offscreen context (to avoid a slow eglMakeCurrent), maybe that impacts something here? I thought we released the EGL context/surface before destroying contexts though.
,
Nov 27 2017
I can try to trace through the code (or add some logging) to understand if there are any corner cases. FWIW, I believe we use virtualized contexts on a lot of Android (all Qualcomm, here: https://cs.chromium.org/chromium/src/gpu/config/gpu_driver_bug_list.json?rcl=95b1a4573777738781dc863fffa6fce8eb55351a&l=261, and scattered other rules)
,
Nov 29 2017
Tracing through the code it really looks like we shouldn't be keeping the surface around while trying to create a new one. When we lose the context, if I understand correctly: 1) We send a disconnect command from the GPU proc to CommandBufferProxyImpl 2) This command triggers us to send a GpuChannelMsg_DestroyCommandBuffer message *before* calling gpu_control_client_->OnGpuControlLostContext 3) Only after calling OnGpuControlLostContext will we eventually pick up the context loss scenario in the LayerTreeFrameSink (through ContextLostObserver) and request a new LayerTreeFrameSink/Context via GpuChannelMsg_CreateCommandBuffer So ordering appears correct here, the CreateCommandBuffer command should always come after the DestroyCommandBuffer command (although there may be other paths I haven't found). In terms of when GLSurface is deleted, it should happen during the DestroyCommandBuffer command. This command forwards through to GpuCommandBufferStub::Destroy, see https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub.cc?rcl=81d9e65fc673569e57e0563043788a876b358a67&l=525. We make the surface current, if possible, then release both the stub as well as the decoder's surface reference. We do this whether or not there are additional references on the decoder. The only way we should have a problem is if someone else has taken a reference on the surface (other than the stub/decoder). It appears that some classes can take surface references in roundabout ways (SurfaceTextureGLOwnerImpl for instance, which grabs a reference to the active surface, see here: https://cs.chromium.org/chromium/src/media/gpu/android/surface_texture_gl_owner.cc?rcl=9a3b9b7bc4aaa92d88acf4ee9efae8b2b4718370&l=62, although I'm not sure if this is hittable for the window surface?) In order to rule out weird refs that we don't expect, I wonder if we can run the following canary experiment: We can CHECK(surface_->HasOneRef()) before removing the last expected ref (here: https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_decoder.cc?rcl=138492abcdb3e0d9369f25868fff4b9dababf915&l=5057) in the window surface case (might require some plumbing). If this is ever false, then something *has* taken a ref and is likely responsible for extending the lifetime / preventing cleanup.
,
Nov 29 2017
Thanks! Maybe we don't need to be as extreme as a release CHECK. Crash reports for clank collect logs (from unsandboxed process only I think, so gpu process is ok), we could just log the state, and see if that log shows up in any of these reports. There should be plenty even in a dev build.
,
Nov 29 2017
Good point. I'll put together a CL for this logging later today.
,
Dec 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a569f96f5126e739d0dae3304c33bba6457a3236 commit a569f96f5126e739d0dae3304c33bba6457a3236 Author: Eric Karl <ericrk@chromium.org> Date: Thu Dec 07 01:31:11 2017 Logging to ensure that non-offscreen surfaces are destroyed as expected We're hitting scenarios where it appears that we may be creating an EGL window surface for a given window before destroying the previous. This leads to errors on Android, and shouldn't be possible. We're suspecting that someone may be taking a reference on the glSurface, extending the lifetime beyond the expected range. This CL adds logging to catch this scenario. Bug: 787086 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I31bb0c55fb393a0719486f32f885273ee81ab9f8 Reviewed-on: https://chromium-review.googlesource.com/802186 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#522293} [modify] https://crrev.com/a569f96f5126e739d0dae3304c33bba6457a3236/gpu/command_buffer/service/gles2_cmd_decoder.cc
,
Dec 7 2017
What should we be doing when this appears in the logs for (passing) unit tests? [9353:9364:1207/132859.462971:3902836794069:ERROR:gles2_cmd_decoder.cc(5058)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time
,
Dec 7 2017
@#13: links to bots / repro steps? This logging was added to investigate an unexpected condition, and it sounds like you ran into it.
,
Dec 7 2017
Which unit test out of curiosity? Is this causing a lot of spam? We could remove it if it's like causing every test to spew out loads of spam. If not, it's ok to just ignore it.
,
Dec 9 2017
I noticed this on Linux. Example with existing tests: out_linux/rel/unit_tests $TESTF=TabTest.\* IMPORTANT DEBUGGING NOTE: batches of tests are run inside their own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with --single-process-tests. Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. Note: Google Test filter = TabTest.HitTestTopPixel:TabTest.LayoutAndVisibilityOfElements:TabTest.TooltipProvidedByTab:TabTest.CloseButtonLayout:TabTest.CloseButtonFocus:TabTest.LayeredThrobber:TabTest.TitleHiddenWhenSmall [==========] Running 7 tests from 1 test case. [----------] Global test environment set-up. [----------] 7 tests from TabTest [ RUN ] TabTest.HitTestTopPixel [73922:73932:1209/144312.779544:4080090110817:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.HitTestTopPixel (168 ms) [ RUN ] TabTest.LayoutAndVisibilityOfElements [73922:73932:1209/144312.828316:4080090159415:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.LayoutAndVisibilityOfElements (48 ms) [ RUN ] TabTest.TooltipProvidedByTab [73922:73932:1209/144312.842938:4080090174033:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.TooltipProvidedByTab (14 ms) [ RUN ] TabTest.CloseButtonLayout [73922:73932:1209/144312.853802:4080090184898:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.CloseButtonLayout (11 ms) [ RUN ] TabTest.CloseButtonFocus [73922:73932:1209/144312.865122:4080090196216:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.CloseButtonFocus (11 ms) [ RUN ] TabTest.LayeredThrobber [73922:73932:1209/144312.876605:4080090207699:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.LayeredThrobber (12 ms) [ RUN ] TabTest.TitleHiddenWhenSmall [73922:73932:1209/144312.892664:4080090223764:ERROR:gles2_cmd_decoder.cc(5088)] crbug.com/787086 : Decoder is not the sole owner of |surface_| at destruction time [ OK ] TabTest.TitleHiddenWhenSmall (17 ms) [----------] 7 tests from TabTest (282 ms total) [----------] Global test environment tear-down [==========] 7 tests from 1 test case ran. (282 ms total) [ PASSED ] 7 tests. [ If it matters, my args.gn: target_os = "linux" is_debug = false is_official_build = false is_chrome_branded = false is_component_build = true enable_nacl = false use_goma = true dcheck_always_on = true remove_webcore_debug_symbols = true symbol_level = 0
,
Dec 11 2017
Thanks! Looked at the test. Turns out all InProcessCommandBuffer would generate that log, because the surface ref is cleared after decoder destroy: https://cs.chromium.org/chromium/src/gpu/ipc/in_process_command_buffer.cc?rcl=17b77d94eb6f27e6cac263969fe9999522f2ade2&l=463 Moving that to before destroy gets rid of the log. This isn't a problem for regular chrome, apparently due to an old archived bug: https://cs.chromium.org/chromium/src/gpu/ipc/service/command_buffer_stub.cc?rcl=17b77d94eb6f27e6cac263969fe9999522f2ade2&l=492
,
Dec 11 2017
,
Dec 11 2017
Reverting the logging CL, as it didn't appear to catch anything in the wild. We had a number of crashes after the CL landed, but none with the log statement: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20product.Version%3D%2765.0.3288.3%27%20OMIT%20RECORD%20IF%20SUM(REGEXP(special_protos.android_info.logcat_errors%2C%20%27.*already%20connected%20to%20another%20API.*%27))%20%3D%200&sql_dialect=dremelsql&ignore_case=false&enable_rewrite=false&omit_field_name=&omit_field_value=&omit_field_opt=#magicsignature:50,+operatingsystem,osversion
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b9fe37fc214c456257f79ff6e9c76d37657100d commit 7b9fe37fc214c456257f79ff6e9c76d37657100d Author: Eric Karl <ericrk@chromium.org> Date: Tue Dec 12 01:43:25 2017 Revert "Logging to ensure that non-offscreen surfaces are destroyed as expected" This reverts commit a569f96f5126e739d0dae3304c33bba6457a3236. Reason for revert: This CL was designed to detect a potential clean-up race. Race not detected, so reverting. Original change's description: > Logging to ensure that non-offscreen surfaces are destroyed as expected > > We're hitting scenarios where it appears that we may be creating an > EGL window surface for a given window before destroying the previous. > This leads to errors on Android, and shouldn't be possible. We're > suspecting that someone may be taking a reference on the glSurface, > extending the lifetime beyond the expected range. This CL adds logging > to catch this scenario. > > Bug: 787086 > Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel > Change-Id: I31bb0c55fb393a0719486f32f885273ee81ab9f8 > Reviewed-on: https://chromium-review.googlesource.com/802186 > Commit-Queue: Eric Karl <ericrk@chromium.org> > Reviewed-by: Bo <boliu@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#522293} TBR=boliu@chromium.org,piman@chromium.org,ericrk@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 787086 Change-Id: Ib921aa04c7a3589b514b4b14091989d368cf2202 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/820111 Reviewed-by: Eric Karl <ericrk@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#523295} [modify] https://crrev.com/7b9fe37fc214c456257f79ff6e9c76d37657100d/gpu/command_buffer/service/gles2_cmd_decoder.cc
,
Feb 9 2018
from internal b/70292592, looks like trying to recreate the whole surface is a valid recovery path here
,
Apr 24 2018
Happened to have found this. Turns out there is already a very similar workaround that recreates the surface on gpu crash, for jellybean: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java?rcl=d4f651d0a3dc71044e7207615b83f8d23e998f18&l=304 Was originally added in the internal repo in https://chrome-internal-review.googlesource.com/c/clank/internal/apps/+/216126 Probably too crude though. It's recreating the surface on every GPU shutdown, but we only want to do it only if it's specifically failing due to this surface issue.
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa commit b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa Author: Bo Liu <boliu@chromium.org> Date: Wed Sep 26 03:04:45 2018 android: Recreate surface on failure There is apparently a bug where android does not release the driver connection to the surface under edge cases such as picture-in-picture. The problem has not been reliably reproduced, but it represents majority of LOG(FATAL) in CompositorImpl::OnGpuChannelEstablished. Luckily, there was already a workaround to recreate the surface implemented for a similar bug in Android JB. This CL distinguishes kSurfaceFailure from ContextResult::kFatalFailure. On non-android platforms, treat kSurfaceFailure exactly the same as kFatalFailure. On Android, re-use the JB workaround to recreate the surface. Note currently OOP-D code path does not return the ContextResult back to CompositorImpl. This CL currently only works when OOP-D is disabled. This is an existing OOP-D bug that should be fixed separately. 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: Ia27967c939d17c73d7b3e2a95f4b12bc465e33a1 Reviewed-on: https://chromium-review.googlesource.com/1244106 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: Changwan Ryu <changwan@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#594202} [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerImpl.java [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/android/java/src/org/chromium/chrome/browser/vr/VrCompositorSurfaceManager.java [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerImplTest.java [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/browser/android/compositor/compositor_view.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/chrome/browser/android/compositor/compositor_view.h [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/browser/compositor/gpu_process_transport_factory.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/browser/compositor/viz_process_transport_factory.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/browser/compositor/viz_process_transport_factory.h [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/public/browser/android/compositor_client.h [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/content/test/layouttest_support.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/gpu/command_buffer/common/BUILD.gn [add] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/gpu/command_buffer/common/context_result.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/gpu/command_buffer/common/context_result.h [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/gpu/ipc/in_process_command_buffer.cc [modify] https://crrev.com/b3a757e3b45e96cef37eda8bf57fbe081b8ee1aa/gpu/ipc/service/gles2_command_buffer_stub.cc
,
Sep 26
,
Oct 5
The following revision refers 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
,
Oct 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f797ab2ecd6154d1cab6a779ef0cc96bdc69db2 commit 8f797ab2ecd6154d1cab6a779ef0cc96bdc69db2 Author: Eric Karl <ericrk@chromium.org> Date: Fri Oct 05 20:53:34 2018 Android OOP-D: Recreate Surface on Failure This is the OOP-D counterpart to: https://chromium-review.googlesource.com/c/chromium/src/+/1244106 A previous patch added a callback to handle context creation failures. This patch now adds the surface error workaround, causing us to recreate our surface on a surface error. 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: I9c283f3b4052afcfe865b4564220d779aca24b88 Reviewed-on: https://chromium-review.googlesource.com/c/1257658 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#597285} [modify] https://crrev.com/8f797ab2ecd6154d1cab6a779ef0cc96bdc69db2/content/browser/renderer_host/compositor_impl_android.cc
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a00c99f33923ed19737bbe4baf525ad9cd6137f9 commit a00c99f33923ed19737bbe4baf525ad9cd6137f9 Author: Eric Karl <ericrk@chromium.org> Date: Tue Oct 09 20:20:52 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. TBR=ericrk@chromium.org (cherry picked from commit 7733742ec0041a96de40c887ed1899e7f5dc9216) 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-Original-Commit-Position: refs/heads/master@{#596943} Reviewed-on: https://chromium-review.googlesource.com/c/1271787 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#928} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/components/viz/test/DEPS [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/components/viz/test/mock_display_client.h [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/content/browser/renderer_host/compositor_impl_android.h [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/gpu/ipc/common/BUILD.gn [add] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/gpu/ipc/common/context_result.mojom [add] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/gpu/ipc/common/context_result.typemap [add] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/gpu/ipc/common/context_result_struct_traits.h [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/gpu/ipc/common/typemaps.gni [modify] https://crrev.com/a00c99f33923ed19737bbe4baf525ad9cd6137f9/services/viz/privileged/interfaces/compositing/display_private.mojom
,
Oct 9
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision a00c99f33923ed19737bbe4baf525ad9cd6137f9 was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a00c99f33923ed19737bbe4baf525ad9cd6137f9 Commit: a00c99f33923ed19737bbe4baf525ad9cd6137f9 Author: ericrk@chromium.org Commiter: ericrk@chromium.org Date: 2018-10-09 20:20:52 +0000 UTC 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. TBR=ericrk@chromium.org (cherry picked from commit 7733742ec0041a96de40c887ed1899e7f5dc9216) 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-Original-Commit-Position: refs/heads/master@{#596943} Reviewed-on: https://chromium-review.googlesource.com/c/1271787 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#928} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7edf2b25409672bd9cfa3ffe1526e499f3aa187 commit f7edf2b25409672bd9cfa3ffe1526e499f3aa187 Author: Alexandr Ilin <alexilin@chromium.org> Date: Wed Oct 10 09:49:59 2018 Revert "Android OOP-D: Handle fatal context results" This reverts commit a00c99f33923ed19737bbe4baf525ad9cd6137f9. Reason for revert: this breaks compilation on M70 branch. See details in crbug.com/893948 Original change's description: > 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. > > TBR=ericrk@chromium.org > > (cherry picked from commit 7733742ec0041a96de40c887ed1899e7f5dc9216) > > 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-Original-Commit-Position: refs/heads/master@{#596943} > Reviewed-on: https://chromium-review.googlesource.com/c/1271787 > Reviewed-by: Eric Karl <ericrk@chromium.org> > Cr-Commit-Position: refs/branch-heads/3538@{#928} > Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} TBR=boliu@chromium.org,tsepez@chromium.org,ericrk@chromium.org Change-Id: I851ccd4af9df6b2daf6b5daff23236143a143a53 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 787086 , 893948 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 Reviewed-on: https://chromium-review.googlesource.com/c/1273296 Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#944} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/components/viz/service/display_embedder/gpu_display_provider.cc [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/components/viz/test/DEPS [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/components/viz/test/mock_display_client.h [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/content/browser/renderer_host/compositor_impl_android.h [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/gpu/ipc/common/BUILD.gn [delete] https://crrev.com/749071a9e7ff731b07bdce0492f9d9c2ff1a4a4c/gpu/ipc/common/context_result.mojom [delete] https://crrev.com/749071a9e7ff731b07bdce0492f9d9c2ff1a4a4c/gpu/ipc/common/context_result.typemap [delete] https://crrev.com/749071a9e7ff731b07bdce0492f9d9c2ff1a4a4c/gpu/ipc/common/context_result_struct_traits.h [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/gpu/ipc/common/typemaps.gni [modify] https://crrev.com/f7edf2b25409672bd9cfa3ffe1526e499f3aa187/services/viz/privileged/interfaces/compositing/display_private.mojom
,
Oct 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7edf2b25409672bd9cfa3ffe1526e499f3aa187 Commit: f7edf2b25409672bd9cfa3ffe1526e499f3aa187 Author: alexilin@chromium.org Commiter: alexilin@chromium.org Date: 2018-10-10 09:49:59 +0000 UTC Revert "Android OOP-D: Handle fatal context results" This reverts commit a00c99f33923ed19737bbe4baf525ad9cd6137f9. Reason for revert: this breaks compilation on M70 branch. See details in crbug.com/893948 Original change's description: > 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. > > TBR=ericrk@chromium.org > > (cherry picked from commit 7733742ec0041a96de40c887ed1899e7f5dc9216) > > 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-Original-Commit-Position: refs/heads/master@{#596943} > Reviewed-on: https://chromium-review.googlesource.com/c/1271787 > Reviewed-by: Eric Karl <ericrk@chromium.org> > Cr-Commit-Position: refs/branch-heads/3538@{#928} > Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} TBR=boliu@chromium.org,tsepez@chromium.org,ericrk@chromium.org Change-Id: I851ccd4af9df6b2daf6b5daff23236143a143a53 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 787086 , 893948 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 Reviewed-on: https://chromium-review.googlesource.com/c/1273296 Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#944} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 12
On dev, OnGpuChannelEstablished signature is gone and OnFatalOrSurfaceContextCreationFailure. I think we can claim victory here. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by boliu@chromium.org
, Nov 20 2017