New issue
Advanced search Search tips

Issue 824522 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocked on:
issue 819481

Blocking:
issue 833229
issue 861882



Sign in to add a comment

On Linux with EGL, X11 surface is destroyed before EGL surface which uses it

Project Member Reported by kbr@chromium.org, Mar 21 2018

Issue description

In Issue 819481 it was observed that when Chromium uses SwiftShader for its rendering, the X11 window is destroyed before the EGL surface which references it.

This was causing crashes in SwiftShader, though that issue's been fixed.

There may be similar problems when using ANGLE on Chromium, and there might be a similar problem with GLX.

Corentin, as you worked on ANGLE support for Linux, could you look at the window teardown path and see whether the order of destruction can be changed here?

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/fbba4900f68b12171277175980df2e52517cb6b6

commit fbba4900f68b12171277175980df2e52517cb6b6
Author: Nicolas Capens <capn@google.com>
Date: Fri Jul 06 20:55:30 2018

Harden against X11 instability.

Avoid accessing null pointers when an X11 call fails.

Since EGL doesn't own the X11 window, we expect it to be valid
for the duration of the EGL surface. Fail hard if that's not the case.

 Bug chromium:833229 
 Bug chromium:824522 

Change-Id: Iba5e3832fe312fb50232a13e2163a022f5048a76
Reviewed-on: https://swiftshader-review.googlesource.com/19788
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>

[modify] https://crrev.com/fbba4900f68b12171277175980df2e52517cb6b6/src/Main/FrameBufferX11.cpp
[modify] https://crrev.com/fbba4900f68b12171277175980df2e52517cb6b6/src/Main/FrameBufferX11.hpp

Blocking: 833229
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 9

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

commit 179cd58b78d2722d2693464a2be7cf0aab36591c
Author: Nicolas Capens <capn@chromium.org>
Date: Mon Jul 09 13:34:12 2018

Destroy EGL surface before X11 window.

EGL can still be using the window, so we first need to destroy the
surface, then the window.

BUG= 824522 

Change-Id: Ifa4e51cef3f625e9fc6abedfce6336f7bf835e7c
Reviewed-on: https://chromium-review.googlesource.com/1128465
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nicolas Capens <capn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573288}
[modify] https://crrev.com/179cd58b78d2722d2693464a2be7cf0aab36591c/ui/gl/gl_surface_egl_x11.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 9

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

commit 9b9a275b081bf97a3b4cf06bfd2f73fbe7d69020
Author: Nicolas Capens <capn@chromium.org>
Date: Mon Jul 09 13:44:03 2018

Roll SwiftShader 551478a..6a990f8

https://swiftshader.googlesource.com/SwiftShader.git/+log/551478a..6a990f8

BUG= chromium:833229 ,  chromium:824522 ,  chromium:860533 

TBR=kbr@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng;luci.chromium.try:android_optional_gpu_tests_rel

Change-Id: I6ed8a81614a509c703f47be4f8fdb74168381010
Reviewed-on: https://chromium-review.googlesource.com/1128266
Commit-Queue: Nicolas Capens <capn@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573290}
[modify] https://crrev.com/9b9a275b081bf97a3b4cf06bfd2f73fbe7d69020/DEPS

Cc: cwallez@chromium.org
Owner: capn@chromium.org
Status: Started (was: Assigned)
Status: Fixed (was: Started)
Blocking: 861882
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 13

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/8fb6f6a129f5ed809db7ba3a8bc862dd5e7bc75d

commit 8fb6f6a129f5ed809db7ba3a8bc862dd5e7bc75d
Author: Nicolas Capens <capn@google.com>
Date: Fri Jul 13 18:18:05 2018

Remove X11 window validation.

Despite being a 'client resource', the window can become invalid due to
events outside of the client code's control, which causes
XGetWindowAttributes to fail because it retrieves servers-side data
that is no longer available. Hence it is something we should expect to
see happen, and not (always) an indication of a bug that needs fixing.
Also, we should be able to safely continue with an invalid window.

At this point it's up to the client code to catch the X error and
handle it appropriately. The EGL spec does not indicate that it should
catch it instead and generate an error (eglSwapBuffers can generate
EGL_CONTEXT_LOST but that's reserved for power management events).

 Bug chromium:861882 
 Bug chromium:824522 

Change-Id: I78a364516b9466f652c94de68553369935590bde
Reviewed-on: https://swiftshader-review.googlesource.com/19868
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>

[modify] https://crrev.com/8fb6f6a129f5ed809db7ba3a8bc862dd5e7bc75d/src/Main/FrameBufferX11.cpp
[modify] https://crrev.com/8fb6f6a129f5ed809db7ba3a8bc862dd5e7bc75d/src/Main/FrameBufferX11.hpp

Sign in to add a comment