1184217 - crash in mozilla::a11y::DocAccessibleParent::Unbind()

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[alex_mayorga]

This bug was filed from the Socorro interface and is 
report bp-677205bf-91da-40d7-aca1-3407d2150712.
=============================================================

https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3AUnbind%28%29

This has gone up 11 ranks ans is now top crasher 14 for 42.

It seems to be Windows only.

Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::a11y::DocAccessibleParent::Unbind() 	accessible/ipc/DocAccessibleParent.h
1 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
2 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
3 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
4 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
5 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
6 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
7 	xul.dll 	mozilla::a11y::ProxyAccessible::Shutdown() 	accessible/ipc/ProxyAccessible.cpp
8 	xul.dll 	mozilla::a11y::DocAccessibleParent::RecvHideEvent(unsigned __int64 const&) 	accessible/ipc/DocAccessibleParent.cpp
9 	xul.dll 	mozilla::a11y::PDocAccessibleParent::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PDocAccessibleParent.cpp
10 	xul.dll 	mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PContentParent.cpp
11 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
12 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
13 	xul.dll 	RunnableMethod<mozilla::ipc::MessageChannel, bool ( mozilla::ipc::MessageChannel::*)(void), Tuple0>::Run() 	ipc/chromium/src/base/task.h
14 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
15 	xul.dll 	mozilla::ipc::DoWorkRunnable::Run() 	ipc/glue/MessagePump.cpp
16 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
17 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
18 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
19 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
20 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
21 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
22 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
23 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
24 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
25 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
26 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
27 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
28 	firefox.exe 	NS_internal_main(int, char**) 	browser/app/nsBrowserApp.cpp
29 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
30 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
31 	kernel32.dll 	BaseThreadInitThunk 	
32 	ntdll.dll 	RtlUserThreadStart 	
33 	kernel32.dll 	BasepReportFault 	
34 	kernel32.dll 	BasepReportFault

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

URLs associated with these reports all seem to be mail.yahoo.com related

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Sorry not quite all, here is an example of an odd one out: http://in.bookmyshow.com/movies/minions-3d/ET00025778 (seems to show the page, the put up an in-content "Select Your City" dialog)

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: check consistance of the document tree before removing accessibles in a hide event

This is just a diagnostic patch, if the problem is that the document tree is inconsistant we should crash earlier, and if its something else we won't

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/909610f2dfc2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/909610f2dfc2

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Looking up CheckDocTree in crash stats I only see two:
https://crash-stats.mozilla.com/report/index/a6c18852-3f3d-4c2a-8e46-e3cac2150719
https://crash-stats.mozilla.com/report/index/8a1824aa-b114-4e2c-bd89-358fe2150719

(interesting similar endings on those URLs but they are different stacks)

I don't see any Unbind stacks since build id: 20150715095506 (since the diagnostic patch landed).

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

OK we do have stacks: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3AUnbind%28%29#tab-reports

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

For these Unbind stacks it looks we fail here:
ParentDoc()->mChildDocs.RemoveElement(this);

We are deeply nested in mozilla::a11y::ProxyAccessible::Shutdown() calls.

Comment 10

[Trevor Saunders (:tbsaunde)]

So presumably MparentDoc is null.  I can't easily prove that, but it seems like a good guess.  That seems broken given the ProxyAccessible::Shutdown() calling Unbind() is for a parent.

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: make CheckDocTree check the entire document tree not just the subtree

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8652419 [details] [diff] [review]
make CheckDocTree check the entire document tree not just the subtree

Review of attachment 8652419 [details] [diff] [review]:
-----------------------------------------------------------------

Hmmm. I sort of want you to differentiate the !doc->mTopLevel case. Can you put that check in a auxiliary method and crash there directly (so crash stats is easier to understand)?

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d82f63f06e7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=13289529&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2629e3e4b0c

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: make CheckDocTree check the entire document tree not just the subtree r=davidb[

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8653000 [details] [diff] [review]
make CheckDocTree check the entire document tree not just the subtree r=davidb[

Review of attachment 8653000 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/DocAccessibleParent.h
@@ +73,5 @@
>    virtual void ActorDestroy(ActorDestroyReason aWhy) override
>    {
> +    if (mShutdown) {
> +      return;
> +    }

Does this belong in a separate patch/bug?

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #16)
> Comment on attachment 8653000 [details] [diff] [review]
> make CheckDocTree check the entire document tree not just the subtree
> r=davidb[
> 
> Review of attachment 8653000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/ipc/DocAccessibleParent.h
> @@ +73,5 @@
> >    virtual void ActorDestroy(ActorDestroyReason aWhy) override
> >    {
> > +    if (mShutdown) {
> > +      return;
> > +    }
> 
> Does this belong in a separate patch/bug?
noo, if you try and check a shutdown non top level doc you crash because it has no parent doc, and its not top level.  However that's ok in the special case of being shut down.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18151dec259d
make CheckDocTree check the entire document tree not just the subtree r=davidb

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18151dec259d

Comment 20

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

We may need to back this out from nightly, if it caused the crash in bug 1329644.

Comment 21

[Carsten Book [:Tomcat]]

backed out in e68cbc3b5b3d from central

Comment 22

[Carsten Book [:Tomcat]]

nightlys retriggered too and backout merged also also to the integration trees

Comment 23

[alexander :surkov (:asurkov)]

seems fixed