1396267 - crash near null in [@ mozilla::a11y::IDRefsIterator::IDRefsIterator]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Found with m-c:
BuildID=20170901232924
SourceStamp=5278dfcf5eb9f58eaf06ad1ce67e7fd4aba34772

==24590==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x7f4d35009c12 bp 0x7ffc4da9bdb0 sp 0x7ffc4da9bdb0 T0)
==24590==The signal is caused by a READ memory access.
==24590==Hint: address points to the zero page.
    #0 0x7f4d35009c11 in GetBoolFlag src/dom/base/nsINode.h:1624:12
    #1 0x7f4d35009c11 in IsInUncomposedDoc src/dom/base/nsINode.h:556
    #2 0x7f4d35009c11 in mozilla::a11y::IDRefsIterator::IDRefsIterator(mozilla::a11y::DocAccessible*, nsIContent*, nsIAtom*) src/accessible/base/AccIterator.cpp:260
    #3 0x7f4d350bd6e8 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2081:18
    #4 0x7f4d3502b59c in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:813:18
    #5 0x7f4d31e02b8c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1886:12
    #6 0x7f4d31e11c0b in TickDriver src/layout/base/nsRefreshDriver.cpp:337:13
    #7 0x7f4d31e11c0b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:307
    #8 0x7f4d31e118f4 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:328:5
    #9 0x7f4d31e13f1b in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:770:5
    #10 0x7f4d31e13f1b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:683
    #11 0x7f4d31e0f547 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:529:20
    #12 0x7f4d2b2a604d in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1039:14
    #13 0x7f4d2b2ab688 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:521:10
    #14 0x7f4d2c0462f1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #15 0x7f4d2bfa6a0b in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #16 0x7f4d2bfa6a0b in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #17 0x7f4d2bfa6a0b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #18 0x7f4d317210ef in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #19 0x7f4d35850201 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #20 0x7f4d35a31f01 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4635:22
    #21 0x7f4d35a33af8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4799:8
    #22 0x7f4d35a34f2b in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4894:21
    #23 0x4eb673 in do_main src/browser/app/nsBrowserApp.cpp:236:22
    #24 0x4eb673 in main src/browser/app/nsBrowserApp.cpp:309
    #25 0x7f4d48c1582f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #26 0x41d1c8 in _start (/home/user/workspace/browsers/m-c-1504308564-asan-opt/firefox+0x41d1c8)

Comment 1

[Eitan Isaacson [:eeejay]]

This is what happens:
1. Body is restyled with "display: none".
  a. This ultimately triggers a content insertion with a null container, and the child being the document element. Apparently this is a legitimate case.
  b. This causes us to queue a content insertion with the owner being the doc accessible as fallback (this also seems to be intended)
2. Document element (document.documentElement) is unparented.
3. Insertion from stage 1 is processed. This calls UpdateRootElIfNeeded which fails to find a nsIContent to associate with the doc accessible because the document element was removed in stage 2.
4. aria-owns relocation is processed on the doc accessible that now does not have an associated content.

I think we will need to put in a stronger check before doing a relocation to make sure the accessible still has an associated content node.

Comment 2

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

SGTM

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Check that owner has content before aria-owns relocation. r?surkov

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8906771 [details] [diff] [review]
Check that owner has content before aria-owns relocation. r?surkov

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

::: accessible/base/NotificationController.cpp
@@ +814,1 @@
>        mDocument->DoARIAOwnsRelocation(mRelocations[idx]);

I wonder if it can be a document's case: if the document doesn't have a body and etc, then there's no place to put aria-owns attribute

Comment 6

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #5)

> ::: accessible/base/NotificationController.cpp
> @@ +814,1 @@
> >        mDocument->DoARIAOwnsRelocation(mRelocations[idx]);
> 
> I wonder if it can be a document's case: if the document doesn't have a body
> and etc, then there's no place to put aria-owns attribute

yeah, I should read bug comments before looking at the patch :)

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8906771 [details] [diff] [review]
Check that owner has content before aria-owns relocation. r?surkov

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

that makes sense, so r=me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32268f6138e0
Check that owner has content before aria-owns relocation. r=surkov

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/32268f6138e0