1330765 - crash near null [@mozilla::a11y::DocManager::RemoveListeners]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

There seems to be a timing aspect to this bug. I have put a call to setTimeout() in the test case with a random timeout in it to help. The test case calls window.location.reload(true) when complete. I have only tested with with e10s disabled.

Requires fuzzPriv extension to reproduce:
https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension

==24189==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000308 (pc 0x7f9c9785f287 bp 0x7ffc7539fe60 sp 0x7ffc7539fd00 T0)
    #0 0x7f9c9785f286 in GetWindow /home/worker/workspace/build/src/obj-firefox/dist/include/nsIDocument.h:1293:12
    #1 0x7f9c9785f286 in mozilla::a11y::DocManager::RemoveListeners(nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:454
    #2 0x7f9c9785f04b in mozilla::a11y::DocManager::NotifyOfDocumentShutdown(mozilla::a11y::DocAccessible*, nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:108:3
    #3 0x7f9c979066f3 in mozilla::a11y::DocAccessible::Shutdown() /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:483:3
    #4 0x7f9c978606c2 in mozilla::a11y::DocManager::ClearDocCache() /home/worker/workspace/build/src/accessible/base/DocManager.cpp:580:7
    #5 0x7f9c9786047f in mozilla::a11y::DocManager::Shutdown() /home/worker/workspace/build/src/accessible/base/DocManager.cpp:220:3
    #6 0x7f9c978ac5e0 in nsAccessibilityService::Shutdown() /home/worker/workspace/build/src/accessible/base/nsAccessibilityService.cpp:1336:3
    #7 0x7f9c9785f12c in RemoveFromXPCDocumentCache /home/worker/workspace/build/src/accessible/base/DocManager.cpp:98:5
    #8 0x7f9c9785f12c in mozilla::a11y::DocManager::NotifyOfDocumentShutdown(mozilla::a11y::DocAccessible*, nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:116
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Eitan Isaacson [:eeejay]]

I think I may be reproducing this locally. Looks like DocAccessible::Shutdown() is re-entered after mDocumentNode is already set to null.

#0  0x00007fffeac60452 in mozilla::a11y::DocManager::RemoveListeners(nsIDocument*) (this=0x7fffb5b716c0, aDocument=0x0) at /accessible/base/DocManager.cpp:454
#1  0x00007fffeac603bc in mozilla::a11y::DocManager::NotifyOfDocumentShutdown(mozilla::a11y::DocAccessible*, nsIDocument*) (this=0x7fffb5b716c0, aDocument=0x7fffb57f3ad0, aDOMDocument=0x0)
    at /accessible/base/DocManager.cpp:108
#2  0x00007fffeac8feab in mozilla::a11y::DocAccessible::Shutdown() (this=0x7fffb57f3ad0)
    at /accessible/generic/DocAccessible.cpp:500
#3  0x00007fffeac60a51 in mozilla::a11y::DocManager::ClearDocCache() (this=0x7fffb5b716c0)
    at /accessible/base/DocManager.cpp:552
#4  0x00007fffeac609f2 in mozilla::a11y::DocManager::Shutdown() (this=0x7fffb5b716c0)
    at /accessible/base/DocManager.cpp:220
#5  0x00007fffeac773e2 in nsAccessibilityService::Shutdown() (this=0x7fffb5b716c0)
    at /accessible/base/nsAccessibilityService.cpp:1328
#6  0x00007fffeac7c3e8 in MaybeShutdownAccService(unsigned int) (aFormerConsumer=<optimized out>)
    at /accessible/base/nsAccessibilityService.cpp:1826
#7  0x00007fffeac60418 in mozilla::a11y::DocManager::RemoveFromXPCDocumentCache(mozilla::a11y::DocAccessible*) (this=0x7fffb5b716c0, aDocument=<optimized out>)
    at /accessible/base/DocManager.cpp:98
#8  0x00007fffeac60418 in mozilla::a11y::DocManager::NotifyOfDocumentShutdown(mozilla::a11y::DocAccessible*, nsIDocument*) (this=0x7fffb5b716c0, aDocument=0x7fffb3771c00, aDOMDocument=0x7fffb2fc1800)
    at /accessible/base/DocManager.cpp:116
#9  0x00007fffeac8feab in mozilla::a11y::DocAccessible::Shutdown() (this=0x7fffb3771c00)
    at /accessible/generic/DocAccessible.cpp:500
#10 0x00007fffeac8fced in mozilla::a11y::DocAccessible::Shutdown() (this=0x7fffb57f3ad0)
    at /accessible/generic/DocAccessible.cpp:465
