45198 2010-09-03 14:12:23 -0700 DeleteDC called while non-stock objects still contained. 2010-09-07 12:57:56 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 bfulgham bfulgham aroben bweinstein commit-queue sfalken oldest_to_newest 274168 0 bfulgham 2010-09-03 14:12:23 -0700 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) 274182 1 66543 bfulgham 2010-09-03 14:31:48 -0700 Created attachment 66543 Small patch to clean up some bitmap release operations. 275171 2 66543 aroben 2010-09-07 11:12:06 -0700 Comment on attachment 66543 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? 275177 3 bfulgham 2010-09-07 11:20:18 -0700 (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. 275180 4 aroben 2010-09-07 11:28:14 -0700 (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. 275188 5 bfulgham 2010-09-07 11:37:26 -0700 (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]" 275191 6 aroben 2010-09-07 11:43:50 -0700 (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. 275192 7 66543 aroben 2010-09-07 11:44:10 -0700 Comment on attachment 66543 Small patch to clean up some bitmap release operations. r=me 275226 8 66543 commit-queue 2010-09-07 12:57:52 -0700 Comment on attachment 66543 Small patch to clean up some bitmap release operations. Clearing flags on attachment: 66543 Committed r66902: <http://trac.webkit.org/changeset/66902> 275228 9 commit-queue 2010-09-07 12:57:56 -0700 All reviewed patches have been landed. Closing bug. 66543 2010-09-03 14:31:48 -0700 2010-09-07 12:57:52 -0700 Small patch to clean up some bitmap release operations. bitmap_cleanup.patch text/plain 3063 bfulgham SW5kZXg6IFdlYmtpdC93aW4vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYmtpdC93aW4vQ2hh bmdlTG9nCShyZXZpc2lvbiA2Njc3MSkKKysrIFdlYmtpdC93aW4vQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTAtMDktMDMgIEJyZW50IEZ1bGdoYW0gIDxiZnVs Z2hhbUB3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIENsZWFuIHVwIGEgcG90ZW50aWFsIHJlc291cmNlIGxlYWsuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NTE5OAorCisgICAgICAgIFNl dmVyYWwgYml0bWFwIGRldmljZSBjb250ZXh0IHdlcmUgYmVpbmcgY3JlYXRlZCBhbmQgdXNlZCwK KyAgICAgICAgYW5kIGRlc3Ryb3llZCB3aXRob3V0IHJldHVybmluZyB0aGUgY29udGV4dCB0byBp dHMgb3JpZ2luYWwKKyAgICAgICAgc3RhdGUuICBUaGlzIHNob3dlZCB1cCBhcyBiaXRtYXAgbGVh a3MgaW4gQm91bmRzQ2hlY2tlci4KKworICAgICAgICAqIEZ1bGxzY3JlZW5WaWRlb0NvbnRyb2xs ZXIuY3BwOgorICAgICAgICAqIFdlYlZpZXcuY3BwOgorICAgICAgICAoV2ViVmlldzo6c2Nyb2xs QmFja2luZ1N0b3JlKToKKyAgICAgICAgKFdlYlZpZXc6OnBhaW50KToKKwogMjAxMC0wOS0wMyAg QWRhbSBSb2JlbiAgPGFyb2JlbkBhcHBsZS5jb20+CiAKICAgICAgICAgQXR0ZW1wdCB0byBmaXhp bmcgV2luZG93cyBuaWdodGxpZXMgYWdhaW4KSW5kZXg6IFdlYmtpdC93aW4vRnVsbHNjcmVlblZp ZGVvQ29udHJvbGxlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2Via2l0L3dpbi9GdWxsc2NyZWVuVmlk ZW9Db250cm9sbGVyLmNwcAkocmV2aXNpb24gNjY3MzMpCisrKyBXZWJraXQvd2luL0Z1bGxzY3Jl ZW5WaWRlb0NvbnRyb2xsZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00NzYsNyArNDc2LDcgQEAg dm9pZCBGdWxsc2NyZWVuVmlkZW9Db250cm9sbGVyOjpkcmF3KCkKICAgICBIREMgd2luZG93REMg PSBHZXREQyhtX2h1ZFdpbmRvdyk7CiAgICAgSERDIGJpdG1hcERDID0gQ3JlYXRlQ29tcGF0aWJs ZURDKHdpbmRvd0RDKTsKICAgICA6OlJlbGVhc2VEQyhtX2h1ZFdpbmRvdywgd2luZG93REMpOwot ICAgIFNlbGVjdE9iamVjdChiaXRtYXBEQywgbV9iaXRtYXAuZ2V0KCkpOworICAgIEhHRElPQkog b2xkQml0bWFwID0gU2VsZWN0T2JqZWN0KGJpdG1hcERDLCBtX2JpdG1hcC5nZXQoKSk7CiAKICAg ICBHcmFwaGljc0NvbnRleHQgY29udGV4dChiaXRtYXBEQywgdHJ1ZSk7CiAKQEAgLTU0Myw2ICs1 NDMsNyBAQCB2b2lkIEZ1bGxzY3JlZW5WaWRlb0NvbnRyb2xsZXI6OmRyYXcoKQogCiAgICAgY29u dGV4dC5yZXN0b3JlKCk7CiAKKyAgICA6OlNlbGVjdE9iamVjdChiaXRtYXBEQywgb2xkQml0bWFw KTsKICAgICA6OkRlbGV0ZURDKGJpdG1hcERDKTsKIH0KIApJbmRleDogV2Via2l0L3dpbi9XZWJW aWV3LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJraXQvd2luL1dlYlZpZXcuY3BwCShyZXZpc2lvbiA2 NjczMykKKysrIFdlYmtpdC93aW4vV2ViVmlldy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTgyNiw3 ICs4MjYsNyBAQCB2b2lkIFdlYlZpZXc6OnNjcm9sbEJhY2tpbmdTdG9yZShGcmFtZVZpCiAgICAg Ly8gQ29sbGVjdCBvdXIgZGV2aWNlIGNvbnRleHQgaW5mbyBhbmQgc2VsZWN0IHRoZSBiaXRtYXAg dG8gc2Nyb2xsLgogICAgIEhEQyB3aW5kb3dEQyA9IDo6R2V0REMobV92aWV3V2luZG93KTsKICAg ICBIREMgYml0bWFwREMgPSA6OkNyZWF0ZUNvbXBhdGlibGVEQyh3aW5kb3dEQyk7Ci0gICAgOjpT ZWxlY3RPYmplY3QoYml0bWFwREMsIG1fYmFja2luZ1N0b3JlQml0bWFwLT5oYW5kbGUoKSk7Cisg ICAgSEdESU9CSiBvbGRCaXRtYXAgPSA6OlNlbGVjdE9iamVjdChiaXRtYXBEQywgbV9iYWNraW5n U3RvcmVCaXRtYXAtPmhhbmRsZSgpKTsKICAgICAKICAgICAvLyBTY3JvbGwgdGhlIGJpdG1hcC4K ICAgICBSRUNUIHNjcm9sbFJlY3RXaW4oc2Nyb2xsVmlld1JlY3QpOwpAQCAtODQ4LDYgKzg0OCw3 IEBAIHZvaWQgV2ViVmlldzo6c2Nyb2xsQmFja2luZ1N0b3JlKEZyYW1lVmkKICAgICB1cGRhdGVC YWNraW5nU3RvcmUoZnJhbWVWaWV3LCBiaXRtYXBEQywgZmFsc2UpOwogCiAgICAgLy8gQ2xlYW4g dXAuCisgICAgOjpTZWxlY3RPYmplY3QoYml0bWFwREMsIG9sZEJpdG1hcCk7CiAgICAgOjpEZWxl dGVEQyhiaXRtYXBEQyk7CiAgICAgOjpSZWxlYXNlREMobV92aWV3V2luZG93LCB3aW5kb3dEQyk7 CiB9CkBAIC05ODcsNyArOTg4LDcgQEAgdm9pZCBXZWJWaWV3OjpwYWludChIREMgZGMsIExQQVJB TSBvcHRpbwogICAgIG1fcGFpbnRDb3VudCsrOwogCiAgICAgSERDIGJpdG1hcERDID0gOjpDcmVh dGVDb21wYXRpYmxlREMoaGRjKTsKLSAgICA6OlNlbGVjdE9iamVjdChiaXRtYXBEQywgbV9iYWNr aW5nU3RvcmVCaXRtYXAtPmhhbmRsZSgpKTsKKyAgICBIR0RJT0JKIG9sZEJpdG1hcCA9IDo6U2Vs ZWN0T2JqZWN0KGJpdG1hcERDLCBtX2JhY2tpbmdTdG9yZUJpdG1hcC0+aGFuZGxlKCkpOwogCiAg ICAgLy8gVXBkYXRlIG91ciBiYWNraW5nIHN0b3JlIGlmIG5lZWRlZC4KICAgICB1cGRhdGVCYWNr aW5nU3RvcmUoZnJhbWVWaWV3LCBiaXRtYXBEQywgYmFja2luZ1N0b3JlQ29tcGxldGVseURpcnR5 LCB3aW5kb3dzVG9QYWludCk7CkBAIC0xMDEyLDYgKzEwMTMsNyBAQCB2b2lkIFdlYlZpZXc6OnBh aW50KEhEQyBkYywgTFBBUkFNIG9wdGlvCiAgICAgICAgIHVwZGF0ZVJvb3RMYXllckNvbnRlbnRz KCk7CiAjZW5kaWYKIAorICAgIDo6U2VsZWN0T2JqZWN0KGJpdG1hcERDLCBvbGRCaXRtYXApOwog ICAgIDo6RGVsZXRlREMoYml0bWFwREMpOwogCiAgICAgaWYgKCFkYykK