Fix Linux capturer crash on resize

Problem was a race-condition triggered by two resize
operations in quick succession:

* Capturer receives ConfigureNotify with a large screen size.
It stores the large size, and calls XServerPixelBuffer::Init()
* Meanwhile, the root window is resized to a smaller size.
* XServerPixelBuffer::Init() directly queries the current size,
and creates buffers with the new smaller size.
* Capturer requests screen capture, using the stored large size.
It crashes when trying to copy too much data from the
PixelBuffer.
* If it didn't crash, the Capturer would then get the
ConfigureNotify event for the smaller size.

This CL fixes the XServerPixelBuffer to use the sizes from the
ConfigureNotify events. As an added precaution, this also clips
the XDamage regions to the current size of the PixelBuffer,
before capturing screen pixels.


BUG=166713
TEST="while true; do xrandr -s 1; xrandr -s 2; done"


Review URL: https://chromiumcodereview.appspot.com/11639025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174112 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 46 insertions, 23 deletions

diff --git a/remoting/capturer/linux/x_server_pixel_buffer.cc b/remoting/capturer/linux/x_server_pixel_buffer.cc
index 00615df..c495cbc 100644
--- a/remoting/capturer/linux/x_server_pixel_buffer.cc
+++ b/remoting/capturer/linux/x_server_pixel_buffer.cc
@@ -59,7 +59,8 @@ int GetLastXServerError() {
 namespace remoting {
 
 XServerPixelBuffer::XServerPixelBuffer()
-    : display_(NULL), root_window_(0), x_image_(NULL),
+    : display_(NULL), root_window_(0),
+      root_window_size_(SkISize::Make(0, 0)), x_image_(NULL),
       shm_segment_info_(NULL), shm_pixmap_(0), shm_gc_(NULL) {
 }
 
@@ -90,18 +91,24 @@ void XServerPixelBuffer::Release() {
   }
 }
 
-void XServerPixelBuffer::Init(Display* display) {
+void XServerPixelBuffer::Init(Display* display,
+                              const SkISize& screen_size) {
   Release();
   display_ = display;
+  root_window_size_ = screen_size;
   int default_screen = DefaultScreen(display_);
   root_window_ = RootWindow(display_, default_screen);
   InitShm(default_screen);
 }
 
-void XServerPixelBuffer::InitShm(int screen) {
+// static
+SkISize XServerPixelBuffer::GetRootWindowSize(Display* display) {
   XWindowAttributes root_attr;
-  XGetWindowAttributes(display_, root_window_, &root_attr);
-  int width = root_attr.width, height = root_attr.height;
+  XGetWindowAttributes(display, DefaultRootWindow(display), &root_attr);
+  return SkISize::Make(root_attr.width, root_attr.height);
+}
+
+void XServerPixelBuffer::InitShm(int screen) {
   Visual* default_visual = DefaultVisual(display_, screen);
   int default_depth = DefaultDepth(display_, screen);
 
@@ -117,7 +124,8 @@ void XServerPixelBuffer::InitShm(int screen) {
   shm_segment_info_->shmaddr = reinterpret_cast<char*>(-1);
   shm_segment_info_->readOnly = False;
   x_image_ = XShmCreateImage(display_, default_visual, default_depth, ZPixmap,
-                             0, shm_segment_info_, width, height);
+                             0, shm_segment_info_, root_window_size_.width(),
+                             root_window_size_.height());
   if (x_image_) {
     shm_segment_info_->shmid = shmget(
         IPC_PRIVATE, x_image_->bytes_per_line * x_image_->height,
@@ -149,7 +157,7 @@ void XServerPixelBuffer::InitShm(int screen) {
   }
 
   if (havePixmaps)
-    havePixmaps = InitPixmaps(width, height, default_depth);
+    havePixmaps = InitPixmaps(default_depth);
 
   shmctl(shm_segment_info_->shmid, IPC_RMID, 0);
   shm_segment_info_->shmid = -1;
@@ -158,7 +166,7 @@ void XServerPixelBuffer::InitShm(int screen) {
           << " with" << (havePixmaps?"":"out") << " pixmaps.";
 }
 
-bool XServerPixelBuffer::InitPixmaps(int width, int height, int depth) {
+bool XServerPixelBuffer::InitPixmaps(int depth) {
   if (XShmPixmapFormat(display_) != ZPixmap)
     return false;
 
@@ -166,7 +174,8 @@ bool XServerPixelBuffer::InitPixmaps(int width, int height, int depth) {
   shm_pixmap_ = XShmCreatePixmap(display_, root_window_,
                                  shm_segment_info_->shmaddr,
                                  shm_segment_info_,
-                                 width, height, depth);
+                                 root_window_size_.width(),
+                                 root_window_size_.height(), depth);
   XSync(display_, False);
   if (GetLastXServerError() != 0) {
     // |shm_pixmap_| is not not valid because the request was not processed
@@ -203,6 +212,7 @@ void XServerPixelBuffer::Synchronize() {
 }
 
 uint8* XServerPixelBuffer::CaptureRect(const SkIRect& rect) {
+  DCHECK(SkIRect::MakeSize(root_window_size_).contains(rect));
   if (shm_segment_info_) {
     if (shm_pixmap_) {
       XCopyArea(display_, root_window_, shm_pixmap_, shm_gc_,
diff --git a/remoting/capturer/linux/x_server_pixel_buffer.h b/remoting/capturer/linux/x_server_pixel_buffer.h
index 39b239a8f..afd96b0 100644
--- a/remoting/capturer/linux/x_server_pixel_buffer.h
+++ b/remoting/capturer/linux/x_server_pixel_buffer.h
@@ -23,7 +23,15 @@ class XServerPixelBuffer {
   ~XServerPixelBuffer();
 
   void Release();
-  void Init(Display* display);
+
+  // Allocate (or reallocate) the pixel buffer with the given size, which is
+  // assumed to be the current size of the root window.
+  // |screen_size| should either come from GetRootWindowSize(), or
+  // from a recent ConfigureNotify event on the root window.
+  void Init(Display* display, const SkISize& screen_size);
+
+  // Request the current size of the root window from the X Server.
+  static SkISize GetRootWindowSize(Display* display);
 
   // If shared memory is being used without pixmaps, synchronize this pixel
   // buffer with the root window contents (otherwise, this is a no-op).
@@ -37,6 +45,8 @@ class XServerPixelBuffer {
   // call to CaptureRect.
   // In the case where the full-screen data is captured by Synchronize(), this
   // simply returns the pointer without doing any more work.
+  // The caller must ensure that |rect| is no larger than the screen size
+  // supplied to Init().
   uint8* CaptureRect(const SkIRect& rect);
 
   // Return information about the most recent capture. This is only guaranteed
@@ -54,10 +64,11 @@ class XServerPixelBuffer {
 
  private:
   void InitShm(int screen);
-  bool InitPixmaps(int width, int height, int depth);
+  bool InitPixmaps(int depth);
 
   Display* display_;
   Window root_window_;
+  SkISize root_window_size_;
   XImage* x_image_;
   XShmSegmentInfo* shm_segment_info_;
   Pixmap shm_pixmap_;
diff --git a/remoting/capturer/video_frame_capturer_linux.cc b/remoting/capturer/video_frame_capturer_linux.cc
index b662c38..3b17687 100644
--- a/remoting/capturer/video_frame_capturer_linux.cc
+++ b/remoting/capturer/video_frame_capturer_linux.cc
@@ -90,7 +90,7 @@ class VideoFrameCapturerLinux : public VideoFrameCapturer {
   scoped_refptr<CaptureData> CaptureScreen();
 
   // Called when the screen configuration is changed. |root_window_size|
-  // specifies size the most recent size of the root window.
+  // specifies the most recent size of the root window.
   void ScreenConfigurationChanged(const SkISize& root_window_size);
 
   // Synchronize the current buffer with |last_buffer_|, by copying pixels from
@@ -199,8 +199,6 @@ bool VideoFrameCapturerLinux::Init() {
     return false;
   }
 
-  x_server_pixel_buffer_.Init(display_);
-
   root_window_ = RootWindow(display_, DefaultScreen(display_));
   if (root_window_ == BadValue) {
     LOG(ERROR) << "Unable to get the root window";
@@ -224,17 +222,11 @@ bool VideoFrameCapturerLinux::Init() {
     LOG(INFO) << "X server does not support XFixes.";
   }
 
-  if (ShouldUseXDamage()) {
-    InitXDamage();
-  }
-
   // Register for changes to the dimensions of the root window.
   XSelectInput(display_, root_window_, StructureNotifyMask);
 
-  // Update the root window size.
-  XWindowAttributes root_attributes;
-  XGetWindowAttributes(display_, root_window_, &root_attributes);
-  root_window_size_.set(root_attributes.width, root_attributes.height);
+  root_window_size_ = XServerPixelBuffer::GetRootWindowSize(display_);
+  x_server_pixel_buffer_.Init(display_, root_window_size_);
 
   if (has_xfixes_) {
     // Register for changes to the cursor shape.
@@ -242,6 +234,10 @@ bool VideoFrameCapturerLinux::Init() {
                             XFixesDisplayCursorNotifyMask);
   }
 
+  if (ShouldUseXDamage()) {
+    InitXDamage();
+  }
+
   return true;
 }
 
@@ -435,6 +431,12 @@ scoped_refptr<CaptureData> VideoFrameCapturerLinux::CaptureScreen() {
 
     // Capture the damaged portions of the desktop.
     helper_.SwapInvalidRegion(&invalid_region);
+
+    // Clip the damaged portions to the current screen size, just in case some
+    // spurious XDamage notifications were received for a previous (larger)
+    // screen size.
+    invalid_region.op(SkIRect::MakeSize(root_window_size_),
+                      SkRegion::kIntersect_Op);
     for (SkRegion::Iterator it(invalid_region); !it.done(); it.next()) {
       CaptureRect(it.rect(), capture_data);
     }
@@ -472,7 +474,7 @@ void VideoFrameCapturerLinux::ScreenConfigurationChanged(
   queue_.SetAllFramesNeedUpdate();
 
   helper_.ClearInvalidRegion();
-  x_server_pixel_buffer_.Init(display_);
+  x_server_pixel_buffer_.Init(display_, root_window_size_);
 }
 
 void VideoFrameCapturerLinux::SynchronizeFrame() {