Merge lp:~uriboni/unity-2d/spread-safer-window-image-provider into lp:unity-2d/3.0

[Ugo Riboni]

[spread] Implement a safer logic to convert the captured window pixmap into images that is resistant to the window being unmapped while we convert.

This MR makes sure that if the conversion of the captured window pixmap to fails due to the window being unmapped while we're converting it, we don't crash (and we retry to grab the pixmap following the alternative code path for unmapped windows).

The reason for the crash is explained in large detail in the attached bug.

To test this patch I suggest you do *not* apply the fix in lp:~uriboni/unity-2d/spread-no-windows-when-inactive , then follow the instructions in the bug report and verify that no matter how many times you try you don't get the crash.
A more scientific approach (given the fact that the bug is not reproducible reliably, but only randomly after usually two or three tries) would be to go in with a debugger and verify that we're actually at least once failing to convert the image and recover safely from that.

[Florian Boucault (fboucault)]

Functionally fine, fixes the crasher and does not introduce regressions.

review: Approve (functional)

=== modified file 'libunity-2d-private/Unity2d/windowimageprovider.cpp'
--- libunity-2d-private/Unity2d/windowimageprovider.cpp	2011-02-04 18:00:55 +0000
+++ libunity-2d-private/Unity2d/windowimageprovider.cpp	2011-04-15 11:52:29 +0000
@@ -109,16 +109,43 @@
     /* After doing this, split the rest of the id on the character "|". The first
        part is the window ID of the decorations, the latter of the actual content. */
     atPos = windowIds.indexOf('|');
-    QString frameId = (atPos == -1) ? windowIds : windowIds.left(atPos);
-    QString contentId = (atPos == -1) ? windowIds : windowIds.mid(atPos + 1);
-
-    QPixmap shot;
+    Window frameId = ((atPos == -1) ? windowIds : windowIds.left(atPos)).toULong();
+    Window contentId = ((atPos == -1) ? windowIds : windowIds.mid(atPos + 1)).toULong();
+
+    QImage image;
+    QPixmap pixmap = getWindowPixmap(frameId, contentId);
+    if (!pixmap.isNull()) {
+        image = convertWindowPixmap(pixmap, frameId);
+        if (image.isNull()) {
+            /* This means that the window got unmapped while we were converting it to
+               an image. Try again so that we will pick up the pixmap captured by
+               metacity instead. */
+            pixmap = getWindowPixmap(frameId, contentId);
+            if (!pixmap.isNull()) {
+                image = convertWindowPixmap(pixmap, frameId);
+            }
+        }
+    }
+
+    if (!image.isNull()) {
+        if (requestedSize.isValid()) {
+            image = image.scaled(requestedSize);
+        }
+        size->setWidth(image.width());
+        size->setHeight(image.height());
+    }
+
+    return image;
+}
+
+QPixmap WindowImageProvider::getWindowPixmap(Window frameWindowId,
+                                             Window contentWindowId)
+{
     XWindowAttributes attr;
-    Window frame = (Window) frameId.toULong();
 
-    XGetWindowAttributes(QX11Info::display(), frame, &attr);
+    XGetWindowAttributes(QX11Info::display(), frameWindowId, &attr);
     if (attr.map_state == IsViewable) {
-        shot = QPixmap::fromX11Pixmap(frame);
+        return QPixmap::fromX11Pixmap(frameWindowId);
     } else {
         /* If the window is not viewable, grabbing the pixmap directly will fail.
            Therefore we try to retrieve the captured pixmap from the special metacity
@@ -127,20 +154,25 @@
            window including decorations.
         */
         XID pixmap;
-        Window content = (Window) contentId.toULong();
-        if (!tryGetWindowCapture(content, &pixmap)) {
-            return QImage();
-        }
-        else {
-            shot = QPixmap::fromX11Pixmap(pixmap);
-        }
-    }
-
-    if (shot.isNull()) {
-        return QImage();
-    }
-
-    QImage result;
+        if (!tryGetWindowCapture(contentWindowId, &pixmap)) {
+            return QPixmap();
+        } else {
+            return QPixmap::fromX11Pixmap(pixmap);
+        }
+    }
+}
+
+/* All the exception checks in this method are due to the fact that
+   the windowPixmap is an X11 Drawable tied to a window. When we called
+   this function the drawable was valid since the window was mapped, however
+   there's no guarantee it will stay that way.
+   All the operations we do here involve at some point converting the pixmap
+   into a QImage (since we use the rester paint engine). This conversion will
+   throw std::bad_alloc if the drawable is not valid, so we need to catch it.
+*/
+QImage WindowImageProvider::convertWindowPixmap(QPixmap windowPixmap,
+                                                Window frameWindowId)
+{
     if (m_x11supportsShape) {
         /* The borders of the window may be irregularly shaped.
            To capture this correctly we need to ask the Shape extension
@@ -150,34 +182,38 @@
         XRectangle *rectangles;
         int rectangle_count, rectangle_order;
         rectangles = XShapeGetRectangles (QX11Info::display(),
-                                          frame,
+                                          frameWindowId,
                                           ShapeBounding,
                                           &rectangle_count,
                                           &rectangle_order);
 
-        result = QImage(shot.size(), QImage::Format_ARGB32_Premultiplied);
+        QImage result(windowPixmap.size(), QImage::Format_ARGB32_Premultiplied);
         result.fill(Qt::transparent);
         QPainter painter(&result);
 
         for (int i = 0; i < rectangle_count; i++) {
             XRectangle r = rectangles[i];
-            painter.drawPixmap(QPointF(r.x, r.y), shot,
-                               QRectF(r.x, r.y, r.width, r.height));
+            try {
+                /* Since we're using the raster paint engine, this will internally
+                   call QX11PixmapData::toImage on windowPixmap to be able to
+                   draw it on the QImage */
+                painter.drawPixmap(QPointF(r.x, r.y), windowPixmap,
+                                   QRectF(r.x, r.y, r.width, r.height));
+            } catch (std::bad_alloc) {
+                return QImage();
+            }
         }
 
         painter.end();
         XFree(rectangles);
+        return result;
     } else {
-        result = shot.toImage();
-    }
-
-    if (requestedSize.isValid()) {
-        result = result.scaled(requestedSize);
-    }
-    size->setWidth(result.width());
-    size->setHeight(result.height());
-
-    return result;
+        try {
+            return windowPixmap.toImage();
+        } catch (std::bad_alloc) {
+            return QImage();
+        }
+    }
 }
 
 /*! Tries to ask the X Composite extension (if supported) to redirect all

=== modified file 'libunity-2d-private/Unity2d/windowimageprovider.h'
--- libunity-2d-private/Unity2d/windowimageprovider.h	2011-01-27 09:30:14 +0000
+++ libunity-2d-private/Unity2d/windowimageprovider.h	2011-04-15 11:52:29 +0000
@@ -21,6 +21,8 @@
 #include <QImage>
 #include <QSize>
 
+typedef unsigned long Window;
+
 class WindowImageProvider : public QDeclarativeImageProvider
 {
 public:
@@ -30,6 +32,9 @@
     static void activateComposite();
 
 private:
+    QPixmap getWindowPixmap(Window frameWindowId, Window contentWindowId);
+    QImage convertWindowPixmap(QPixmap windowPixmap, Window frameWindowId);
+
     bool m_x11supportsShape;
 };