1713680 - Crash in [@ mozilla::a11y::DocAccessibleParent::AddChildDoc] MOZ_DIAGNOSTIC_ASSERT(false) (Binding to parent that isn't a valid OuterDoc!)

Status: RESOLVED FIXED

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

Description

[James Teh [:Jamie]]

Reported in bug 1669748 comment 19. Same signature, but slightly different crash: this hits an assertion I added in bug 1712210.

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/7624dc19-bf06-411f-8e29-3e7c70210528

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(false) (Binding to parent that isn't a valid OuterDoc!)

Top 10 frames of crashing thread:

0 xul.dll mozilla::a11y::DocAccessibleParent::AddChildDoc accessible/ipc/DocAccessibleParent.cpp:608
1 xul.dll mozilla::a11y::DocAccessibleParent::AddSubtree accessible/ipc/DocAccessibleParent.cpp:151
2 xul.dll mozilla::a11y::DocAccessibleParent::RecvShowEvent accessible/ipc/DocAccessibleParent.cpp:78
3 xul.dll mozilla::a11y::PDocAccessibleParent::OnMessageReceived ipc/ipdl/PDocAccessibleParent.cpp:302
4 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:6677
5 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2079
6 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:766
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1159
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:85
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:328

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1713680: When deferring a11y IPC calls, always defer calls to BrowserBridgeChild::SendSetEmbedderAccessible too.

Previously, we only deferred SendSetEmbedderAccessible calls for in-process iframes if the DocAccessibleChild hadn't sent its constructor yet.
This meant that top level content process documents (tab documents or OOP iframes) called SendSetEmbedderAccessible even if a11y insertions/removals were still being deferred.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bb5bfa18cf9
When deferring a11y IPC calls, always defer calls to BrowserBridgeChild::SendSetEmbedderAccessible too. r=eeejay

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3bb5bfa18cf9

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 5

[James Teh [:Jamie]]

Still crashing. 😩
Windows: bp-95cc4ca0-e601-437f-a133-b482a0210607
Also, we have some crashes with the same assertion on Linux: bp-0a2adbf1-4ab1-41d1-9c2e-5fd660210603
I originally thought this was Windows specific due to the deferring of IPC calls on Windows and my patch attempted to fix that. However, it seems this isn't the only way we can hit this problem.

Comment 6

[James Teh [:Jamie]]

New hypothesis: it looks like a new PBrowserBridge gets created when you change the src of an iframe:
data:text/html,<iframe id="iframe" src="https://google.com/nf"></iframe><button onclick="iframe.src = 'https://example.com/';">change
That means that BrowserBridgeParent::RecvSetEmbedderAccessible wouldn't clean up a stale pending addition for the old iframe document, since it was a different BrowserBridgeParent. I'm not quite sure how this results in this assertion, since the OuterDoc should still be the same and thus have the same id, but perhaps this can combine with layout frame reconstruction or similar to create strange results.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1713680: Correctly handle the case where the BrowserBridgeParent for an OOP iframe changes while addition of its DocAccessibleParent is still pending.

An OuterDocAccessible can be recreated, causing the embedder accessible for a BrowserBridgeParent (OOP iframe) to change.
However, changing the src of an iframe can also cause a new BrowserBridgeParent to be created.
Previously, if addition of the child document was still pending when this occurred (because the OuterDocAccessible hadn't been sent to the parent yet), this pending addition could remain, causing problems if the id was reused later.

To fix this (and to hopefully make this more robust given the continued problems we're seeing in the wild with this area of the code), I've completely refactored the way we handle these pending child doc additions.
Rather than tracking the pending additions by their accessible id and child doc, we track them by their BrowserBridgeParent.
This way, we're closest to a single source of truth.
We also remove a pending addition when an associated BrowserBridgeParent is destroyed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/238ccbc3793f
Correctly handle the case where the BrowserBridgeParent for an OOP iframe changes while addition of its DocAccessibleParent is still pending. r=eeejay

Comment 9

[Marian-Vasile Laza]

Backed out changeset 238ccbc3793f (Bug 1713680) for causing bustages on BrowserBridgeParent.cpp
Backout link
Push with failures
Failure Log

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0a20cd5b703
Correctly handle the case where the BrowserBridgeParent for an OOP iframe changes while addition of its DocAccessibleParent is still pending. r=eeejay

Comment 11

[Atila Butkovits]

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