Closed Bug 1011736 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

32 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 --- affected

People

(Reporter: jbecerra, Assigned: jwei)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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
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.
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
Crash Signature: [@ mozilla::a11y::Accessible::Description(nsString&)] → [@ mozilla::a11y::Accessible::Description(nsString&)] [@ mozilla::a11y::Accessible::Shutdown()]
Duplicate of this bug: 1011735
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.
(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?
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)
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.
Blocks: 949518
Summary: crash in mozilla::a11y::Accessible::Description(nsString&) → crash in mozilla::a11y::Accessible::Shutdown()
Uses the quick fix (null check) since it's localised to this call.
Assignee: nobody → jwei
Attachment #8425945 - Flags: review?(trev.saunders)
(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.
Attachment #8425945 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/1475788f1869
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.