820067 - crash in nsDocShell::InternalLoad

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Scoobidiver (away)]

It first showed up in 20.0a1/20121210 and is currently #3 top crasher in this build. The regression range is
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e83d0987a31&tochange=725eb8792d27
It might be a regression from bug 787134.

Signature 	nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, wchar_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsIDocShell**, nsIRequest**) More Reports Search
UUID	97a55bed-822c-4692-91e7-3fd9a2121210
Date Processed	2012-12-10 18:31:44
Uptime	823
Last Crash	9.1 weeks before submission
Install Age	2.3 hours since version was first installed.
Install Time	2012-12-10 16:16:34
Product	Firefox
Version	20.0a1
Build ID	20121210030747
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 16 model 4 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x40
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x9460, AdapterSubsysID: 22811787, AdapterDriverVersion: 8.961.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ WebGL? EGL? EGL+ GL Context? GL Context+ WebGL+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x9460
Total Virtual Memory	4294836224
Available Virtual Memory	3486474240
System Memory Use Percentage	41
Available Page File	6122147840
Available Physical Memory	2529824768

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsDocShell::InternalLoad 	docshell/base/nsDocShell.cpp:9003
1 	xul.dll 	nsDocShell::LoadHistoryEntry 	docshell/base/nsDocShell.cpp:10609
2 	xul.dll 	nsDocShell::LoadURI 	docshell/base/nsDocShell.cpp:1398
3 	xul.dll 	nsSHistory::InitiateLoad 	docshell/shistory/src/nsSHistory.cpp:1751
4 	xul.dll 	nsSHistory::LoadEntry 	docshell/shistory/src/nsSHistory.cpp:1618
5 	xul.dll 	nsSHistory::GoBack 	docshell/shistory/src/nsSHistory.cpp:839
6 	xul.dll 	nsDocShell::GoBack 	docshell/base/nsDocShell.cpp:3942
7 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
8 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1488
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:389
10 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2341
11 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:338
12 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:404
13 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:437
14 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5786
15 	xul.dll 	mozilla::dom::EventHandlerNonNull::Call 	obj-firefox/dom/bindings/EventHandlerBinding.cpp:46
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsDocShell%3A%3AInternalLoad%28nsIURI*%2C+nsIURI*%2C+nsISupports*%2C+unsigned+int%2C+wchar_t+const*%2C+char+const*%2C+nsAString_internal+const%26%2C+nsIInputStream*%2C+nsIInputStream*%2C+unsigned+int%2C+nsISHEntry*%2C+bool%2C+nsIDocShell**%2C+nsIRequest**%29

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a null-deref on this line:

  9003      mTiming->NotifyUnloadAccepted(mCurrentURI);

The actual code is:

9002 if (timeBeforeUnload) {
9003   mTiming->NotifyUnloadAccepted(mCurrentURI);
9004 } 

and the if condition just got changed in bug 818559.  It used to null-check mTiming.

I believe the new code is wrong: there's a call into arbitrary script between when timeBeforeUnload is set and when it's checked there, so mTiming can well be null at this point...

Scoobidiver, thank you for filing these crash regressions!

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Grr. Why did I miss that.

Comment 3

[:Benjamin Peterson]

Attachment: null-check again

I didn't realize scripts could be so evil.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Sorry, I backed out Bug 818559 already.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 690494 [details] [diff] [review]
null-check again

Scripts can be arbitrarily evil.  ;)

r=me

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

r+ from me too :)

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/mozilla-central/rev/ab7c54f6472f

Comment 8

[Paul Silaghi, QA [:pauly]]

Checking the crashstats, I see 8 crashes on FF 21 and 5 on FF 20. Any thoughts?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Paul, are the crashes happening at the same place what this bug is about?
Could you provide links to the crash-stats you're looking at?

Comment 10

[Paul Silaghi, QA [:pauly]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Paul, are the crashes happening at the same place what this bug is about?
They are on different lines, but it's the same signature.

> Could you provide links to the crash-stats you're looking at?
Since my last comment, another 16 crashes appeared on FF 20b2:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=1&range_unit=weeks&hang_type=any&process_type=any&signature=nsDocShell%3A%3AInternalLoad%28nsIURI%2A%2C%20nsIURI%2A%2C%20nsISupports%2A%2C%20unsigned%20int%2C%20wchar_t%20const%2A%2C%20char%20const%2A%2C%20nsAString_internal%20const%26%2C%20nsIInputStream%2A%2C%20nsIInputStream%2A%2C%20unsigned%20int%2C%20nsISHEntry%2A%2C%20bool%2C%20nsIDocShell%2A%2A%2C%20nsIRequest%2A%2A%29

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Those all look like different bugs than this.

Comment 12

[Paul Silaghi, QA [:pauly]]

Verified fixed based on comment 11. Continue tracking this signature in bug 854864.