#11 0x00007fffeac61692 in mozilla::a11y::DocManager::HandleEvent(nsIDOMEvent*) (this=0x7fffb5b716c0, aEvent=<optimized out>) at /accessible/base/DocManager.cpp:386
#12 0x00007fffeac6180d in non-virtual thunk to mozilla::a11y::DocManager::HandleEvent(nsIDOMEvent*) ()
    at /objdir-linux/dist/bin/libxul.so
#13 0x00007fffe90bd1de in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (this=0x7fffc786dc10, aListener=<optimized out>, aDOMEvent=0x7fffb34abe70, aCurrentTarget=0x7fffc7753d80)
    at /dom/events/EventListenerManager.cpp:1136
#14 0x00007fffe90bdc52 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x7fffc786dc10, 
    aPresContext=0x0, aEvent=
    0x7fffb3482d40, aDOMEvent=0x7fffffffbd30, aCurrentTarget=0x7fffc7753d80, aEventStatus=0x7fffffffbd38) at /dom/events/EventListenerManager.cpp:1310
#15 0x00007fffe90b346b in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=..., aVisitor=..., aCallback=<optimized out>, aCd=...)
    at /dom/events/EventDispatcher.cpp:442
#16 0x00007fffe90b4346 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=<optimized out>, aPresContext=<optimized out>, aEvent=<optimized out>, aDOMEvent=0x7fffb34abe70, aEventStatus=<optimized out>, aCallback=0x0, aTargets=<optimized out>)
    at /dom/events/EventDispatcher.cpp:825
#17 0x00007fffe90a6407 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (aTarget=0x7fffc77a1800, aEvent=<optimized out>, aDOMEvent=0x7fffb34abe70, aPresContext=0x0, aEventStatus=0x0)
    at /dom/events/EventDispatcher.cpp:891
#18 0x00007fffe85c421e in nsDocument::DispatchPageTransition(mozilla::dom::EventTarget*, nsAString const&, bool) (this=0x7fffc7745000, aDispatchTarget=0x7fffc77a1800, aType=..., aPersisted=<optimized out>)
---Type <return> to continue, or q <return> to quit---
    at /dom/base/nsDocument.cpp:8945
#19 0x00007fffe85c421e in nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*) (this=0x7fffc7745000, aPersisted=false, aDispatchStartTarget=<optimized out>)
    at /dom/base/nsDocument.cpp:9106
#20 0x00007fffe9b3339b in nsDocumentViewer::PageHide(bool) (this=0x7fffc77049c0, aIsUnload=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /layout/base/nsDocumentViewer.cpp:1408
#21 0x00007fffeac02cad in nsDocShell::FirePageHideNotificationInternal(bool, bool) (this=
    0x7fffc77a0000, aIsUnload=<error reading variable: access outside bounds of object referenced via synthetic pointer>, aSkipCheckingDynEntries=<error reading variable: access outside bounds of object referenced via synthetic pointer>) at /docshell/base/nsDocShell.cpp:1708
#22 0x00007fffeabfc11a in nsDocShell::FirePageHideNotification(bool) (this=0x7fffc77a0000, aIsUnload=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /docshell/base/nsDocShell.cpp:1690
#23 0x00007fffeabfc11a in nsDocShell::Destroy() (this=0x7fffc77a0000)
    at /docshell/base/nsDocShell.cpp:5847
#24 0x00007fffeac10a30 in non-virtual thunk to nsDocShell::Destroy() ()
    at /objdir-linux/dist/include/mozilla/Vector.h:937
#25 0x00007fffeac44e67 in nsXULWindow::Destroy() (this=0x7fffc7703220)
    at /xpfe/appshell/nsXULWindow.cpp:499
#26 0x00007fffeac39be5 in nsWebShellWindow::Destroy() (this=0x7fffc7703220)
    at /xpfe/appshell/nsWebShellWindow.cpp:753
#27 0x00007fffe84da331 in nsGlobalWindow::ReallyCloseWindow() (this=0x7fffc77a1800)
    at /dom/base/nsGlobalWindow.cpp:9480
#28 0x00007fffe84ed393 in nsCloseEvent::Run() (this=<optimized out>)
    at /dom/base/nsGlobalWindow.cpp:9236
