1011736 - crash in mozilla::a11y::Accessible::Shutdown()

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[juan becerra [:juanb]]

This bug was filed from the Socorro interface and is 
report bp-ad7bc7e0-2db1-4da3-b9c4-15e692140509.
=============================================================

This is a new signature on nightly 32a1, starting around 5/8. It's mostly dupes. Mostly on Windows 8.1. No comments in the bug reports.


0 	xul.dll 	mozilla::a11y::Accessible::Description(nsString &) 	
1 	xul.dll 	mozilla::a11y::HTMLImageMapAccessible::Release() 	
2 	xul.dll 	nsTHashtable<nsBaseHashtableET<nsISupportsHashKey,mozilla::RestyleTracker::RestyleData> >::s_HashKey(PLDHashTable *,void const *) 	
3 	xul.dll 	nsXPCWrappedJS::GetNewOrUsed(JS::Handle<JSObject *>,nsID const &,nsXPCWrappedJS * *) 	
4 	xul.dll 	mozilla::dom::HTMLMetaElement::HTMLMetaElement(already_AddRefed<nsINodeInfo> &) 	
5 	xul.dll 	mozilla::dom::HTMLMetaElement::HTMLMetaElement(already_AddRefed<nsINodeInfo> &) 	
6 	xul.dll 	mozilla::Omnijar::InitOne(nsIFile *,mozilla::Omnijar::Type) 	
7 	xul.dll 	mozilla::RefreshDriverTimer::SetRate(double) 	
8 	xul.dll 	mozilla::net::SpdyStream3::ParseHttpRequestHeaders(char const *,unsigned int,unsigned int *) 	
9 	xul.dll 	mozilla::net::SpdyStream3::ParseHttpRequestHeaders(char const *,unsigned int,unsigned int *) 	
10 	xul.dll 	[thunk]:mozilla::css::MediaRule::QueryInterface`adjustor{24}' (nsID const &,void * *) 	
11 	xul.dll 	mozilla::Omnijar::InitOne(nsIFile *,mozilla::Omnijar::Type) 	
12 	xul.dll 	mozilla::dom::Selection::GetFocusNode() 	
13 	xul.dll 	mozilla::places::AsyncStatementTelemetryTimer::HandleCompletion(unsigned short) 	
14 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
15 	firefox.exe 	NS_internal_main(int,char * *) 	browser/app/nsBrowserApp.cpp
16 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
17 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt_bld/self_x86/crt/src/crtexe.c:552
18 	kernel32.dll 	kernel32.dll@0x1919f 	
19 	ntdll.dll 	ntdll.dll@0x4a8cb 	
20 	ntdll.dll 	ntdll.dll@0x4a8a1

Comment 1

[Trevor Saunders (:tbsaunde)]

I just took a quick look, but after the first frame the rest is almost certainly garbage :(  Its possible there's something wrong with Accessible::Description or someone new is calling it, but that seems kind of unlikely I don't remember anything related changing recently.

Comment 2

[(Away)]

There was a symbols problem last week; below is the true stack for comment 0. SelectionMgr() was null here:

xul!mozilla::a11y::Accessible::Shutdown
xul!mozilla::a11y::Accessible::LastRelease
xul!mozilla::a11y::Accessible::DeleteCycleCollectable
xul!mozilla::a11y::Accessible::cycleCollection::DeleteCycleCollectable
xul!SnowWhiteKiller::~SnowWhiteKiller
xul!nsCycleCollector::FreeSnowWhite
xul!nsCycleCollector::Shutdown
xul!nsCycleCollector_shutdown
xul!mozilla::ShutdownXPCOM
xul!ScopedXPCOMStartup::~ScopedXPCOMStartup
xul!ProfileLockedDialog
xul!ProfileLockedDialog
xul!SelectProfile
xul!XREMain::XRE_mainStartup

Comment 4

[Trevor Saunders (:tbsaunde)]

Ok, so this is simple enough.  when we shut down gAccessibilityService can go away before the last accessible does.

I can see three solutions
- leak the service intentionally
- make accessibles keep the service alive
- null check SelectionMgr() here and hope it isn't an issue elsewhere.

thoughts?  Assuming it works as I think it should I think I prefer leaking the service because its simple and doesn't have a runtime cost.

Comment 5

[(Away)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> thoughts?  Assuming it works as I think it should I think I prefer leaking
> the service because its simple and doesn't have a runtime cost.

Isn't there some tool that complains about shutdown leaks?

Comment 6

[(Away)]

Actually there may be a more specific regression here. These started showing up on Nightly on May 2nd, and have had about a dozen reports a day since then, except for the 8th to 13th when the messed up symbols sent those reports to a different signature. And they all seem to have the ProfileLockedDialog in common. Did something change recently in the profile-locked path or in the Accessible code?

https://crash-stats.mozilla.com/report/list?product=Firefox&query_type=contains&range_unit=days&process_type=any&hang_type=any&signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3AShutdown%28%29&date=2014-05-20+23%3A00%3A00&range_value=28#tab-reports

(I'm going to ignore the two crashes from 28.0 as noise since the stacks are pretty different)

Comment 7

[(Away)]

So it looks like that call to SelectionMgr() is new code from bug 949518, which lines up perfectly with the crash dates starting May 2.

The option of a one-off null check may not be so bad in that light. The other callers are probably ok since they're not coming up on the crash team's radar.

Comment 8

[Jonathan Wei [:jwei]]

Attachment: Added null check to SelectionMgr() method call

Uses the quick fix (null check) since it's localised to this call.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to David Major [:dmajor] (UTC+12) from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > thoughts?  Assuming it works as I think it should I think I prefer leaking
> > the service because its simple and doesn't have a runtime cost.
> 
> Isn't there some tool that complains about shutdown leaks?

if you use the standard refcounting macros yes, but if you don't do that and just define AddRef to return 2 and Release 1 then that's fine (we have various things in the tree that do that)


(In reply to Jonathan Wei [:jwei] from comment #8)
> Created attachment 8425945 [details] [diff] [review]
> Added null check to SelectionMgr() method call
> 
> Uses the quick fix (null check) since it's localised to this call.

I'd prefer not doing this I think, but I guess you may be low on top and I guess it doesn't really cause a problem.

Comment 10

[Jonathan Wei [:jwei]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=604e285323e3
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1475788f1869

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1475788f1869

Comment 12

[Bogdan Maris, Desktop Test Engineering]

No crashes are recorded by Socorro in the last 2 weeks on Firefox 34.0a1, 33.0a2, 32.0b with signature @mozilla::a11y::Accessible::Description(nsString&)
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A32.0b&platform=win&platform=mac&platform=lin&range_value=2&range_unit=weeks&date=07%2F30%2F2014+06%3A00%3A00&query_search=signature&query_type=contains&query=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3ADescription%28nsString%26%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

No crashes are recorded by Socorro in the last 2 weeks on Firefox 34.0a1, 33.0a2, 32.0b with signature @mozilla::a11y::Accessible::Shutdown()
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A32.0b&platform=win&platform=mac&platform=lin&range_value=2&range_unit=weeks&date=07%2F30%2F2014+06%3A00%3A00&query_search=signature&query_type=contains&query=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3AShutdown&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Based on this results I`m marking this as verified fixed.