Closed Bug 135535 Opened 22 years ago Closed 22 years ago

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

Tracking

(Not tracked)

Status:

VERIFIED FIXED

People

(Reporter: timeless, Assigned: dcone)

References

(
URL
)

Details

(Keywords: crash)

Attachments

(2 files, 1 obsolete file)

Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup 22 years ago Ere Maijala (slow) 12.73 KB, patch		Details \| Diff \| Splinter Review
v2 Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup 22 years ago Ere Maijala (slow) 14.29 KB, patch		Details \| Diff \| Splinter Review
Patch to use the correct call to delete the DC. 22 years ago dcone (gone) 2.28 KB, patch	kmcclusk : review+ attinasi : superreview+ chofmann : approval+	Details \| Diff \| Splinter Review

timeless

Reporter

Description

•

22 years ago

talkback TB4831788X

Doron Rosenberg (IBM)

Comment 1

•

22 years ago

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

timeless

Reporter

Comment 2

•

22 years ago

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.

Assignee: pavlov → tor

URL: http://support.microsoft.com/default....

Keywords: crash

tor

Comment 3

•

22 years ago

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).

Assignee: tor → dcone

Ere Maijala (slow)

Comment 4

•

22 years ago

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.

Ere Maijala (slow)

Comment 5

•

22 years ago

Attached patch Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup (obsolete) — Details — Splinter Review

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?

timeless

Reporter

Comment 6

•

22 years ago

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 :)

Ere Maijala (slow)

Comment 7

•

22 years ago

Attached patch v2 Patch to add error checking, DeleteObject() -> DeleteDC(), cleanup — Details — Splinter Review

Fixed the errors and cleaned up all return statements.

Attachment #78125 - Attachment is obsolete: true

Stuart Parmenter

Comment 8

•

22 years ago

Moving bugs to new Image: GFX component

Component: ImageLib → Image: GFX

dcone (gone)

Assignee

Comment 9

•

22 years ago

Attached patch Patch to use the correct call to delete the DC. — Details — Splinter Review

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.

Marc Attinasi

Comment 10

•

22 years ago

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)

Attachment #81175 - Flags: superreview+

Kevin McCluskey (gone)

Comment 11

•

22 years ago

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

r=kmcclusk@netscape.com

Attachment #81175 - Flags: review+

Ere Maijala (slow)

Comment 12

•

22 years ago

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

timeless

Reporter

Comment 13

•

22 years ago

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

dcone (gone)

Assignee

Comment 14

•

22 years ago

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

Status: NEW → RESOLVED

Closed: 22 years ago

Resolution: --- → FIXED

timeless

Reporter

Comment 15

•

22 years ago

i filed bug 143363 for the crash

Status: RESOLVED → VERIFIED

Summary: crash [@nsImageWin::DrawComposited24] → we call DeleteObject() on DCs instead of DeleteDC()

chris hofmann

Comment 16

•

22 years ago

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.

Attachment #81175 - Flags: approval+

timeless

Reporter

Comment 17

•

22 years ago

3.92.2.4 <dcone@netscape.com> 14 May 2002 18:25

Keywords: fixed1.0.0

timeless

Reporter

Updated

•

21 years ago

Component: Image: GFX → GFX: Win32

Nobody; OK to take it and work on it

Updated

•

16 years ago

Product: Core → Core Graveyard

You need to log in before you can comment on or make changes to this bug.