Display Window Icon In Picker UI

chrome/browser/media/window_icon_util_x11.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/window_icon_util.h"
 
 #include <X11/Xatom.h>
 #include <X11/Xutil.h>
 
 #include "ui/gfx/x/x11_types.h"
 
+// The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty

[sky, 2016/08/25 15:44:51]

Are the changes in this file related to this patch?

[qiangchen, 2016/08/25 17:07:59]

Yes. As I tested in real case, sometimes crash. The reason is stated in the
comment.

[sky, 2016/08/25 19:44:17]

Having to replace the error handler and ignore the value seems like a good
indication you are doing something wrong. Especially when the failure case on 38
leaves your error handler installed. From what I can tell none of the other code
in chrome that looks up properties has hacks like this.

Your comment doesn't explain the error you were encountering, so I can't help
diagnose it. I'm adding two reviewers more familiar with X.

[qiangchen, 2016/08/25 20:46:58]

The |id.id| in the call of XGetWindowProperty is actually the window handler. If
the handler is invalid, XGetWindowProperty would throw an exception. One case to
trigger this is that window gets closed before XGetWindowProperty gets called.
Just want to make sure failure to get icon would not block normal workflow. BTW,
without doing this, it will definitely crash the unittest, as the unittest just
makes fake id. Or do you think we just need to set error handler for unittest to
suppress its crash?

[qiangchen, 2016/08/25 21:08:44]

A little more investigation when I try to SetErrorHandler in unittest.
Xlib.h has many name conflicts with our testing frame work "gtest.h", and thus
if I include both .h files in, it cannot compile.

+// may throw an exception, and thus crash Chrome. A possibility is that when
+// GetWindowIcon is called after the window is already closed. Anyway, failure
+// to get an icon is not severe enough to worth blocking the normal workflow or
+// crashing Chrome.
+int ErrorHandler(Display* display, XErrorEvent* event) {
+  return 0;
+}
+
 gfx::ImageSkia GetWindowIcon(content::DesktopMediaID id) {
   DCHECK(id.type == content::DesktopMediaID::TYPE_WINDOW);
 
   Display* display = gfx::GetXDisplay();
   Atom property = XInternAtom(display, "_NET_WM_ICON", True);
   Atom actual_type;
   int actual_format;
   unsigned long bytes_after;  // NOLINT: type required by XGetWindowProperty
   unsigned long size;
   long* data;
 
+  XErrorHandler old_handler = XSetErrorHandler(&ErrorHandler);
   int status = XGetWindowProperty(display, id.id, property, 0L, ~0L, False,
                                   AnyPropertyType, &actual_type, &actual_format,
                                   &size, &bytes_after,
                                   reinterpret_cast<unsigned char**>(&data));
   if (status != Success) {
     return gfx::ImageSkia();
   }
+  XSetErrorHandler(old_handler);
 
   // The format of |data| is concatenation of sections like
   // [width, height, pixel data of size width * height], and the total bytes
   // number of |data| is |size|. And here we are picking the largest icon.
   int width = 0;
   int height = 0;
   int start = 0;
   int i = 0;
   while (i + 1 < static_cast<int>(size)) {
     if ((i == 0 || static_cast<int>(data[i] * data[i + 1]) > width * height) &&
@@ skipping 15 matching lines @@
   for (long y = 0; y < height; ++y) {
     for (long x = 0; x < width; ++x) {
       pixels_data[result.rowBytesAsPixels() * y + x] =
           static_cast<uint32_t>(data[start + width * y + x]);
     }
   }
 
   XFree(data);
   return gfx::ImageSkia::CreateFrom1xBitmap(result);
 }