1400617 - Assertion failure: aOwner->Elm() (aOwner->Elm() must be a valid pointer) in [@ mozilla::a11y::DocAccessible::DoARIAOwnsRelocation]

Status: RESOLVED WONTFIX

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: aOwner->Elm() (aOwner->Elm() must be a valid pointer), at /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2068

This only seems to affect ESR52 (20170916-292ae551b2b3)

The test case takes a few tries to trigger the issue.

0|0|libxul.so|mozilla::a11y::DocAccessible::DoARIAOwnsRelocation|hg:hg.mozilla.org/releases/mozilla-esr52:accessible/generic/DocAccessible.cpp:292ae551b2b3|2068|0x0
0|1|libxul.so|mozilla::a11y::NotificationController::WillRefresh|hg:hg.mozilla.org/releases/mozilla-esr52:accessible/base/NotificationController.cpp:292ae551b2b3|802|0xc
0|2|libxul.so|nsRefreshDriver::Tick|hg:hg.mozilla.org/releases/mozilla-esr52:layout/base/nsRefreshDriver.cpp:292ae551b2b3|1798|0x11
0|3|libxul.so|mozilla::RefreshDriverTimer::TickRefreshDrivers|hg:hg.mozilla.org/releases/mozilla-esr52:layout/base/nsRefreshDriver.cpp:292ae551b2b3|295|0xf
0|4|libxul.so|mozilla::RefreshDriverTimer::Tick|hg:hg.mozilla.org/releases/mozilla-esr52:layout/base/nsRefreshDriver.cpp:292ae551b2b3|317|0x12
0|5|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver|hg:hg.mozilla.org/releases/mozilla-esr52:layout/base/nsRefreshDriver.cpp:292ae551b2b3|663|0x5
0|6|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync|hg:hg.mozilla.org/releases/mozilla-esr52:layout/base/nsRefreshDriver.cpp:292ae551b2b3|501|0xb
0|7|libxul.so|mozilla::layout::VsyncChild::RecvNotify|hg:hg.mozilla.org/releases/mozilla-esr52:layout/ipc/VsyncChild.cpp:292ae551b2b3|64|0x9
0|8|libxul.so|mozilla::layout::PVsyncChild::OnMessageReceived|hg:hg.mozilla.org/releases/mozilla-esr52:obj-firefox/ipc/ipdl/PVsyncChild.cpp:292ae551b2b3|169|0xf
0|9|libxul.so|mozilla::ipc::MessageChannel::DispatchAsyncMessage|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessageChannel.cpp:292ae551b2b3|1743|0x6
0|10|libxul.so|mozilla::ipc::MessageChannel::DispatchMessage|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessageChannel.cpp:292ae551b2b3|1681|0xb
0|11|libxul.so|mozilla::ipc::MessageChannel::RunMessage|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessageChannel.cpp:292ae551b2b3|1572|0xb
0|12|libxul.so|mozilla::ipc::MessageChannel::MessageTask::Run|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessageChannel.cpp:292ae551b2b3|1597|0xc
0|13|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/releases/mozilla-esr52:xpcom/threads/nsThread.cpp:292ae551b2b3|1216|0x11
0|14|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/releases/mozilla-esr52:xpcom/glue/nsThreadUtils.cpp:292ae551b2b3|361|0xd
0|15|libxul.so|mozilla::ipc::MessagePump::Run|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessagePump.cpp:292ae551b2b3|96|0xa
0|16|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/chromium/src/base/message_loop.cc:292ae551b2b3|232|0x17
0|17|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/chromium/src/base/message_loop.cc:292ae551b2b3|225|0x8
0|18|libxul.so|nsBaseAppShell::Run|hg:hg.mozilla.org/releases/mozilla-esr52:widget/nsBaseAppShell.cpp:292ae551b2b3|156|0xd
0|19|libxul.so|XRE_RunAppShell|hg:hg.mozilla.org/releases/mozilla-esr52:toolkit/xre/nsEmbedFunctions.cpp:292ae551b2b3|866|0x6
0|20|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/glue/MessagePump.cpp:292ae551b2b3|269|0x5
0|21|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/chromium/src/base/message_loop.cc:292ae551b2b3|232|0x17
0|22|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/chromium/src/base/message_loop.cc:292ae551b2b3|225|0x8
0|23|libxul.so|XRE_InitChildProcess|hg:hg.mozilla.org/releases/mozilla-esr52:toolkit/xre/nsEmbedFunctions.cpp:292ae551b2b3|698|0xf
0|24|plugin-container|content_process_main|hg:hg.mozilla.org/releases/mozilla-esr52:ipc/contentproc/plugin-container.cpp:292ae551b2b3|197|0xe
0|25|libc-2.23.so||||0x20830
0|26|plugin-container|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/releases/mozilla-esr52:mfbt/Assertions.h:292ae551b2b3|170|0x5

Comment 1

[Eitan Isaacson [:eeejay]]

Pretty certain we fixed this in bug 1396267. I'll see if a backported patch helps.

Comment 2

[Eitan Isaacson [:eeejay]]

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

Comment 3

[alexander :surkov (:asurkov)]

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

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

it makes sense for sure, however shouldn't we request bug 1396267 backported instead?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact that even justifies fixing this on ESR52? Beyond making the fuzzers happy, that is.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> Is there a user impact that even justifies fixing this on ESR52? Beyond
> making the fuzzers happy, that is.

You can crash Firefox from content when a11y is enabled? If that is not a strong enough case for ESR, I'm fine with that too. I'll ask the gods for approval, we'll see what happens.

As Alex suggests, this is a dup of bug 1396267, but let's keep it a blocker since we already have conversation here.

Comment 6

[Eitan Isaacson [:eeejay]]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: There is a potential crash from a content.
User impact if declined: A potential crash when a11y is enabled.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): This should be very low risk.
String or UUID changes made by this patch: None.



This patch is very low risk. OTOH, I'm not sure if the user benefit is all that worth it, even if this is a crasher. Honestly on the fence if this is needed.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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

I get that this would be more correct and might avoid a crash. But I am only a frail mortal and so would like more evidence that the crashes actually happen on ESR before uplifting. Happy to reconsider if anyone feels strongly about it.

Comment 8

[Eitan Isaacson [:eeejay]]

Thanks for making the hard choices for us.