Crash in IPCError-browser | PDocAccessibleParent::AddChildDoc binding to nonexistant proxy!

RESOLVED FIXED in Firefox 55

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jimm, Assigned: handyman)

Tracking

({crash})

Trunk
mozilla55
Unspecified
Windows 10
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 disabled, firefox53 disabled, firefox54 disabled, firefox55 fixed)

Details

(Whiteboard: aes+, crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Comment 1

a year ago
I can reproduce the crash:

Reproducible: 100%

Steps To Reproduce:
1. Open https://www.minhembio.com/forum/index.php?showforum=40 and wait to complete page loading
2. Click a subject link and wait to complete page loading
3. Navigation Back
4. Repeat from Step.2 in several times

Actual Results:
Win10 64bit Nightly 64bit :
bp-056b13e8-6533-464f-9ab9-a75650170425

Win10 64bit Nightly 32bit :
bp-3a413e69-964d-45b7-ace3-cd10a0170425
status-firefox55: --- → affected
status-firefox57: affected → ---

Comment 2

a year ago
[Tracking Requested - why for this release]: reproducible crash

And also reproduced on 54.0b2 with e10s:
bp-71cc380f-9413-4fd4-84ff-a9c070170425
status-firefox54: --- → affected
tracking-firefox54: --- → ?
(Reporter)

Comment 3

a year ago
(In reply to Alice0775 White from comment #2)
> [Tracking Requested - why for this release]: reproducible crash
> 
> And also reproduced on 54.0b2 with e10s:
> bp-71cc380f-9413-4fd4-84ff-a9c070170425

Alice, do you have e10s forced on while using an IME / accessibility client? This crash shouldn't happen in beta 54 since a11y activation should disabled e10s.
Flags: needinfo?(alice0775)

Comment 4

a year ago
(In reply to Jim Mathies [:jimm] from comment #3)
> (In reply to Alice0775 White from comment #2)
> > [Tracking Requested - why for this release]: reproducible crash
> > 
> > And also reproduced on 54.0b2 with e10s:
> > bp-71cc380f-9413-4fd4-84ff-a9c070170425
> 
> Alice, do you have e10s forced on while using an IME / accessibility client?
> This crash shouldn't happen in beta 54 since a11y activation should disabled
> e10s.

Yes, ATOK2016 activates a11y. And It happens when forcibly enabled e10s.
status-firefox54: affected → disabled
tracking-firefox54: ? → ---
Flags: needinfo?(alice0775)
(Reporter)

Updated

a year ago
Assignee: nobody → davidp99
(Reporter)

Updated

a year ago
Blocks: 1330484
(Assignee)

Comment 5

a year ago
Created attachment 8862067 [details] [diff] [review]
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages

We sometimes briefly have more than one root DocAccessible associated with a TabChild, for example, while navigating links in a page.  This patch makes sure that we use the correct accessible (the most recent one) when delaying messages that we forward to the parent process.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb89336fc434d18c1cb4b30bf6beef34845b46c6
(Assignee)

Updated

a year ago
Attachment #8862067 - Flags: review?(aklotz)
Comment on attachment 8862067 [details] [diff] [review]
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages

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

r=me with nits addressed.

::: accessible/ipc/win/DocAccessibleChild.cpp
@@ +100,5 @@
>      auto tabChild = static_cast<dom::TabChild*>(Manager());
>      if (!tabChild) {
>        return;
>      }
> +    topLevelIPCDoc =

nit: please put a blank line between the end of the above if block and this line.

::: dom/ipc/TabChild.cpp
@@ +401,5 @@
>    , mLayerObserverEpoch(0)
>  #if defined(XP_WIN) && defined(ACCESSIBILITY)
>    , mNativeWindowHandle(0)
>  #endif
> +  , mTopLevelDocAccessibleChild(nullptr)

Please put inside a #ifdef ACCESSIBILITY block

::: dom/ipc/TabChild.h
@@ +674,5 @@
>    already_AddRefed<nsISHistory> GetRelatedSHistory();
>  
>    mozilla::dom::TabGroup* TabGroup();
>  
> +  void SetTopLevelDocAccessibleChild(PDocAccessibleChild* aTopLevelChild)

Can you put these two methods inside an #ifdef ACCESSIBILITY block?

@@ +678,5 @@
> +  void SetTopLevelDocAccessibleChild(PDocAccessibleChild* aTopLevelChild)
> +  {
> +    mTopLevelDocAccessibleChild = aTopLevelChild;
> +  }
> +  PDocAccessibleChild* GetTopLevelDocAccessibleChild()

Nit: Please add a blank line between the end of the previous method definition and this one.

@@ +850,5 @@
>    // The handle associated with the native window that contains this tab
>    uintptr_t mNativeWindowHandle;
>  #endif // defined(XP_WIN)
>  
> +  PDocAccessibleChild* mTopLevelDocAccessibleChild;

Please put inside a #ifdef ACCESSIBLITY block
Attachment #8862067 - Flags: review?(aklotz) → review+
(Assignee)

Comment 7

a year ago
Created attachment 8863489 [details] [diff] [review]
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages (r=aklotz)

Includes aklotz's comments.
Attachment #8862067 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2e5e6b3091
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages. r=aklotz
Keywords: checkin-needed
Comment on attachment 8863489 [details] [diff] [review]
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages (r=aklotz)

>@@ -1476,16 +1476,19 @@ DocAccessible::DoInitialUpdate()
> {
>   if (nsCoreUtils::IsTabDocument(mDocumentNode)) {
>     mDocFlags |= eTabDocument;
>     if (IPCAccessibilityActive()) {
>       nsIDocShell* docShell = mDocumentNode->GetDocShell();
>       if (RefPtr<dom::TabChild> tabChild = dom::TabChild::GetFrom(docShell)) {
>         DocAccessibleChild* ipcDoc = new DocAccessibleChild(this, tabChild);
>         SetIPCDoc(ipcDoc);
>+        if (IsRoot()) {

the only thing that goes through here is top level documents, so that shouldn't be necessary, and really these top level documents shouldn't be RootAccessibles.

>+    topLevelIPCDoc =
>+      static_cast<DocAccessibleChild*>(tabChild->GetTopLevelDocAccessibleChild());

seems like TabChild could store it as a DocAccessibleChild*, there may be a couple other places that need to account for this case, but I think they may be on the parent side and probably "work" fineish anyway.

I think this may be correct, but another way of dealing with this that is perhaps more obviously correct is to just walk up the tree of accessible documents until we find a root and use that.

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a2e5e6b3091
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: --- → disabled
status-firefox-esr52: --- → disabled
(Reporter)

Updated

a year ago
Blocks: 1369356
You need to log in before you can comment on or make changes to this bug.