Crash [@ AttrArray::GetAttr]
Categories
(Core :: Disability Access APIs, defect, P1)
Tracking
()
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)
(deleted),
text/html
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-release+
|
Details |
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
Reporter | ||
Updated•4 years ago
|
Comment hidden (obsolete) |
Reporter | ||
Updated•4 years ago
|
Comment 2•4 years ago
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
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 | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
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.
Assignee | ||
Comment 7•4 years ago
|
||
We will soon have multiple markup maps using this type.
Assignee | ||
Comment 8•4 years ago
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 10•4 years ago
|
||
For now, these are still inserted into mHTMlMarkupMap, but this will change soon.
Assignee | ||
Comment 11•4 years ago
|
||
Now, MathML elements will never use the HTML markup map.
Assignee | ||
Comment 12•4 years ago
|
||
: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?
Comment 13•4 years ago
|
||
Set release status flags based on info from the regressing bug 493683
Comment 14•4 years ago
|
||
You can either attach to bugzilla as a patch or push a new revision to phabricator.
Comment 15•4 years ago
|
||
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.
Assignee | ||
Comment 16•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 17•4 years ago
|
||
Comment 18•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2882ed45e83
https://hg.mozilla.org/mozilla-central/rev/827266cbdda1
https://hg.mozilla.org/mozilla-central/rev/cbc7e61d6f1e
https://hg.mozilla.org/mozilla-central/rev/8e3004368cb7
https://hg.mozilla.org/mozilla-central/rev/7ab66707d169
Comment 19•4 years ago
|
||
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:
Comment 20•4 years ago
|
||
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.
Comment 21•4 years ago
|
||
bugherder uplift |
Description
•