156318 - [FIX]Trunk, M1BR, M11A crashes while printing [@ GDI32.DLL][@ CNMDR3e.DLL]

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[greer]

Users who are crashing with stacks showing the GDI32.DLL as the stack signature
are uniformly reporting the crash happening during the printing process.
Included below in the stack from the Trunk reports, followed by comments from
M11A because it provides a much larger sampling. (The first Trunk comment in
particular might be a usable testcase):

Stack Trace: 

	 GDI32.DLL + 0x99e6 (0x77f499e6)
	 GDI32.DLL + 0xa26f (0x77f4a26f)
	 GDI32.DLL + 0xa2f5 (0x77f4a2f5)
	 DocumentViewerImpl::Print
[c:/builds/seamonkey/mozilla/content/base/src/nsDocumentViewer.cpp  line 7048]
	 XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 106]
	 XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 1996]
	 XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1267]
	 js_Invoke
[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 790]
	 js_Interpret
[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2744]
	 js_Invoke
[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 806]
	 js_InternalInvoke
[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 881]
	 JS_CallFunctionValue
[c:/builds/seamonkey/mozilla/js/src/jsapi.c  line 3430]
	 nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1045]
	 nsJSEventListener::HandleEvent
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp  line 184]
	 nsEventListenerManager::HandleEventSubType
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp  line
1222]
	 nsEventListenerManager::HandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp  line
2221]
	 nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 3447]
	 nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 3466]
	 nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 3466]
	 PresShell::HandleDOMEventWithTarget
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6240]
	 nsButtonBoxFrame::MouseClicked
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp  line 195]
	 nsButtonBoxFrame::HandleEvent
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp  line 142]
	 PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6209]
	 PresShell::HandleEventWithTarget
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6160]
	 nsEventStateManager::CheckForAndDispatchClick
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp  line 2754]
	 nsEventStateManager::PostHandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp  line 1760]
	 PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6213]
	 PresShell::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6115]
	 nsViewManager::HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp  line 2105]
	 nsView::HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsView.cpp  line 306]
	 nsViewManager::DispatchEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp  line 1916]
	 HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsView.cpp  line 83]
	 nsWindow::DispatchEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 1038]
	 nsWindow::DispatchWindowEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 1055]
	 nsWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 5097]
	 ChildWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 5352]
	 nsWindow::ProcessMessage
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 3814]
	 nsWindow::WindowProc
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp  line 1304]
	 USER32.DLL + 0x3eb0 (0x77e13eb0)
	 USER32.DLL + 0x401a (0x77e1401a)
	 USER32.DLL + 0x92da (0x77e192da)
	 nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 458]
	 main1
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1472]
	 main
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1808]
	 WinMain
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1826]
	 WinMainCRTStartup()
	 KERNEL32.DLL + 0x7903 (0x77e87903)     
     (8087649)	URL: www.apple.com
     (8087649)	Comments: I was at the Apple store  and I had a configuration selected. I
clicked on 'Remove' to remove an item from my cart. I wanted to print out a copy
of the list which still included the item  so I clicked the 'Back' button on the
browser  and the checkout cart came up with that item placed back in the
checkout cart  and then I clicked on Print  and then Ok  and boom  it crashed.
     (8073099)	Comments: Printing SGI STL pages again. The Lizard died with a fraction of
the Print dialogue box overlaying the File menu  with New highlighted.  I was
pressing keys pretty quickly  like the last time it crashed in the same way.
     (8072742)	Comments: Printing something from the SGI STL page . . . 
     (7991235)	Comments: I was sending the order to print a selected part of a web page
     (7975419)	URL: http://www.artistcollaboration.com
     (7975419)	Comments: printing


M11A Comments:
--------------
(8064265)
URL:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/install/hh/install/inf-format_0igi.asp
     (8064265)	Comments: Was scrolling in Print Preview mode
     (8005954)	Comments: 1. crash when attempting to print new message.2. also  multiple
windows for each newsgroup clicked on. can't just keep one window.
     (7960333)	URL:
http://ep.pennnet.com/Articles/Article_Display.cfm?Section=Articles&Subsection=Display&ARTICLE_ID=147049
     (7960333)	Comments: Printing
     (7937029)	Comments: Printing a confirmation of an online order from within Mozilla.
     (7932296)	URL: msdn.microsoft.com
     (7932296)	Comments: Attempting to print a webpage.  I have printed a page from the
same site just seconds before without problems.  (Both sites did use
frames....not sure if that matters)  
     (7929146)	Comments: Browsing my account on etrade.
     (7894944)	Comments: trying to print after doing a print preview  cthen hanging the
