DeleteDC called while non-stock objects still contained.

[Brent Fulgham]

BoundsChecker identified a resource leak in WebView.cpp (paint), see line 990-1015.

1. A new compatible device context is created to match our target dc. (line 989).
2. We immediately select our backing store into the DC, which replaces whatever compatible bitmap was in the context.
3. We do stuff...
4. We destroy the context.

BoundsChecker claims this is wrong, because the context still contains our non-stock bitmap in it.

Proposed change:

===================================================================
--- WebView.cpp (revision 66733)
+++ WebView.cpp (working copy)
@@ -987,7 +987,7 @@
     m_paintCount++;

     HDC bitmapDC = ::CreateCompatibleDC(hdc);
-    ::SelectObject(bitmapDC, m_backingStoreBitmap->handle());
+    HGDIOBJ oldBitmap = ::SelectObject(bitmapDC, m_backingStoreBitmap->handle()
);

     // Update our backing store if needed.
     updateBackingStore(frameView, bitmapDC, backingStoreCompletelyDirty, window
sToPaint);
@@ -1012,6 +1012,7 @@
         updateRootLayerContents();
 #endif

+    ::SelectObject(bitmapDC, oldBitmap);
     ::DeleteDC(bitmapDC);

     if (!dc)

[Brent Fulgham]

Created attachment 66543 [details]
Small patch to clean up some bitmap release operations.

[Adam Roben (:aroben)]

Comment on attachment 66543 [details]
Small patch to clean up some bitmap release operations.

I don't think there are any leaks here. In FullscreenVideoController, the "potentially leaked" bitmap is held in the m_bitmap OwnPtr. In WebView, the "potentially leaked" bitmap is held in the m_backingStoreBitmap RefCountedHBITMAP. In both cases the bitmap is destroyed later.

Is there any other benefit to adding these SelectObject calls?

[Brent Fulgham]

(In reply to comment #2)
> (From update of attachment 66543 [details])
> I don't think there are any leaks here. In FullscreenVideoController, the "potentially leaked" bitmap is held in the m_bitmap OwnPtr. In WebView, the "potentially leaked" bitmap is held in the m_backingStoreBitmap RefCountedHBITMAP. In both cases the bitmap is destroyed later.
> 
> Is there any other benefit to adding these SelectObject calls?

I thought the issue was that CreateCompatibleDC creates a DC with a default bitmap object.  When you select in the m_backingStoreBitmap, it replaces the default bitmap object (the handle of which is returned during the SelectObject call.)  If we destroy the DC while it still contains the m_backingStoreBitmap, the original 'compatible' bitmap is left dangling.

[Adam Roben (:aroben)]

(In reply to comment #3)
> I thought the issue was that CreateCompatibleDC creates a DC with a default bitmap object.  When you select in the m_backingStoreBitmap, it replaces the default bitmap object (the handle of which is returned during the SelectObject call.)  If we destroy the DC while it still contains the m_backingStoreBitmap, the original 'compatible' bitmap is left dangling.

CreateCompatibleDC creates a DC with the stock bitmap selected. The description of the stock bitmap from <http://blogs.msdn.com/b/oldnewthing/archive/2010/04/16/9996968.aspx> makes me think we don't need to bother to select it back in to the DC before destroying the DC, as the stock bitmap is shared across the whole process.

[Brent Fulgham]

(In reply to comment #4)
> (In reply to comment #3)
> > I thought the issue was that CreateCompatibleDC creates a DC with a default bitmap object.  When you select in the m_backingStoreBitmap, it replaces the default bitmap object (the handle of which is returned during the SelectObject call.)  If we destroy the DC while it still contains the m_backingStoreBitmap, the original 'compatible' bitmap is left dangling.
> 
> CreateCompatibleDC creates a DC with the stock bitmap selected. The description of the stock bitmap from <http://blogs.msdn.com/b/oldnewthing/archive/2010/04/16/9996968.aspx> makes me think we don't need to bother to select it back in to the DC before destroying the DC, as the stock bitmap is shared across the whole process.

You feel that an exchange such as the following indicates that we should NOT return the context to its initial state before destroying it?:

"What I am asking is if, *in this specific case*, not restoring the DC before deleting it would introduce a very small leak of some kind or not. And if it causes a leak, what exactly is leaking?

[Since you know it's not something you're supposed to be doing in the first place, what does it matter what happens when you do it? Just don't do it. Here's what happens: An error occurs. -Raymond]"

[Adam Roben (:aroben)]

(In reply to comment #5)
> You feel that an exchange such as the following indicates that we should NOT return the context to its initial state before destroying it?:
> 
> "What I am asking is if, *in this specific case*, not restoring the DC before deleting it would introduce a very small leak of some kind or not. And if it causes a leak, what exactly is leaking?
> 
> [Since you know it's not something you're supposed to be doing in the first place, what does it matter what happens when you do it? Just don't do it. Here's what happens: An error occurs. -Raymond]"

No, that sounds like we should return the context to its initial state. I hadn't seen those comments.

[Adam Roben (:aroben)]

Comment on attachment 66543 [details]
Small patch to clean up some bitmap release operations.

r=me

[WebKit Commit Bot]

Comment on attachment 66543 [details]
Small patch to clean up some bitmap release operations.

Clearing flags on attachment: 66543

Committed r66902: <http://trac.webkit.org/changeset/66902>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.