Closed Bug 1261012 Opened 4 years ago Closed 4 years ago

startup crash in nsDocument::OnPageShow spiking in Firefox 45

Categories

(Core :: General, defect, critical)

45 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 - wontfix
firefox46 + fixed
firefox49 --- fixed

People

(Reporter: philipp, Assigned: m_kato)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-2642ec1f-cc5c-4f99-af23-237272160325.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsDocument::OnPageShow(bool, mozilla::dom::EventTarget*) 	dom/base/nsDocument.cpp
1 	xul.dll 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/glue/nsCOMPtr.cpp
2 	xul.dll 	xul.dll@0x24b02ff 	
3 	xul.dll 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) 	docshell/base/nsDocShell.cpp
4 	xul.dll 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) 	docshell/base/nsDocShell.cpp
5 	xul.dll 	nsGlobalWindow::QueryInterface(nsID const&, void**) 	dom/base/nsGlobalWindow.cpp
6 	xul.dll 	nsDocShell::QueryInterface(nsID const&, void**) 	docshell/base/nsDocShell.cpp
7 	xul.dll 	nsQueryReferent::operator()(nsID const&, void**) 	xpcom/glue/nsWeakReference.cpp
8 	xul.dll 	nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) 	uriloader/base/nsDocLoader.cpp
9 	xul.dll 	nsDocLoader::DocLoaderIsEmpty(bool) 	uriloader/base/nsDocLoader.cpp
10 	xul.dll 	nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	uriloader/base/nsDocLoader.cpp
11 	xul.dll 	nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) 	netwerk/base/nsLoadGroup.cpp
12 	xul.dll 	nsLoadGroup::Cancel(nsresult) 	netwerk/base/nsLoadGroup.cpp
13 	xul.dll 	nsDocLoader::Stop() 	uriloader/base/nsDocLoader.cpp
14 	xul.dll 	nsDocLoader::Stop() 	uriloader/base/nsDocLoader.cpp
15 	xul.dll 	nsDocLoader::Stop() 	uriloader/base/nsDocLoader.cpp
16 	xul.dll 	nsDocLoader::Stop() 	uriloader/base/nsDocLoader.cpp
17 	xul.dll 	nsDocLoader::Destroy() 	uriloader/base/nsDocLoader.cpp
18 	xul.dll 	nsDocLoader::~nsDocLoader() 	uriloader/base/nsDocLoader.cpp
19 	xul.dll 	nsDocLoader::`scalar deleting destructor'(unsigned int) 	
20 	xul.dll 	nsDocShell::Release() 	docshell/base/nsDocShell.cpp
21 	xul.dll 	nsComponentManagerImpl::FreeServices() 	xpcom/components/nsComponentManager.cpp
22 	xul.dll 	ScopedXPCOMStartup::~ScopedXPCOMStartup() 	toolkit/xre/nsAppRunner.cpp
23 	xul.dll 	mozilla::UniquePtr<ScopedXPCOMStartup, mozilla::DefaultDelete<ScopedXPCOMStartup> >::reset(ScopedXPCOMStartup*) 	mfbt/UniquePtr.h
24 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
25 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp

crashes with this signature are increasing in frequency in the 45 release and it's a startup crash 80% of the time.

correlations for this signature seem to implicate the amazon assistant addon from https://addons.mozilla.org/firefox/addon/amazon-browser-bar/ and various language packs:

nsDocument::OnPageShow|EXCEPTION_ACCESS_VIOLATION_READ (104 crashes)
     97% (101/104) vs.   1% (447/84733) abb@amazon.com
     33% (34/104) vs.   0% (157/84733) langpack-en-US@firefox.mozilla.org
     23% (24/104) vs.   3% (2267/84733) {4ED1F68A-5463-4931-9384-8FFF5ED91D92}
     16% (17/104) vs.   0% (113/84733) langpack-de@firefox.mozilla.org
     13% (14/104) vs.   0% (231/84733) jetpack-extension@dashlane.com
     12% (12/104) vs.   0% (99/84733) langpack-fr@firefox.mozilla.org
     11% (11/104) vs.   0% (44/84733) langpack-pl@firefox.mozilla.org
     10% (10/104) vs.   0% (46/84733) langpack-tr@firefox.mozilla.org
     10% (10/104) vs.   0% (57/84733) langpack-pt-BR@firefox.mozilla.org (pt-BR Language Pack, https://addons.mozilla.org/addon/4851)
      9% (9/104) vs.   0% (25/84733) langpack-lt@firefox.mozilla.org

many user comments seem to link the issue to the 45 update and a number of them is also mentioning that their homepage gets reset to yahoo/bing/ask
Is this really startup?  Although I investigate some signatures, all is during shutdown.  bp-2642ec1f-cc5c-4f99-af23-237272160325 is same.

But this is simple null reference crash.
Assignee: nobody → m_kato
During shutdown, mozilla::services::GetObserverServie will return nullptr.  So we should check it.

Review commit: https://reviewboard.mozilla.org/r/46315/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46315/
Attachment #8741231 - Flags: review?(bugs)
...i was just dubbing it as startup crash, because uptime stats for this crash say it's happening under 60 secs after the launch of firefox 75% of the time: https://crash-stats.mozilla.com/signature/?date=%3E%3D2016-01-01&signature=nsDocument%3A%3AOnPageShow
Comment on attachment 8741231 [details]
MozReview Request: Bug 1261012 - Check whether nsIObserverService is nullptr. r?smaug

https://reviewboard.mozilla.org/r/46315/#review43171
Attachment #8741231 - Flags: review?(bugs) → review+
But, looks like http://hg.mozilla.org/mozilla-central/rev/c0a30d9af35c added couple of other cases when return value is not checked. Want to add null checks to those too? r+ with or without those other null checks, but would be good to get the code right elsewhere too, and it is just simple null check.
Blocks: 843910
to be precise, I'm talking about similar code in nsDocument::DispatchContentLoadedEvents, nsDocument::OnPageHide and in nsDocumentViewer::LoadComplete.
Of which nsDocument::OnPageHide got the null check already in http://hg.mozilla.org/mozilla-central/rev/4d2b72314d24
so nsDocument::DispatchContentLoadedEvents and nsDocumentViewer::LoadComplete
[Tracking Requested - why for this release]:
Too late for 45.
ni to liz so that she is aware
We're heading into the RC build for 46 today, but if we feel confident about a fix we could still do another build before 46 is released.   But, I'm not sure we will be able to confirm the fix since there aren't many crashes on 46 now. smaug and Makoto, let me know what you think. 

Tracking to keep an eye on this during the release.
Flags: needinfo?(m_kato)
Flags: needinfo?(lhenry)
Flags: needinfo?(bugs)
The fix is super safe null check, so I don't see reasons to not take it.
And no need to add those other null checks I mentioned in this bug, just land the patch I reviewed.
Flags: needinfo?(bugs)
Ok, can you request uplift then?
Wes can you land this on m-b and m-r please? Thanks.
Flags: needinfo?(m_kato) → needinfo?(wkocher)
Makoto, should don't we merge that 47 & 48 too? Thanks
Flags: needinfo?(m_kato)
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Makoto, should don't we merge that 47 & 48 too? Thanks

I will update fix to add nullptr check per comment #5.
Flags: needinfo?(m_kato)
Comment on attachment 8741231 [details]
MozReview Request: Bug 1261012 - Check whether nsIObserverService is nullptr. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46315/diff/1-2/
Comment on attachment 8741231 [details]
MozReview Request: Bug 1261012 - Check whether nsIObserverService is nullptr. r?smaug

could you review again?  I add some nullptr check.

(after r+, I cannot clear review status by reviewboard :-<.  This seems to be reviewboard bug....)
Attachment #8741231 - Flags: review+ → review?(bugs)
Comment on attachment 8741231 [details]
MozReview Request: Bug 1261012 - Check whether nsIObserverService is nullptr. r?smaug

https://reviewboard.mozilla.org/r/46315/#review46669
Attachment #8741231 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/0fd722ed257c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.