layout to landscape and zoom to 80%
     (7889660)	URL: nucleartourist.com
     (7889660)	Comments: I was trying to print a page
     (7857436)	URL: www.geocaching.com
     (7857436)	Comments: printing crashes frequently on www.geocaching.com
     (7854938)	Comments: tried to start another printing process when earlier was still
underwway
     (7817319)	Comments: printing a store.apple.com order configuration
     (7816431)	Comments: while pressing ok-button of print dialog
     (7816152)	Comments: normal browsing  nothing special
     (7808331)	Comments: printing a doc whilst another was loading
     (7796805)	Comments: browsing und printing of larger HTML-Files
     (7772624)	Comments: failed when i print a page
(8078466)
URL: http://www.choisser.com/packet/part17.html
     (8078466)	Comments: Printing via USB while browsing above URL.
     (8078068)	URL: http://www.choisser.com/packet/part10.html
     (8078068)	Comments: Web access and printing via USB.
     (8054318)	URL: Printing a page where was frames with the order to print each frame
separately.
     (8015211)	URL: http://www.f-secure.com/v-descs/bady.shtml
     (8015211)	Comments: Printing the page to a networked hp 2100
     (7992764)	Comments: I tried to print a mail from squirrelmail.I pressed the print
button  which launch the printer parameters window and then it broke down when I
pressed OK.
     (7968581)	Comments: trying to print from a fandango ticket purchase
     (7968435)	URL: http://www.cnet.com/software/0-6688749-8-9854920-1.html?tag=sptlt
     (7968435)	Comments: tab-browsing a SW review divided into multiple HTML pages
     (7965324)	Comments: Printing an email message
     (7933190)	Comments: I was viewing html page source. I clicked on "print" from the html
page source window and Mozilla crashed.
     (7920037)	URL: http://www.zvuki.ru/M/P/849/
     (7920037)	Comments: I was about to print this page.
     (7899321)	URL: www.google.com
     (7899321)	Comments: Printing.
     (7895151)	URL: www.economist.com
     (7895151)	Comments: Printing a page from economist.com
     (7861428)	URL: http:\\xulplanet.com
     (7861428)	Comments: Printing a map on Yahoo.
     (7843955)	Comments: attempt print screen on pcworld.com  hardware reviews
     (7842094)	URL: www.sapo.pt
     (7842094)	Comments: printing page in epson stylus color 580usb
     (7820358)	URL: www.ebay.com
     (7820358)	Comments: trying to print from a web page.
     (7814577)	Comments: printing

Comment 1

[rods (gone)]

I have looked at about 10 of the TB reports and the build was all 2002061104, so
can I assume it isn't happening now on the trunk?

Comment 2

[greer]

Rod, 

The Trunk reports show that it is happening as recently as the 7-6 build.

GDI32.DLL   7 incidents
Build ID range: 2002062704 to 2002070608

Comment 3

[greer]

Trunk is also showing similar crash stacks ending in CNMDR3e.DLL, adding that
signature to the summary.

Comment 4

[rods (gone)]

Attachment: patch v1

In general, I have changed all the DEVMODE allocating and freeing to native
Windows calls HeapAlloc and HeapFree. It is unclear friom the their docs what
type of memory should be passed into the platform calls global/heap etc. so I
thought it would be best to use all Windows allocated heap memory

The nsDeviceContextSpecWin no longer has to worry about HGLOBAL memory versus
non-HGLOBAL memory so this class is now cleaner.

In nsPrintDialogUtil.cpp (line 799) we weren't freeing memory where we should
have been, the free was in an else clause which was wrong.

The CRASH fix is in nsPrintSettingsWin.cpp:
It was creating and copying only the non-platform specific data by using the
"sizeof" of the DEVMODE struct, instead of checking the struct size with dmSize
and the size of the private (device-specific) data with dmDriverExtra. Now it
creates the correct size of memory and copies all the non-private data and
private data.

With a lot of debgging I made sure we are allocating and deallocating the
DEVMODEs correctly and in the proper order.

Comment 5

[Kevin McCluskey (gone)]

nsbeta1+. [ADT1]. This crash is happending on both the trunk and branch.

Comment 6

[rods (gone)]

NOTE: The real crash is not in the DocumentViewer like all the stack traces
show, the crash is really in the nsDeviceContextWin::GetDeviceContextFor where
it is creating the new DC with the platform call ::CreateDC, which it is using a
DEVMODE to do the creation.


RISK assessment:

I spent a lot of time debugging this and making sure all the allocation and
deallocations are correct and matched up. I think it is safer to use the
HeapAlloc/Free methods because the memory is used ONLY by platform calls.

The fix itself is were there wasn't enough memory being allocated and
initialized for the new DEVMODE (the values would be copied from the old
DEVMODE) and although we were not writing into memory we should not have been,
inside the Windows library certain calls were assuming some data was there that
wasn't, so it was really looking at garbage values and then sometimes crashing
when it was creating the new DC.

I think this is why it isn't 100% reproducable.

The entire patch is ever so slightly higher than low risk. (because of all the
testing and debugging of the memory allocs and frees)

Comment 7

[kinmoz]

Comment on attachment 90630 [details] [diff] [review]
patch v1

Is there potential for freeing pNewDevMode twice in this code, if the
allocation of hCurrentDevMode fails and the call to DocumentProperties() fails?
Should we just bail if hCurrentDevMode is null?


-    pNewDevMode = (LPDEVMODE)malloc(dwNeeded);
+    pNewDevMode = (LPDEVMODE)::HeapAlloc (::GetProcessHeap(),
HEAP_ZERO_MEMORY, dwNeeded);
     if (!pNewDevMode) return NULL;

-    hGlobalDevMode = (HGLOBAL)::GlobalAlloc(GHND, dwNeeded);
-    if (!hGlobalDevMode) {
-      free(pNewDevMode);
+    hCurrentDevMode = (LPDEVMODE)::HeapAlloc (::GetProcessHeap(),
HEAP_ZERO_MEMORY, dwNeeded);
+    if (!hCurrentDevMode) {
+      ::HeapFree(::GetProcessHeap(), 0, pNewDevMode);
     }

     dwRet = ::DocumentProperties(gParentWnd, hPrinter, aPrintName,
pNewDevMode, NULL, DM_OUT_BUFFER);

     if (dwRet != IDOK) {
-      free(pNewDevMode);
-      ::GlobalFree(hGlobalDevMode);
+      ::HeapFree(::GetProcessHeap(), 0, pNewDevMode);
+      ::HeapFree(::GetProcessHeap(), 0, hCurrentDevMode);
       ::ClosePrinter(hPrinter);
       return NULL;
     }



Also, there are a couple of places where you HeapAlloc() an LPDEVMODE object
and then immediately call DocumentProperties() without checking the
success/failure of the allocation. Intentional?

Comment 8

[rods (gone)]

Attachment: patch v2

Fixed up issues mentioned by kin

Comment 9

[Chris Waterson]

Looks okay to me, but I'd like to see someone more knowledgable in Win32 than me
review the size calculation in CopyDevMode.

Comment 10

[Chris Waterson]

Comment on attachment 90641 [details] [diff] [review]
patch v2

sr=waterson, pending r= of windows god.

Comment 11

[rpotts (gone)]

for safety, in EnumeratenativePrinters(...) you should return a NS_ERROR_FAILURE 
if the first call to ::EnumPrinters(...) fails...

since 'dwSizeNeeded' is not initialized, if this call fails, then HeapAlloc() 
will be passed a bogus size...

btw, if the NS_ENSURE_TRUE(...) fails in nsDeviceContextWinSpec::Init(...) [line 
420] then 'deviceName', 'driverName' and 'devMode' are leaked.  also, it appears 
that 'printerName' is being leaked from Init(...)

I'm a little concerned that CreateGlobalDevModeAndInit(...) is no longer 
returning a HGLOBAL block...  This means that the 'hDevMode' member in 
the PRINTDLG struct is *not* being passed a 'movable' block (as documented)...

Is this ok?

-- rick

Comment 12

[dcone (gone)]

Comment on attachment 90641 [details] [diff] [review]
patch v2

r=dcone

Comment 13

[rods (gone)]

Attachment: patch v3

This includes all of rick's suggestions and I changed
CreateGlobalDevModeAndInit(...) back to creating a HGLOBAL (that part of the
original change was unintentional)

Comment 14

[rods (gone)]

Attachment: patch v4

Missed a small change (see previous comment)

Comment 15

[rods (gone)]

Comment on attachment 90691 [details] [diff] [review]
patch v4

bringing don's review forward

Comment 16

[rpotts (gone)]

Comment on attachment 90691 [details] [diff] [review]
patch v4

sr=[e-mail]

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 90691 [details] [diff] [review]
patch v4

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 18

[rods (gone)]

fixed

Comment 19

[John P Baker]

I won't be able to check it myself for a while, but does this fix bug 155736 as well?
It is just that the bit about not copying the device specific data sounds just like it.

Comment 20

[Jaime Rodriguez, Jr.]

sujay: can you verify this as fixed on the trunk?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Approved for branch as well.  Please change mozilla1.0.1+ to fixed1.0.1 when
checked into branch.

Comment 22

[sujay]

I cannot reproduce any of the crashes on trunk....so I"m gonna mark this verified...

Comment 23

[scottputterman]

adding adt1.0.1+.  Please check this in today.

Comment 24

[sujay]

verified on 7/17 branch build.

Comment 25

[sujay]

removing fixed1.0.1 keyword.