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

content/plugin/webplugin_proxy.cc

Index: content/plugin/webplugin_proxy.cc
===================================================================
--- content/plugin/webplugin_proxy.cc	(revision 151213)
+++ content/plugin/webplugin_proxy.cc	(working copy)
@@ -383,14 +383,19 @@
   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()) {
@@ -400,26 +405,26 @@
     // 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
 }
 
@@ -472,16 +477,15 @@
 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);
 }
@@ -495,16 +499,16 @@
                          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;
   }
 
@@ -513,8 +517,8 @@
                            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;
     }
   }
@@ -572,16 +576,16 @@
     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(