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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jim Mathies [:jimm]]

Back again for a second showing.

https://crash-stats.mozilla.com/signature/?date=%3C2017-04-24T17%3A15%3A55%2B00%3A00&date=%3E%3D2017-04-17T17%3A15%3A55%2B00%3A00&product=Firefox&version=55.0a1&signature=IPCError-browser%20%7C%20PDocAccessibleParent%3A%3AAddChildDoc%20binding%20to%20nonexistant%20proxy%21

Comment 1

[Alice0775 White]

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

Comment 2

[Alice0775 White]

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

And also reproduced on 54.0b2 with e10s:
bp-71cc380f-9413-4fd4-84ff-a9c070170425

Comment 3

[Jim Mathies [:jimm]]

(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.

Comment 4

[Alice0775 White]

(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.

Comment 5

[David Parks [:handyman]]

Attachment: 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

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 7

[David Parks [:handyman]]

Attachment: Use the most recent RootDocAccessible when delaying DocAccessibleChild messages (r=aklotz)

Includes aklotz's comments.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2e5e6b3091
Use the most recent RootDocAccessible when delaying DocAccessibleChild messages. r=aklotz

Comment 9

[Trevor Saunders (:tbsaunde)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a2e5e6b3091