Fix race condition with windowless plugin buffers. The problem, which is already fixed for Mac, is …

content/plugin/webplugin_proxy.cc

 // Copyright (c) 2012 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 "content/plugin/webplugin_proxy.h"
 
 #include "build/build_config.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
@@ skipping 365 matching lines @@
     CGContextClearRect(windowless_context(), rect.ToCGRect());
   }
   CGContextClipToRect(windowless_context(), rect.ToCGRect());
   // TODO(caryclark): This is a temporary workaround to allow the Darwin / Skia
   // port to share code with the Darwin / CG port. All ports will eventually use
   // the common code below.
   delegate_->CGPaint(windowless_context(), rect);
   if (windowless_contexts_[saved_index].get() == saved_context_weak)
     CGContextRestoreGState(windowless_contexts_[saved_index]);
 #else
-  windowless_canvas()->save();
+  // See above comment about windowless_context_ changing.
+  // http::/crbug.com139462
+  skia::PlatformCanvas* saved_canvas = windowless_canvas();

[reed1, 2012/08/14 17:19:04]

Do we want to keep this commented line? // SkAutoRef ...

[jam, 2012/08/14 17:29:30]

doh, this is actually the fix. if it didn't repro for Abhishek without it, I'm
really confused! I don't see how the lines below could help anything, since
what's reentrant is the delegate_->Paint() call

[Tom Hudson, 2012/08/14 17:34:55]

It may be the ref isn't necessary.
It used to be we called windowless_canvas() all the time, which would pick up
any changes made by Paint().
Now that you're caching saved_canvas, we've got that data on the stack, instead
of pulling it off of the heap every time we use it. That should be much more
tolerant of reentry.

I suspect this is a standard (mis-)pattern throughout Chrome?

[jam, 2012/08/14 17:40:28]

Paint() is the only function in this scope that can change what
windowless_canvas() returns, and the use-after-free happens inside Paint. So I
don't think the caching of the raw pointer helps here.

[jam, 2012/08/14 20:43:09]

ok, turns out this section was a red herring! If I revert the changes to this
function, it still works without a use-after-free (which makes sense, since the
changes were effectively doing nothing). It turns out the real fix is switching
from scoped_ptr<skia::PlatformCanvas> to SkRefPtr<skia::PlatformCanvas>. I'm not
familiar with skia, so I don't know what other parts are using this canvas, but
it seems like the generic chrome bug of putting a ref counted class in a
scoped_ptr.

+//  SkAutoRef local_ref(saved_canvas);
+
+  saved_canvas->save();
 
   // The given clip rect is relative to the plugin coordinate system.
   SkRect sk_rect = { SkIntToScalar(rect.x()),
                      SkIntToScalar(rect.y()),
                      SkIntToScalar(rect.right()),
                      SkIntToScalar(rect.bottom()) };
-  windowless_canvas()->clipRect(sk_rect);
+  saved_canvas->clipRect(sk_rect);
 
   // Setup the background.
   if (background_canvas_.get() && background_canvas_.get()->getDevice()) {
     // When a background canvas is given, we're in transparent mode. This means
     // the plugin wants to have the image of the page in the canvas it's drawing
     // into (which is windowless_canvases_) so it can do blending. So we copy
     // the background bitmap into the windowless canvas.
     const SkBitmap& background_bitmap =
         skia::GetTopDevice(*background_canvas_)->accessBitmap(false);
-    windowless_canvas()->drawBitmap(background_bitmap, 0, 0);
+    saved_canvas->drawBitmap(background_bitmap, 0, 0);
   } else {
     // In non-transparent mode, the plugin doesn't care what's underneath, so we
     // can just give it black.
     SkPaint black_fill_paint;
     black_fill_paint.setARGB(0xFF, 0x00, 0x00, 0x00);

[reed1, 2012/08/14 17:19:04]

FYI -- canvas->drawColor(0xFF000000); works too

-    windowless_canvas()->drawPaint(black_fill_paint);
+    saved_canvas->drawPaint(black_fill_paint);
   }
 
   // Bring the windowless canvas into the window coordinate system, which is
   // how the plugin expects to draw (since the windowless API was originally
   // designed just for scribbling over the web page).
-  windowless_canvas()->translate(SkIntToScalar(-delegate_->GetRect().x()),
-                                 SkIntToScalar(-delegate_->GetRect().y()));
+  saved_canvas->translate(SkIntToScalar(-delegate_->GetRect().x()),
+                          SkIntToScalar(-delegate_->GetRect().y()));
 
   // Before we send the invalidate, paint so that renderer uses the updated
   // bitmap.
-  delegate_->Paint(windowless_canvas(), offset_rect);
+  delegate_->Paint(saved_canvas, offset_rect);
 
