880947 - crash in nsExternalAppHandler::CreateTransfer

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Scoobidiver (away)]

It first showed up in 24.0a1/20130605. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e3a4ebcf067&tochange=8f9ba85eb61c
It's likely a regression from bug 858234.

Signature 	nsExternalAppHandler::CreateTransfer() More Reports Search
UUID	691cf988-a64e-40f9-b730-d2b152130607
Date Processed	2013-06-07 22:52:47
Uptime	579
Last Crash	9.9 minutes before submission
Install Age	1.3 hours since version was first installed.
Install Time	2013-06-07 21:33:18
Product	Firefox
Version	24.0a1
Build ID	20130607031055
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
Build Architecture	x86
Build Architecture Info	GenuineIntel family 15 model 4 stepping 3
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2582, AdapterSubsysID: 02d91014, AdapterDriverVersion: 6.14.10.4764
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
Processor Notes 	sp-processor10_phx1_mozilla_com_12928:2012
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x2582
Total Virtual Memory	2147352576
Available Virtual Memory	1797607424
System Memory Use Percentage	59
Available Page File	2057306112
Available Physical Memory	428929024

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsExternalAppHandler::CreateTransfer 	uriloader/exthandler/nsExternalHelperAppService.cpp:1955
1 	xul.dll 	nsExternalAppHandler::ContinueSave 	uriloader/exthandler/nsExternalHelperAppService.cpp:2096
2 	xul.dll 	nsExternalAppHandler::SaveDestinationAvailable 	uriloader/exthandler/nsExternalHelperAppService.cpp:1973
3 	xul.dll 	NS_InvokeByIndex 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
4 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1480
5 	mozjs.dll 	js::Invoke 	js/src/vm/Interpreter.cpp:395
6 	mozjs.dll 	js::Interpret 	js/src/vm/Interpreter.cpp:2217
7 	mozjs.dll 	js::RunScript 	js/src/vm/Interpreter.cpp:352
8 	mozjs.dll 	js::Invoke 	js/src/vm/Interpreter.cpp:441
9 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5885
10 	xul.dll 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1433
11 	xul.dll 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:576
12 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:85
13 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:112
14 	xul.dll 	nsExternalAppHandler::RequestSaveDestination 	uriloader/exthandler/nsExternalHelperAppService.cpp:2019

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsExternalAppHandler%3A%3ACreateTransfer%28%29

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Line 1955 is

1952   rv = mTransfer->OnStateChange(nullptr, mRequest,
1953     nsIWebProgressListener::STATE_START |
1954     nsIWebProgressListener::STATE_IS_REQUEST |
1955     nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);

I see that I changed "if (mTransfer)" to NS_ENSURE_SUCCESS when creating the transfer. But that shouldn't alter the control flow.

Comment 2

[:Paolo Amadini]

I guess the aTransfer->Init call in InitializeDownload may be causing Cancel to
be invoked on us (where mTransfer is nullified), so we should check mCanceled
again after the Init call. I also noticed something else I missed during my
previous review: we ignore the return value of CreateTransfer, but if something
fails there, we should call Cancel ourselves (in both places where we call
CreateTransfer).

Monica, if you can create a patch for that, it will be a quick review.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Yes, I will. Thanks, Paolo.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Check if we're cancelled after calling nsITransfer::Init

Address Paolo's comments. One thing I'm not sure about, can we also get cancelled after calling InitializeDownload (i.e., anywhere in CreateTransfer)? That would also account for a null pointer.

Try: https://tbpl.mozilla.org/?tree=Try&rev=697b69654714

Comment 5

[:Paolo Amadini]

Comment on attachment 760206 [details] [diff] [review]
Check if we're cancelled after calling nsITransfer::Init

(In reply to Monica Chew [:mmc] (please use ?needinfo) from comment #4)
> One thing I'm not sure about, can we also get
> cancelled after calling InitializeDownload (i.e., anywhere in
> CreateTransfer)? That would also account for a null pointer.

In theory we could be canceled during any call to an external interface like
nsITransfer. So, the patch should be updated to take this into account.

Also, at first glance I suspect returning success from InitializeDownload when
canceled would not solve the crash. In fact, we don't need a separate
InitializeDownload function at all, we can put all the code in CreateTransfer.

To test the condition, maybe you could artificially introduce a call to Cancel
in nsITransfer::Init, or maybe this can be caused by a blocked download.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Make sure CreateTransfer never touches a null mTransfer object

I understna dthe crash now. Prior to the refactor, InitializeDownload and friends worked on aTransfer, which was not nullified during Cancel. I changed CreateTransfer to operate on aTransfer as well, only assigning to mTransfer when we know we are done touching aTransfer.

I think that makes all the calls to if (mCanceled) superfluous, but I left them in anyway. Please have another look.

Try: https://tbpl.mozilla.org/?tree=Try&rev=606918a14af7

Thanks,
Monica

Comment 7

[:Paolo Amadini]

Comment on attachment 760635 [details] [diff] [review]
Make sure CreateTransfer never touches a null mTransfer object

Review of attachment 760635 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the best solution here. Thanks!

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1914,5 @@
> +  // much that we can't launch the helper application or save to disk. Work on
> +  // aTransfer rather than mTransfer until we know we succeeded, to make it
> +  // clearer that this function is re-entrant.
> +  nsCOMPtr<nsITransfer> aTransfer = do_CreateInstance(
> +    NS_TRANSFER_CONTRACTID, &rv);

Convention: the "a" prefix is used for function arguments, so this should just be called "transfer". (By the way, I'm not a big fan of this convention, as it makes code less resilient when moved around like we're doing here, but it's still in the style guide and used throughout this module.)

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks, Paolo. Renaming and checking in. Btw, I pasted the try link incorrectly before, it is https://tbpl.mozilla.org/?tree=Try&rev=fa583e40fa59

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81dd6f59a133

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81dd6f59a133

Comment 11

[Paul Silaghi, QA [:pauly]]

No crashes at all checking the crashstats for the last 4 weeks. Verified fixed