Commit 2903992a authored by Tom Anderson's avatar Tom Anderson Committed by Commit Bot

Fix crash in X11SoftwareBitmapPresenter teardown

If XGetWindowAttributes() fails in X11SoftwareBitmapPresenter(), |shm_pool_|
will never be constructed. However, we assumed in several places that
|shm_pool_| was non-null, which can cause a crash. For example
~X11SoftwareBitmapPresenter() calls shm_pool_->Teardown() without a check.

This CL adds null checks in the appropriate places.

BUG=1015124
R=rjkroege

Change-Id: I19eabf89961632f5573a4f0020748012aa0d6c8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879502
Auto-Submit: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: default avatarRobert Kroeger <rjkroege@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710072}
parent b92cf6e2
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
#include <stdint.h> #include <stdint.h>
#include <string.h> #include <string.h>
#include <cstring>
#include <memory> #include <memory>
#include <utility> #include <utility>
...@@ -127,6 +128,7 @@ X11SoftwareBitmapPresenter::X11SoftwareBitmapPresenter( ...@@ -127,6 +128,7 @@ X11SoftwareBitmapPresenter::X11SoftwareBitmapPresenter(
: widget_(widget), display_(gfx::GetXDisplay()), gc_(nullptr) { : widget_(widget), display_(gfx::GetXDisplay()), gc_(nullptr) {
DCHECK_NE(widget_, gfx::kNullAcceleratedWidget); DCHECK_NE(widget_, gfx::kNullAcceleratedWidget);
gc_ = XCreateGC(display_, widget_, 0, nullptr); gc_ = XCreateGC(display_, widget_, 0, nullptr);
memset(&attributes_, 0, sizeof(attributes_));
if (!XGetWindowAttributes(display_, widget_, &attributes_)) { if (!XGetWindowAttributes(display_, widget_, &attributes_)) {
LOG(ERROR) << "XGetWindowAttributes failed for window " << widget_; LOG(ERROR) << "XGetWindowAttributes failed for window " << widget_;
return; return;
...@@ -149,13 +151,15 @@ X11SoftwareBitmapPresenter::X11SoftwareBitmapPresenter( ...@@ -149,13 +151,15 @@ X11SoftwareBitmapPresenter::X11SoftwareBitmapPresenter(
} }
X11SoftwareBitmapPresenter::~X11SoftwareBitmapPresenter() { X11SoftwareBitmapPresenter::~X11SoftwareBitmapPresenter() {
XFreeGC(display_, gc_); if (gc_)
XFreeGC(display_, gc_);
shm_pool_->Teardown(); if (shm_pool_)
shm_pool_->Teardown();
} }
bool X11SoftwareBitmapPresenter::ShmPoolReady() const { bool X11SoftwareBitmapPresenter::ShmPoolReady() const {
return shm_pool_->Ready(); return shm_pool_ && shm_pool_->Ready();
} }
bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) { bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
...@@ -164,7 +168,7 @@ bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) { ...@@ -164,7 +168,7 @@ bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
// happens for status icon windows that are typically 16x16px. It's possible // happens for status icon windows that are typically 16x16px. It's possible
// to add a shm codepath, but it wouldn't be buying much since it would only // to add a shm codepath, but it wouldn't be buying much since it would only
// affect windows that are tiny and infrequently updated. // affect windows that are tiny and infrequently updated.
if (!composite_ && shm_pool_->Resize(pixel_size)) { if (!composite_ && shm_pool_ && shm_pool_->Resize(pixel_size)) {
needs_swap_ = false; needs_swap_ = false;
return true; return true;
} }
...@@ -172,7 +176,7 @@ bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) { ...@@ -172,7 +176,7 @@ bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
} }
SkCanvas* X11SoftwareBitmapPresenter::GetSkCanvas() { SkCanvas* X11SoftwareBitmapPresenter::GetSkCanvas() {
if (shm_pool_->Ready()) if (ShmPoolReady())
return shm_pool_->CurrentCanvas(); return shm_pool_->CurrentCanvas();
return nullptr; return nullptr;
} }
...@@ -186,7 +190,7 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface, ...@@ -186,7 +190,7 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
SkPixmap skia_pixmap; SkPixmap skia_pixmap;
if (shm_pool_->Ready()) { if (ShmPoolReady()) {
DCHECK(!sk_surface); DCHECK(!sk_surface);
// TODO(thomasanderson): Investigate direct rendering with DRI3 to avoid any // TODO(thomasanderson): Investigate direct rendering with DRI3 to avoid any
// unnecessary X11 IPC or buffer copying. // unnecessary X11 IPC or buffer copying.
...@@ -279,7 +283,7 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface, ...@@ -279,7 +283,7 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
void X11SoftwareBitmapPresenter::OnSwapBuffers( void X11SoftwareBitmapPresenter::OnSwapBuffers(
SwapBuffersCallback swap_ack_callback) { SwapBuffersCallback swap_ack_callback) {
DCHECK(shm_pool_->Ready()); DCHECK(ShmPoolReady());
if (needs_swap_) if (needs_swap_)
shm_pool_->SwapBuffers(std::move(swap_ack_callback)); shm_pool_->SwapBuffers(std::move(swap_ack_callback));
else else
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment