Closed Bug 1703600 Opened 3 years ago Closed 3 years ago

Crash [@ AttrArray::GetAttr]

Categories

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

defect

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox87 --- wontfix
firefox88 --- fixed
firefox89 --- fixed

People

(Reporter: jkratzer, Assigned: Jamie)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression, testcase, Whiteboard: [bugmon:bisected,confirmed])

Crash Data

Attachments

(7 files)

Attached file testcase.html

Testcase found while fuzzing mozilla-central rev 8f7e11867d56 (built with --enable-debug). Testcase requires the GNOME_ACCESSIBILITY=1 env variable.

    #0 0x7fbe5f6b3484 in AttrArray::GetAttr(nsAtom const*, int) const /builds/worker/checkouts/gecko/dom/base/AttrArray.cpp
    #1 0x7fbe5f7a2011 in mozilla::dom::Element::GetAttr(int, nsAtom const*, mozilla::dom::DOMString&) const /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Element.h:1092:37
    #2 0x7fbe5f753ac7 in mozilla::dom::Element::GetAttr(int, nsAtom const*, nsTSubstring<char16_t>&) const /builds/worker/checkouts/gecko/dom/base/Element.cpp:2698:19
    #3 0x7fbe61016da5 in mozilla::dom::HTMLLabelElement::GetLabeledElement() const /builds/worker/checkouts/gecko/dom/html/HTMLLabelElement.cpp:205:8
    #4 0x7fbe635eafd1 in GetControl /builds/worker/workspace/obj-build/dist/include/mozilla/dom/HTMLLabelElement.h:41:53
    #5 0x7fbe635eafd1 in mozilla::a11y::HTMLLabelAccessible::RelationByType(mozilla::a11y::RelationType) const /builds/worker/checkouts/gecko/accessible/html/HTMLElementAccessibles.cpp:53:35
    #6 0x7fbe635e07b4 in mozilla::a11y::LocalAccessible::BindToParent(mozilla::a11y::LocalAccessible*, unsigned int) /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2132:7
    #7 0x7fbe635d958a in mozilla::a11y::LocalAccessible::InsertChildAt(unsigned int, mozilla::a11y::LocalAccessible*) /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2236:11
    #8 0x7fbe635cf7f0 in AppendChild /builds/worker/workspace/obj-build/dist/include/mozilla/a11y/LocalAccessible.h:356:12
    #9 0x7fbe635cf7f0 in mozilla::a11y::DocAccessible::CacheChildrenInSubtree(mozilla::a11y::LocalAccessible*, mozilla::a11y::LocalAccessible**) /builds/worker/checkouts/gecko/accessible/generic/DocAccessible.cpp:2618:13
    #10 0x7fbe635d0870 in mozilla::a11y::DocAccessible::CreateSubtree(mozilla::a11y::LocalAccessible*) /builds/worker/checkouts/gecko/accessible/generic/DocAccessible-inl.h:125:3
    #11 0x7fbe635d0043 in mozilla::a11y::DocAccessible::ProcessContentInserted(mozilla::a11y::LocalAccessible*, nsTArray<nsCOMPtr<nsIContent> > const*) /builds/worker/checkouts/gecko/accessible/generic/DocAccessible.cpp:2175:7
    #12 0x7fbe6359aa94 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /builds/worker/checkouts/gecko/accessible/base/NotificationController.cpp:757:16
    #13 0x7fbe6246bbc2 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2128:12
    #14 0x7fbe62474011 in TickDriver /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:346:13
    #15 0x7fbe62474011 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:324:7
    #16 0x7fbe62473ef3 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:340:5
    #17 0x7fbe62473508 in RunRefreshDrivers /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:773:5
    #18 0x7fbe62473508 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:702:16
    #19 0x7fbe62472dee in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyParentProcessVsync() /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:615:7
    #20 0x7fbe62472869 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:536:9
    #21 0x7fbe61c85736 in mozilla::dom::VsyncChild::RecvNotify(mozilla::VsyncEvent const&, float const&) /builds/worker/checkouts/gecko/dom/ipc/VsyncChild.cpp:68:15
    #22 0x7fbe5ea132c0 in mozilla::dom::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PVsyncChild.cpp:178:54
    #23 0x7fbe5e80436c in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundChild.cpp:6008:32
    #24 0x7fbe5e4b829e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:2154:25
    #25 0x7fbe5e4b477d in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:2078:9
    #26 0x7fbe5e4b5c26 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1926:3
    #27 0x7fbe5e4b696b in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1957:13
    #28 0x7fbe5db91853 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:470:16
    #29 0x7fbe5db6c123 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:754:26
    #30 0x7fbe5db6b074 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:609:15
    #31 0x7fbe5db6b203 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:393:36
    #32 0x7fbe5db952f6 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:133:37
    #33 0x7fbe5db952f6 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:534:5
    #34 0x7fbe5db7e8f0 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1159:16
    #35 0x7fbe5db8559a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:548:10
    #36 0x7fbe5e4bdbd6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #37 0x7fbe5e428923 in MessageLoop::RunInternal() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:335:10
    #38 0x7fbe5e42883d in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:328:3
    #39 0x7fbe5e42883d in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:310:3
    #40 0x7fbe621af0f8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #41 0x7fbe63a27d33 in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:906:20
    #42 0x7fbe5e4beabc in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:237:9
    #43 0x7fbe5e428923 in MessageLoop::RunInternal() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:335:10
    #44 0x7fbe5e42883d in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:328:3
    #45 0x7fbe5e42883d in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:310:3
    #46 0x7fbe63a2790f in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:738:34
    #47 0x5620e35cdfb6 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #48 0x5620e35cdfb6 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:309:18
    #49 0x7fbe744820b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
