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: 220.127.116.1164 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
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.
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.
Yes, I will. Thanks, Paolo.
Created attachment 760206 [details] [diff] [review] 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 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.
Created attachment 760635 [details] [diff] [review] 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 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.)
Thanks, Paolo. Renaming and checking in. Btw, I pasted the try link incorrectly before, it is https://tbpl.mozilla.org/?tree=Try&rev=fa583e40fa59
No crashes at all checking the crashstats for the last 4 weeks. Verified fixed