gpu: Fix potential CurrentGL dangling pointer [chromium/src : main]
Sunny Sachanandani (Gerrit)
Attention is currently required from: Zhenyao Mo.
Sunny Sachanandani would like Zhenyao Mo to review this change.
gpu: Fix potential CurrentGL dangling pointer
There's a potential issue with GLContextEGL::ReleaseBackpressureFences
leaving the thread local CurrentGL pointer set if there was no previous
current context. This means that the pointer will be dangling when the
context is destroyed. This ordinarily doesn't happen with Ganesh/GL
since there's almost always a current GL context, but it's possible with
Graphite when SharedContext won't ever make its context current.
This CL also cleans up the code related to setting the thread local
CurrentGL pointer:
- GetGlContextForCurrentThread returns a reference to the thread local
CurrentGL pointer so it's renamed to ThreadLocalCurrentGL and is
not exposed via gl_bindings.h (see below).
- GetThreadLocalCurrentGL returns the CurrentGL pointer and is exported
in gl_bindings.h. It does not return a mutable reference though.
- SetCurrentGL sets the thread local CurrentGL pointer so it's renamed
to SetThreadLocalCurrentGL to distinguish it from GetCurrentGL which
is an accessor on the GLContext class.
Bug: 1478112
Change-Id: Idd489ba129b2a59f8fcf5dd4edf379c3e9fe05aa
---
M ui/gl/gl_bindings.h
M ui/gl/gl_context.cc
M ui/gl/gl_context_egl.cc
M ui/gl/gl_gl_api_implementation.cc
M ui/gl/gl_gl_api_implementation.h
M ui/gl/gl_implementation.cc
6 files changed, 36 insertions(+), 25 deletions(-)
To view, visit change 4846817. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Zhenyao Mo.
To view, visit change 4846817. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Zhenyao Mo.
Patchset:
PTAL
File ui/gl/gl_context_egl.cc:
if (prev_current_context != this) {
SetThreadLocalCurrentGL(prev_current_context
? prev_current_context->GetCurrentGL()
: nullptr);
}This is the main fix in this CL BTW.
To view, visit change 4846817. To unsubscribe, or for help writing mail filters, visit settings.
Zhenyao Mo (Gerrit)
Attention is currently required from: Sunny Sachanandani.
Patch set 5:Code-Review +1
1 comment:
Patchset:
LGTM
Thanks for clean up and fixing the dangling pointer!
To view, visit change 4846817. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Patch set 5:Auto-Submit +1Commit-Queue +2
2 comments:
Patchset:
Thanks!
File ui/gl/gl_context_egl.cc:
if (prev_current_context != this) {
SetThreadLocalCurrentGL(prev_current_context
? prev_current_context->GetCurrentGL()
: nullptr);
}This is the main fix in this CL BTW.
Acknowledged
To view, visit change 4846817. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Sunny Sachanandani.
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
gpu: Fix potential CurrentGL dangling pointer
There's a potential issue with GLContextEGL::ReleaseBackpressureFences
leaving the thread local CurrentGL pointer set if there was no previous
current context. This means that the pointer will be dangling when the
context is destroyed. This ordinarily doesn't happen with Ganesh/GL
since there's almost always a current GL context, but it's possible with
Graphite when SharedContext won't ever make its context current.
This CL also cleans up the code related to setting the thread local
CurrentGL pointer:
- GetGlContextForCurrentThread returns a reference to the thread local
CurrentGL pointer so it's renamed to ThreadLocalCurrentGL and is
not exposed via gl_bindings.h (see below).
- GetThreadLocalCurrentGL returns the CurrentGL pointer and is exported
in gl_bindings.h. It does not return a mutable reference though.
- SetCurrentGL sets the thread local CurrentGL pointer so it's renamed
to SetThreadLocalCurrentGL to distinguish it from GetCurrentGL which
is an accessor on the GLContext class.
Bug: 1478112
Change-Id: Idd489ba129b2a59f8fcf5dd4edf379c3e9fe05aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4846817
Reviewed-by: Zhenyao Mo <z...@chromium.org>
Commit-Queue: Sunny Sachanandani <sun...@chromium.org>
Auto-Submit: Sunny Sachanandani <sun...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1194415}
---
M gpu/command_buffer/service/external_semaphore.cc
M ui/gl/gl_bindings.h
M ui/gl/gl_context.cc
M ui/gl/gl_context_egl.cc
M ui/gl/gl_gl_api_implementation.cc
M ui/gl/gl_gl_api_implementation.h
M ui/gl/gl_implementation.cc
7 files changed, 39 insertions(+), 28 deletions(-)