135535 - we call DeleteObject() on DCs instead of DeleteDC()

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[timeless]

talkback TB4831788X

Comment 1

[Doron Rosenberg (IBM)]

Buildid: 2002040406 (win32)

nsImageWin::DrawComposited24
[d:\builds\seamonkey\mozilla\gfx\src\windows\nsImageWin.cpp, line 623]
nsImageWin::DrawComposited
[d:\builds\seamonkey\mozilla\gfx\src\windows\nsImageWin.cpp, line 653]
nsImageWin::Draw [d:\builds\seamonkey\mozilla\gfx\src\windows\nsImageWin.cpp,
line 530]
nsRenderingContextImpl::DrawImage
[d:\builds\seamonkey\mozilla\gfx\src\nsRenderingContextImpl.cpp, line 916]
nsImageBoxFrame::PaintImage
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsImageBoxFrame.cpp, line 533]
nsImageBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsImageBoxFrame.cpp, line 481]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsBoxFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1699]
nsBoxFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1836]
nsBoxFrame::Paint
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1652]
nsContainerFrame::PaintChild
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 256]
nsContainerFrame::PaintChildren
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 197]
nsContainerFrame::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 178]
PresShell::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5779]
nsView::Paint [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 280]
nsViewManager::RenderDisplayListElement
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1181]
nsViewManager::RenderViews
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1130]
nsViewManager::Refresh [d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp,
line 721]
nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1721]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83]
nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 869]
nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 891]
nsWindow::OnPaint [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,
line 4509]
nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3398]
nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1131]
KERNEL32.DLL + 0x363b (0xbff7363b)
KERNEL32.DLL + 0x24407 (0xbff94407) 
Trigger Reason  Access violation

Comment 2

[timeless]

msdn has this code:
    // Capture a copy of the source area.
    if (!StretchBlt(hdcSrc1, 0,0,nWidthDest,nHeightDest,
                    hdcSrc,  nXOriginSrc, nYOriginSrc, nWidthSrc, nHeightSrc, 
                    SRCCOPY))
         goto HANDLEERROR;

/*...*/
HANDLEERROR:
/*doCleanup()*/
    return FALSE;
}

we have:
 645 uid408    3.58   ::StretchBlt(memDC, 0, 0, aSWidth, aSHeight,
 646 blakeross 3.77                TheHDC, aDX, aDY, aDWidth, aDHeight, SRCCOPY);
 647 uid408    3.58 
 648                  /* Do composite */
 649                  DrawComposited24(screenBits, aSX, aSY, aSWidth, aSHeight);
<- this is one frame from where we crash

What's happening is that my system is running out of resources (this is w98, i
had n6.22 FogCity + mozNightly Classic open). when i crash, the windows crash
dialog is using emergency fonts, so i'm sure it's a resource problem.

Comment 3

[tor]

Guessing we're leaking win32 resources somewhere and eventually the bitmap
allocation fails so we end up dereferencing a null pointer.

Reassigning to a win32 person (dcone).

Comment 4

[Ere Maijala (slow)]

Just a quick note as I should've gone to bed already. nsImageWin has many places
where DC is deleted using DeleteObject() instead of DeleteDC(). I don't really
know what difference it makes, but it looks suspicious to me. 

Comment 5

[Ere Maijala (slow)]

Attachment: Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup

This patch adds a lot of null checks. HDC's are also deleted with DeleteDC()
instead of DeleteObject(). Drawing surface's ReleaseDC() is called to free
GetDC()'d HDC's (this only affects DDRAW version, which doesn't seem to be
functional anyway AFAICT). 

Timeless, could you try this patch?

Comment 6

[timeless]

Comment on attachment 78125 [details] [diff] [review]
Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup

@@ -525,8 +526,11 @@

       if (PR_FALSE == didComposite){
	 if (8==mAlphaDepth) {		    
-	    DrawComposited(TheHDC, aDX, aDY, aDWidth, aDHeight,
-	      aSX, srcy, aSWidth, aSHeight);
+	    if (!DrawComposited(TheHDC, aDX, aDY, aDWidth, aDHeight,
+	      aSX, srcy, aSWidth, aSHeight)) {
+		((nsDrawingSurfaceWin *)aSurface)->ReleaseDC();
+		return NS_ERROR_FAILURE;
+	    }
	 } else {
the indentation here was bad to begin with, but we should clean it up.
the file is probably using 2 space indentation, so something like:
@@ -525,8 +526,11 @@

       if (PR_FALSE == didComposite){
	 if (8==mAlphaDepth) {		    
-	   DrawComposited(TheHDC, aDX, aDY, aDWidth, aDHeight,
-			  aSX, srcy, aSWidth, aSHeight);
+	   if (!DrawComposited(TheHDC, aDX, aDY, aDWidth, aDHeight,
+			       aSX, srcy, aSWidth, aSHeight)) {
+	     ((nsDrawingSurfaceWin *)aSurface)->ReleaseDC();
+	     return NS_ERROR_FAILURE;
+	   }
	 } else {
might be better

@@ -631,14 +635,22 @@
...
+  HDC memDC = ::CreateCompatibleDC(TheHDC);
+  if (NULL == TheHDC) {
you should be checking memDC not TheHDC

@@ -1192,7 +1220,10 @@
...
+	 return (PR_FALSE);
don't use parens for return.


I can try to build w/ this patch and then transfer the output to that computer,
but it's not very easy, it'd be better for me if some flavor of this was in the
tree :)

Comment 7

[Ere Maijala (slow)]

Attachment: v2 Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup

Fixed the errors and cleaned up all return statements.

Comment 8

[Stuart Parmenter]

Moving bugs to new Image: GFX component

Comment 9

[dcone (gone)]

Attachment: Patch to use the correct call to delete the DC.

I think the first thing is to use this correct call to delete the DC, that
could be the cause of this problem.. if we are loosing resources.  The next
step.. would be the error checking but only after this patch gets in.  I have
done alot of cleaning up since this bug was filed.. so I am sure alot of that
has changed.. or been talken care of.

Comment 10

[Marc Attinasi]

Comment on attachment 81175 [details] [diff] [review]
Patch to use the correct call to delete the DC.

sr=attinasi - I bet this fixes a LOT of resource leaks, since DeleteObject does
not work for DC's (and since we are not checking the return value we won't know
that - see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/wcesdkr/htm/_w
cesdk_win32_deleteobject.asp)

Comment 11

[Kevin McCluskey (gone)]

Comment on attachment 81175 [details] [diff] [review]
Patch to use the correct call to delete the DC.

r=[e-mail]

Comment 12

[Ere Maijala (slow)]

Right, there were about 80 leaked DC's just from starting up and shutting down,
according to Sleuth QA. 

Comment 13

[timeless]

that url was for winCE, this one is for win32
http://msdn.microsoft.com/library/default.asp?url=
/library/en-us/gdi/devcons_1vsk.asp

Comment 14

[dcone (gone)]

fixed.  I will open new bugs to address any other issues brought up here.

Comment 15

[timeless]

i filed bug 143363 for the crash

Comment 16

[chris hofmann]

Comment on attachment 81175 [details] [diff] [review]
Patch to use the correct call to delete the DC.

a=chofmann,jesup,valeski for the 1.0 banch.

Comment 17

[timeless]

3.92.2.4 <[e-mail]> 14 May 2002 18:25