#29 0x00007fffe75869b8 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffde915060, aMayWait=<optimized out>, aResult=0x7fffffffc2f7) at /xpcom/threads/nsThread.cpp:1421
#30 0x00007fffe758ab5f in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffb5b716c0, aMayWait=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /xpcom/threads/nsThreadUtils.cpp:483
#31 0x00007fffe7a98a68 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7fffde936dc0, aDelegate=0x7ffff6b72380) at /ipc/glue/MessagePump.cpp:96
#32 0x00007fffe7a34eeb in MessageLoop::RunInternal() (this=0x7fffb5b71750)
    at /ipc/chromium/src/base/message_loop.cc:320
#33 0x00007fffe7a34eeb in MessageLoop::RunHandler() (this=<optimized out>)
    at /ipc/chromium/src/base/message_loop.cc:313
#34 0x00007fffe7a34eeb in MessageLoop::Run() (this=0x7fffb5b71750)
    at /ipc/chromium/src/base/message_loop.cc:293
#35 0x00007fffe98bfdf9 in nsBaseAppShell::Run() (this=0x7fffd7fbb560)
    at /widget/nsBaseAppShell.cpp:156
#36 0x00007fffeaef102e in nsAppStartup::Run() (this=0x7fffd7fe1c40)
    at /toolkit/components/startup/nsAppStartup.cpp:283
#37 0x00007fffeaf818a7 in XREMain::XRE_mainRun() (this=<optimized out>)
    at /toolkit/xre/nsAppRunner.cpp:4590
#38 0x00007fffeaf820e9 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7fffffffc590, argc=<optimized out>, argv=<optimized out>, aConfig=...)
    at /toolkit/xre/nsAppRunner.cpp:4774
#39 0x00007fffeaf8269c in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=0, argv=0x0, aConfig=...) at /toolkit/xre/nsAppRunner.cpp:4869
#40 0x0000000000405a8e in do_main(int, char**, char**) (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /browser/app/nsBrowserApp.cpp:237

Comment 3

[alexander :surkov (:asurkov)]

maybe we need to rethink MaybeShutdownAccService strategy (also see bug 1371511)

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Remove document from accessible cache before xpc doc cache.

Here is a potential fix. Remove the doc from mDocAccessibleCache before mXPCDocumentCache, because the latter can shutdown the service and re-enter into the doc's shutdown method.

Comment 5

[Eitan Isaacson [:eeejay]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aee8572fdc2639fbd74d9cc5ce9ec8bfe47b39f

Comment 6

[Eitan Isaacson [:eeejay]]

OK, I have a bit more clarity as to what is happening here.

Both the document+subdocument and the last reference to the nsIAccessibleRetrieval service are being dropped at the same time.

This causes a race condition where the mXPCDocumentCache is being emptied AND xpcAccessibilityService is scheduled for a delayed shutdown. If the service schedules a shutdown (due to last external ref being released), AND the xpc doc cache is emptied BEFORE the shutdown is executed, we end up with a doc's Shutdown method being erroneously reentered.

Patch coming up.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Only attempt to shutdown a11y if last XPC doc was just removed. r?surkov

Comment 8

[Eitan Isaacson [:eeejay]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0895a6a488ba45b5cdf001458d7fca68cc0dc76c

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8880114 [details] [diff] [review]
Only attempt to shutdown a11y if last XPC doc was just removed. r?surkov

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

::: accessible/base/DocManager.cpp
@@ +95,4 @@
>  
> +    if (!HasXPCDocuments()) {
> +      MaybeShutdownAccService(nsAccessibilityService::eXPCOM);
> +    }

the change is reasonable - it doesn't make sense to shutdown a11y because xpcom goes down, if it never was up, but I'm curious if it fixes the crash if an xpcom document exists

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8880114 [details] [diff] [review]
Only attempt to shutdown a11y if last XPC doc was just removed. r?surkov

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

I grant r+, since the patch makes sense, however I'm not yet sure if that's a real fix of the problem

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f76c34241217
Only attempt to shutdown a11y if last XPC doc was just removed. r=surkov

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 14

[Eitan Isaacson [:eeejay]]

Comment on attachment 8880114 [details] [diff] [review]
Only attempt to shutdown a11y if last XPC doc was just removed. r?surkov

Approval Request Comment
[Feature/Bug causing the regression]: 1330765
[User impact if declined]: A race condition when both accessibility is shutdown along with nested documents closing.
[Is this code covered by automated tests?]: No, but as we transition to browser accessibility tests, we will be encountering this race more often.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is a straightforward change.
[String changes made/needed]: None.

Comment 15

[Liz Henry (:lizzard)]

Comment on attachment 8880114 [details] [diff] [review]
Only attempt to shutdown a11y if last XPC doc was just removed. r?surkov

Crash fix, sound like it may avoid shutdown issues, let's bring this to beta 5.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ab067d477d2b