don't rely on save/restore for the selected bitmap

skia/ext/raster_handle_allocator_win.cc

Index: skia/ext/raster_handle_allocator_win.cc
diff --git a/skia/ext/raster_handle_allocator_win.cc b/skia/ext/raster_handle_allocator_win.cc
index 22d9bf4b0e2d6f43a92d1ea5f42cb2ffca9e0444..c921343da36735730bcb1a9ad396fec015967c9b 100644
--- a/skia/ext/raster_handle_allocator_win.cc
+++ b/skia/ext/raster_handle_allocator_win.cc
@@ -19,23 +19,24 @@
 
 namespace {
 
+struct HDCContextRec {
+  HDC fHDC;
+  HBITMAP fOldBitmap;  // we need to restore this to the HDC
+  HBITMAP fNewBitmap;  // we need to delete this

[bungeman-chromium, 2017/03/06 16:30:27]

nit: THIS IS SPARTA!!!!

all the other fields in this directory follow the Chromium naming convention of
important_field_

[reed1, 2017/03/06 16:41:51]

Done.

+};
+
 static void DeleteHDCCallback(void*, void* context) {
   DCHECK_NE(context, nullptr);
-  HDC hdc = static_cast<HDC>(context);
-
-  // We must get this before we call RestoreDC, as that will drop hbitmap and
-  // reselect the original/default bitmap.
-  HGDIOBJ hbitmap = GetCurrentObject(hdc, OBJ_BITMAP);
-  DCHECK_NE(hbitmap, nullptr);
+  const HDCContextRec* rec = static_cast<const HDCContextRec*>(context);
 
-  bool success = RestoreDC(hdc, -1); // effectly performs the deselect of hbitmap
+  // must first select back in the old bitmap, so that fNewBitmap will be
+  // unowned.
+  SelectObject(rec->fHDC, rec->fOldBitmap);
+  bool success = DeleteObject(rec->fNewBitmap);

[bungeman-chromium, 2017/03/06 16:30:27]

nit: maybe DCHECK that the old oblect is 'NewBitmap'

[reed1, 2017/03/06 16:41:51]

will delete what is returned

   DCHECK(success);
-
-  // Now we are the only owner, so we can delete our bitmap
-  success = DeleteObject(hbitmap);
-  DCHECK(success);
-  success = DeleteDC(hdc);
+  success = DeleteDC(rec->fHDC);
   DCHECK(success);
+  delete rec;
 }
 
 // Allocate the layer and fill in the fields for the Rec, or return false
@@ -47,9 +48,9 @@ static bool Create(int width,
                    bool do_clear,
                    SkRasterHandleAllocator::Rec* rec) {
   void* pixels;
-  HBITMAP hbitmap =
+  HBITMAP newBitmap =
       skia::CreateHBitmap(width, height, is_opaque, shared_section, &pixels);
-  if (!hbitmap) {
+  if (!newBitmap) {
     LOG(ERROR) << "CreateHBitmap failed";
     return false;
   }
@@ -60,21 +61,16 @@ static bool Create(int width,
 
   HDC hdc = CreateCompatibleDC(nullptr);
   if (!hdc) {
-    DeleteObject(hbitmap);
+    DeleteObject(newBitmap);
     return false;
   }
   SetGraphicsMode(hdc, GM_ADVANCED);
 
-  int saveIndex = SaveDC(hdc);  // so we can Restore in the delete callback
-  DCHECK_NE(saveIndex, 0);
-
-  // Be sure to select *after* we called SaveDC.
-  // Because we're using save/restore, we don't need to explicitly track the
-  // returned "previous" value.
-  SelectObject(hdc, hbitmap);
+  HBITMAP oldBitmap = static_cast<HBITMAP>(SelectObject(hdc, newBitmap));
+  DCHECK(oldBitmap);
 
   rec->fReleaseProc = DeleteHDCCallback;
-  rec->fReleaseCtx = hdc;
+  rec->fReleaseCtx = new HDCContextRec{hdc, oldBitmap, newBitmap};
   rec->fPixels = pixels;
   rec->fRowBytes = row_bytes;
   rec->fHandle = hdc;