Flags: in-testsuite?
Summary: Crash [@ operator] | [@ mozilla::a11y::HTMLLabelAccessible::RelationByType] → Crash [@ AttrArray::GetAttr]
Whiteboard: [bugmon:confirmed]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210408095111-83a21ab93aff.
The bug appears to have been introduced in the following build range:

Start: 37cbc73b07c3e0b8d9441bea2beae71ecdc1dd14 (20210127045122)
End: 2c9004c95a7a7d39c2c1417b66fda37892398263 (20210127053539)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=37cbc73b07c3e0b8d9441bea2beae71ecdc1dd14&tochange=2c9004c95a7a7d39c2c1417b66fda37892398263

Whiteboard: [bugmon:bisected,confirmed]
Regressed by: 493683
Has Regression Range: --- → yes

Simpler test case:
data:text/html,<math><label>
The HTMlLabelAccessible code assumes it can get an HTMlLabelElement for the DOM node. However, when it's inside a MathML subtree, it can't.

The wallpaper fix would be to null check HTMLLabelElement::FromNode. However, HTML elements aren't created inside a MathML subtree, so we shouldn't be creating HTML accessibles for them. I suspect we'll eventually hit some other bug like this. The correct fix is to separate the HTML and MathML markup maps.

Assignee: nobody → jteh
Severity: -- → S3
Priority: -- → P1

I wrote all the patches and then discovered that this:
data:text/html,<math><a href="https://mozilla.org/">Mozilla
seems to create a link, even though it isn't an HTMl element. So this approach breaks that case.

My understanding from the specs is that <a> isn't a MathML element. 😕 It's valid to put HTML inside <mtext>, but in that case, the descendants are HTML elements and things work correctly. That is, this still works with my patches:
data:text/html,<math><mtext><a href="https://mozilla.org/">Mozilla

I asked about this on Matrix. Nevertheless, I've spent too much time on this already, so I think I'm just going to wallpaper. We seem to do this in a few other places already anyway.

See Also: → 1704013

I got a response on matrix. It turns out that the <a> was a red herring. In reality, the href attribute on any MathML element will create a link. So even this:
data:text/html,<math><mn href="https://mozilla.org/">1
creates a link. The fact that this currently works for <a> is a happy accident, but <a> isn't valid MathML anyway.

All of this means my original approach is still valid.

We will soon have multiple markup maps using this type.

For now, this just uses mHTMlMarkupMap, but it might choose the MathML map in future.
In some places, we're already inside a block which explicitly checks IsHTMlElement.
In those cases, we still use mHTMlMarkupMap directly, since using GetMarkupMapInfoForNode would redundantly check the element type.

For now, these are still inserted into mHTMlMarkupMap, but this will change soon.

Now, MathML elements will never use the HTML markup map.

:ryanvm, I think we should get this crash fixed in beta, but the patch set I'm proposing for 89 is larger and a tad riskier than is ideal for beta. I can create a tiny null check fix which just hacks around the crash for 88. Does this seem reasonable? How do I go about submitting a separate patch that's quite different but only for beta?

Flags: needinfo?(ryanvm)

Set release status flags based on info from the regressing bug 493683

You can either attach to bugzilla as a patch or push a new revision to phabricator.

Crash Signature: [@ mozilla::dom::Element::GetAttr ]

Given the volume and where we are in the cycle, I'd consider a low-risk patch as an RC ride-along but won't hold up builds for it.

Attachment #9214579 - Attachment description: Bug 1703600 part 3: Rename MarkupMap.h to HTMlMarkupMap.h. → Bug 1703600 part 3: Rename MarkupMap.h to HTMLMarkupMap.h.
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2882ed45e83
part 1: Rename HTMLMarkupMapInfo to MarkupMapInfo. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/827266cbdda1
part 2: Add GetMarkupMapInfoForNode and use it wherever we aren't explicitly dealing with a specific type of element. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/cbc7e61d6f1e
part 3: Rename MarkupMap.h to HTMLMarkupMap.h. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/8e3004368cb7
part 4: Move MathML elements into MathMLMarkupMap.h. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/7ab66707d169
part 5: Create mMathMLMarkupMap and use it for MathML elements. r=eeejay

Comment on attachment 9215006 [details]
Bug 1703600: Simple null check for beta to prevent crash with HTMlLabelAccessible created inside MathML subtree.

Beta/Release Uplift Approval Request

  • User impact if declined: This fixes an easy crasher.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a simple null check. The risk is in a new test that is introduced, but that would be more of a CI headache, not a product bug.
  • String changes made/needed:
Attachment #9215006 - Flags: approval-mozilla-release?

Comment on attachment 9215006 [details]
Bug 1703600: Simple null check for beta to prevent crash with HTMlLabelAccessible created inside MathML subtree.

Approved for 88.0rc2, thanks.

Attachment #9215006 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: