612272 - crash [@ ShowNativePrintDialog ] when trying to print some pages

Status: VERIFIED FIXED

(Core :: Printing: Setup, defect)

Description

[Scoobidiver (away)]

It is a new crash signature. Crashes first appeared in 4.0b8pre/20101111 build.
It is #134 top crasher in 4.0b8pre for the last week.

Comments say:
"trying to print a page from Vanguard.com"
"trying to print a page from the Ohio Business Gateway"

Signature	ShowNativePrintDialog
UUID	ea23fbf7-3e30-4fa1-bee8-becb92101115
Time 	2010-11-15 05:52:59.504941
Uptime	82487
Last Crash	84243 seconds (23.4 hours) before submission
Install Age	144905 seconds (1.7 days) since version was first installed.
Product	Firefox
Version	4.0b8pre
Build ID	20101113042433
Branch	2.0
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION_WRITE
Crash Address	0x4
App Notes 	AdapterVendorID: 10de, AdapterDeviceID: 0421

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	ShowNativePrintDialog 	embedding/components/printingui/src/win/nsPrintDialogUtil.cpp:888
1 	xul.dll 	NativeShowPrintDialog 	embedding/components/printingui/src/win/nsPrintDialogUtil.cpp:1476
2 	xul.dll 	nsPrintingPromptService::ShowPrintDialog 	embedding/components/printingui/src/win/nsPrintingPromptService.cpp:192
3 	xul.dll 	nsPrintEngine::DoCommonPrint 	layout/printing/nsPrintEngine.cpp:618
4 	xul.dll 	nsPrintEngine::CommonPrint 	layout/printing/nsPrintEngine.cpp:445
5 	xul.dll 	nsPrintEngine::Print 	layout/printing/nsPrintEngine.cpp:763
6 	xul.dll 	DocumentViewerImpl::Print 	layout/base/nsDocumentViewer.cpp:3640
7 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
8 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1589
9 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:708
10 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:881
11 	mozjs.dll 	js::JSProxyHandler::call 	js/src/jsproxy.cpp:248
12 	mozjs.dll 	JSCrossCompartmentWrapper::call 	js/src/jswrapper.cpp:593
13 	mozjs.dll 	js::JSProxy::call 	js/src/jsproxy.cpp:791
14 	mozjs.dll 	js::proxy_Call 	js/src/jsproxy.cpp:1016
15 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:701
16 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4755
17 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:665
18 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:768
19 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:881
20 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:4908
21 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2171
22 	xul.dll 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:228
23 	xul.dll 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1112
24 	xul.dll 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:1208
25 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:628

The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df1d1ff6b489&tochange=0f17e5f1eb01

More reports at:
http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=ShowNativePrintDialog

Comment 1

[Benjamin Smedberg]

#134 is not serious enough to block on, unless we get reliable STR.

Comment 2

[Scoobidiver (away)]

It is now #18 top crasher in 4.0b8pre for the last week.
I think it should be a blocker for Gecko 2.0.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's going to be hard to block on without steps to reproduce.

Comment 4

[timeless]

849 ShowNativePrintDialog(HWND aHWnd,
858 HGLOBAL hDevNames = NULL;
880 // Now create a DEVNAMES struct so the the dialog is initialized correctly.
883 hDevNames = (HGLOBAL)::GlobalAlloc(GHND, sizeof(wchar_t) * (len + 1) +
884 sizeof(DEVNAMES));
885 DEVNAMES* pDevNames = (DEVNAMES*)::GlobalLock(hDevNames);
886 pDevNames->wDriverOffset = sizeof(DEVNAMES)/sizeof(wchar_t);
887 pDevNames->wDeviceOffset = sizeof(DEVNAMES)/sizeof(wchar_t);
888 pDevNames->wOutputOffset = sizeof(DEVNAMES)/sizeof(wchar_t)+len; 

http://msdn.microsoft.com/en-us/library/ms646833(VS.85).aspx
typedef struct tagDEVNAMES {
  WORD wDriverOffset;
  WORD wDeviceOffset;
  WORD wOutputOffset;
  WORD wDefault;
} DEVNAMES, *LPDEVNAMES;

http://msdn.microsoft.com/en-us/library/aa366584(VS.85).aspx
LPVOID WINAPI GlobalLock(
  __in  HGLOBAL hMem
);

hMem [in]

    A handle to the global memory object. This handle is returned by either the GlobalAlloc or GlobalReAlloc function.

Return Value

If the function succeeds, the return value is a pointer to the first byte of the memory block.

If the function fails, the return value is NULL. To get extended error information, call GetLastError.

Someone called Lock without checking for failure. Someone else clearly has a lock.

To analyze this (for steps to reproduce) you're going to want to look at either all modules, all threads, or all processes. 

I'd sooner just handle this with a correct null check.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess we can try to null check our way to victory here, but I can't see why GlobalLock would ever fail. It's not an exclusive lock, it's just an old pre-Win32 function from the days before virtual memory when you could allocate moveable memory blocks that Windows relocates.

Comment 6

[timeless]

Attachment: add null checks and manage printernames

So, afaict we also tried very hard to leak printerNames

the null checks for what i believe is the crash are in:
@@ -851,45 +849,52 @@ ShowNativePrintDialog(HWND              

I compiled this but don't have the cycles to do more than that.

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 494361 [details] [diff] [review]
add null checks and manage printernames

Nit:
> +  if (printerName.IsEmpty()) return NS_ERROR_FAILURE;
move the return to a separate line
r=mats

Spotted an unrelated problem while reviewing:
http://hg.mozilla.org/mozilla-central/annotate/5f9204fe5cd5/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#l1205
the "NS_ENSURE_SUCCESS(rv, rv);" on line 1225 should be removed because
there's no assignment of 'rv' after the initialization on line 1212.
There's no other use of 'rv' after that so line 1212 can also be removed.
This is a regression from bug 461283 (so it's also in 1.9.1/1.9.2).
Feel free to fix that too while you're here.

BTW, I tested the patch with the printer name set to "" in prefs -
it worked fine.

Comment 8

[timeless]

Attachment: and remove bogus failure

mats, re: that rv, wow

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 494463 [details] [diff] [review]
and remove bogus failure

Doesn't need approval, this bug is blocking.

Comment 10

[chris hofmann]

we should try to get this for beta8.  its now ranked #6 on b8pre.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5bab39756981

Comment 13

[chris hofmann]

no more crashes after buildid 20101203030309

Comment 14

[Ed Morley [:emorley]]

Believed to have caused bug 619330.