Closed Bug 1359129 Opened 2 years ago Closed 2 years ago

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

Categories

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

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: jimm, Assigned: handyman)

References

Details

(Keywords: crash, Whiteboard: aes+)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
[Tracking Requested - why for this release]: reproducible crash

And also reproduced on 54.0b2 with e10s:
bp-71cc380f-9413-4fd4-84ff-a9c070170425
(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)
(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.
Flags: needinfo?(alice0775)
Assignee: nobody → davidp99
Blocks: 1330484
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
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+
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/2a2e5e6b3091
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1369356
You need to log in before you can comment on or make changes to this bug.