-  windowless_canvas()->restore();
+  saved_canvas->restore();
 #endif
 }
 
 void WebPluginProxy::UpdateGeometry(
     const gfx::Rect& window_rect,
     const gfx::Rect& clip_rect,
     const TransportDIB::Handle& windowless_buffer0,
     const TransportDIB::Handle& windowless_buffer1,
     int windowless_buffer_index,
     const TransportDIB::Handle& background_buffer,
@@ skipping 32 matching lines @@
       !damaged_rect_.IsEmpty()) {
     InvalidateRect(damaged_rect_);
   }
 }
 
 #if defined(OS_WIN)
 
 void WebPluginProxy::CreateCanvasFromHandle(
     const TransportDIB::Handle& dib_handle,
     const gfx::Rect& window_rect,
-    scoped_ptr<skia::PlatformCanvas>* canvas_out) {
-  scoped_ptr<skia::PlatformCanvas> canvas(new skia::PlatformCanvas);
-  if (!canvas->initialize(
+    SkRefPtr<skia::PlatformCanvas>* canvas) {
+  *canvas = new skia::PlatformCanvas;
+  if (!canvas->get()->initialize(
           window_rect.width(),
           window_rect.height(),
           true,
           dib_handle)) {
-    canvas_out->reset();
+    *canvas = NULL;
   }
-  canvas_out->reset(canvas.release());
   // The canvas does not own the section so we need to close it now.
   CloseHandle(dib_handle);
 }
 
 void WebPluginProxy::SetWindowlessBuffers(
     const TransportDIB::Handle& windowless_buffer0,
     const TransportDIB::Handle& windowless_buffer1,
     const TransportDIB::Handle& background_buffer,
     const gfx::Rect& window_rect) {
   CreateCanvasFromHandle(windowless_buffer0,
                          window_rect,
                          &windowless_canvases_[0]);
   if (!windowless_canvases_[0].get()) {
-    windowless_canvases_[1].reset();
-    background_canvas_.reset();
+    windowless_canvases_[1] = NULL;
+    background_canvas_ = NULL;
     return;
   }
   CreateCanvasFromHandle(windowless_buffer1,
                          window_rect,
                          &windowless_canvases_[1]);
   if (!windowless_canvases_[1].get()) {
-    windowless_canvases_[0].reset();
-    background_canvas_.reset();
+    windowless_canvases_[0] = NULL;
+    background_canvas_ = NULL;
     return;
   }
 
   if (background_buffer) {
     CreateCanvasFromHandle(background_buffer,
                            window_rect,
                            &background_canvas_);
     if (!background_canvas_.get()) {
-      windowless_canvases_[0].reset();
-      windowless_canvases_[1].reset();
+      windowless_canvases_[0] = NULL;
+      windowless_canvases_[1] = NULL;
       return;
     }
   }
 }
 
 #elif defined(OS_MACOSX)
 
 void WebPluginProxy::CreateDIBAndCGContextFromHandle(
     const TransportDIB::Handle& dib_handle,
     const gfx::Rect& window_rect,
@@ skipping 37 matching lines @@
                                   &background_dib_,
                                   &background_context_);
 }
 
 #elif defined(USE_X11)
 
 void WebPluginProxy::CreateDIBAndCanvasFromHandle(
     const TransportDIB::Handle& dib_handle,
     const gfx::Rect& window_rect,
     scoped_ptr<TransportDIB>* dib_out,
-    scoped_ptr<skia::PlatformCanvas>* canvas_out) {
+    SkRefPtr<skia::PlatformCanvas>* canvas) {
   TransportDIB* dib = TransportDIB::Map(dib_handle);
-  skia::PlatformCanvas* canvas = NULL;
   // dib may be NULL if the renderer has already destroyed the TransportDIB by
   // the time we receive the handle, e.g. in case of multiple resizes.
   if (dib) {
-    canvas = dib->GetPlatformCanvas(window_rect.width(), window_rect.height());
+    *canvas = dib->GetPlatformCanvas(window_rect.width(), window_rect.height());
+  } else {
+    *canvas = NULL;
   }
   dib_out->reset(dib);
-  canvas_out->reset(canvas);
 }
 
 void WebPluginProxy::CreateShmPixmapFromDIB(
     TransportDIB* dib,
     const gfx::Rect& window_rect,
     XID* pixmap_out) {
   if (dib) {
     Display* display = ui::GetXDisplay();
     XID root_window = ui::GetX11RootWindow();
     XShmSegmentInfo shminfo = {0};
@@ skipping 185 matching lines @@
   // Retrieve the IME status from a plug-in and send it to a renderer process
   // when the plug-in has updated it.
   int input_type;
   gfx::Rect caret_rect;
   if (!delegate_->GetIMEStatus(&input_type, &caret_rect))
     return;
 
   Send(new PluginHostMsg_NotifyIMEStatus(route_id_, input_type, caret_rect));
 }
 #endif