Display Window Icon In Picker UI

chrome/browser/media/window_icon_util_x11.cc

Index: chrome/browser/media/window_icon_util_x11.cc
diff --git a/chrome/browser/media/window_icon_util_x11.cc b/chrome/browser/media/window_icon_util_x11.cc
index 36462dc229d131d6adbc58c1fadb4b26427e3624..99e20bf5a55ab20d5999c90920cd59fafb3cbb24 100644
--- a/chrome/browser/media/window_icon_util_x11.cc
+++ b/chrome/browser/media/window_icon_util_x11.cc
@@ -9,6 +9,15 @@
 
 #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);
 
@@ -20,6 +29,7 @@ gfx::ImageSkia GetWindowIcon(content::DesktopMediaID id) {
   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,
@@ -27,6 +37,7 @@ gfx::ImageSkia GetWindowIcon(content::DesktopMediaID id) {
   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