Crash in mozilla::a11y::DocAccessibleParent::RemoveAccessible(ProxyAccessible* aAccessible)

RESOLVED FIXED in Firefox 42

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lsocks, Assigned: lsocks)

Tracking

({crash})

unspecified
mozilla42
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(crash signature)

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8624233 [details]
backtrace

I found this crash while trying to reproduce bug 1100602. No reliable way to reproduce yet, but it's been captured in rr.
(Assignee)

Comment 1

3 years ago
So far it looks like we're creating duplicate ProxyAccessibles with the same ID. This seems to be happening when nodes are reparented and a new node is inserted between an existing parent and child. When this happens, the children ProxyAccessibles are being created anew instead of using the existing ones, so when they are shut down, we try to remove them from the DocAccessibleParent twice which throws the assertion failure.
(Assignee)

Updated

3 years ago
Assignee: nobody → lorien
(Assignee)

Comment 2

3 years ago
Caused when click event listeners are added to a previously non-accessible node, making it accessible. Since we don't listen to event listener changes, we don't properly handle this case.
(Assignee)

Updated

3 years ago
Depends on: 1180798

Updated

2 years ago
Crash Signature: [@ mozilla::a11y::DocAccessibleParent::RemoveAccessible(ProxyAccessible* aAccessible)]
Keywords: crash
(Assignee)

Comment 3

2 years ago
Created attachment 8635521 [details] [diff] [review]
recreate accessibles when relevant event listeners change
Attachment #8635521 - Flags: feedback?(tbsaunde+mozbugs)
(Assignee)

Comment 4

2 years ago
Created attachment 8635523 [details] [diff] [review]
recreate accessibles when relevant event listeners change
Attachment #8635521 - Attachment is obsolete: true
Attachment #8635521 - Flags: feedback?(tbsaunde+mozbugs)
Attachment #8635523 - Flags: feedback?(tbsaunde+mozbugs)
(Assignee)

Comment 5

2 years ago
Created attachment 8635528 [details]
recreate accessibles when relevant event listeners change

forgot to rename test2 when I renamed the tests
Attachment #8635523 - Attachment is obsolete: true
Attachment #8635523 - Flags: feedback?(tbsaunde+mozbugs)
Attachment #8635528 - Flags: feedback?(tbsaunde+mozbugs)
Comment on attachment 8635528 [details]
recreate accessibles when relevant event listeners change

>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+  nsCOMPtr<nsIEventListenerService> eventListenerService =
>+    do_GetService("@mozilla.org/eventlistenerservice;1");
>+  if (!eventListenerService)
>+    return NS_ERROR_FAILURE;

I'd say just assert it.  caring about the xpcom idea this can fail is silly.

>+
>+  uint32_t targetCount;
>+  nsresult rv = aEventChanges->GetLength(&targetCount);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }

NS_ENSURE_SUCCESS please

>+
>+  for (uint32_t i = 0 ; i < targetCount ; i++) {
>+    nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+    nsIDOMEventTarget* target;

nsCOMPtr please

>+    change->GetTarget(&target);
>+    nsIArray* listenerNames;

same

>+    change->GetChangedListenerNames(&listenerNames);
>+
>+    uint32_t changeCount;
>+    rv = listenerNames->GetLength(&changeCount);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+    }

NS_ENSURE_SUCCESS please

>+
>+    for (uint32_t i = 0 ; i < changeCount ; i++) {
>+      nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+      // We are only interested in event listener changes which may
>+      // make an element accessible or inaccessible.
>+      if (listenerName == nsGkAtoms::onclick ||
>+          listenerName == nsGkAtoms::onmousedown ||
>+          listenerName == nsGkAtoms::onmouseup) {
>+
>+        nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+        if (!node) {
>+          return NS_OK;
>+        }
>+
>+        nsCOMPtr<nsIDocument> ownerDoc = node->OwnerDoc();

I don't see a reason to hold a ref to the document so nsIDocument* is fine.

>+        if (!ownerDoc) {
>+          return NS_OK;
>+        }

not needed it can never fail.
Attachment #8635528 - Flags: feedback?(tbsaunde+mozbugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8636101 [details] [diff] [review]
recreate accessibles when relevant event listeners change
Attachment #8635528 - Attachment is obsolete: true
Attachment #8636101 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8636101 [details] [diff] [review]
recreate accessibles when relevant event listeners change

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1437413726 14400
>#      Mon Jul 20 13:35:26 2015 -0400
># Node ID 9a787e7db7c4fbecdc2a091bd21f898b532223fc
># Parent  5df788c56ae77e1a2e906535c5ef26837c22f5a2
>Bug 1175913 - Subscribe to EventListenerService and recreate accessibles on click listener changes
>
>diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
>--- a/accessible/base/nsAccessibilityService.cpp
>+++ b/accessible/base/nsAccessibilityService.cpp
>@@ -17,16 +17,17 @@
> #include "HTMLLinkAccessible.h"
> #include "HTMLListAccessible.h"
> #include "HTMLSelectAccessible.h"
> #include "HTMLTableAccessibleWrap.h"
> #include "HyperTextAccessibleWrap.h"
> #include "RootAccessible.h"
> #include "nsAccessiblePivot.h"
> #include "nsAccUtils.h"
>+#include "nsArrayUtils.h"
> #include "nsAttrName.h"
> #include "nsEventShell.h"
> #include "nsIURI.h"
> #include "OuterDocAccessible.h"
> #include "Platform.h"
> #include "Role.h"
> #ifdef MOZ_ACCESSIBILITY_ATK
> #include "RootAccessibleWrap.h"
>@@ -269,23 +270,74 @@ nsAccessibilityService::nsAccessibilityS
> 
> nsAccessibilityService::~nsAccessibilityService()
> {
>   NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!");
>   gAccessibilityService = nullptr;
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
>+// nsIListenerChangeListener
>+
>+NS_IMETHODIMP
>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+  uint32_t targetCount;
>+  nsresult rv = aEventChanges->GetLength(&targetCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  for (uint32_t i = 0 ; i < targetCount ; i++) {
>+    nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    change->GetTarget(getter_AddRefs(target));
>+    nsCOMPtr<nsIArray> listenerNames;

seems like blank line before this var would be typical.

>+    change->GetChangedListenerNames(getter_AddRefs(listenerNames));
>+
>+    uint32_t changeCount;
>+    rv = listenerNames->GetLength(&changeCount);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    for (uint32_t i = 0 ; i < changeCount ; i++) {
>+      nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+      // We are only interested in event listener changes which may
>+      // make an element accessible or inaccessible.
>+      if (listenerName == nsGkAtoms::onclick ||
>+          listenerName == nsGkAtoms::onmousedown ||
>+          listenerName == nsGkAtoms::onmouseup) {
>+

and this one seems unnecessary

>+        nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+        if (!node) {
>+          return NS_OK;
>+        }

does that really make sense? I'm not sure if you can get these particular events on non nsIContent things, but you certainly can get other events on non nodes, and then I'd think you just want to skip this target's changes.

>+
>+        nsIDocument* ownerDoc = node->OwnerDoc();
>+        DocAccessible* document = GetExistingDocAccessible(ownerDoc);
>+
>+        if (document) {
>+            document->RecreateAccessible(node);

so you are possibly going to call RecreateAccessible multiple times for the same thing right? that seems pointless.

>+        }
>+      }
>+    }
>+    NS_RELEASE(target);
>+    NS_RELEASE(listenerNames);

these release those objects too many times now.

>+  // Subscribe to EventListenerService.
>+  nsCOMPtr<nsIEventListenerService> eventListenerService =
>+    do_GetService("@mozilla.org/eventlistenerservice;1");
>+  if (!eventListenerService)
>+    return false;
>+  eventListenerService->AddListenerChangeListener(this);

blank line after return please
Attachment #8636101 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 9

2 years ago
Created attachment 8636733 [details] [diff] [review]
recreate accessibles when relevant event listeners change
Attachment #8636101 - Attachment is obsolete: true
Attachment #8636733 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8636733 [details] [diff] [review]
recreate accessibles when relevant event listeners change

>+  for (uint32_t i = 0 ; i < targetCount ; i++) {
>+    nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    change->GetTarget(getter_AddRefs(target));

you could move this down to just before you QI it  to nsINOde right?
Attachment #8636733 - Flags: review?(tbsaunde+mozbugs) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8639953 [details] [diff] [review]
listener changes updated

carry r=tbsaunde
Attachment #8636733 - Attachment is obsolete: true
Attachment #8639953 - Flags: review+
(Assignee)

Comment 12

2 years ago
Created attachment 8640621 [details] [diff] [review]
recreate accessibles when relevant event listeners change

Fix test and only recreate for HTML accessibles.
Attachment #8639953 - Attachment is obsolete: true
Attachment #8640621 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Depends on: 1189108
(Assignee)

Comment 13

2 years ago
Created attachment 8641704 [details] [diff] [review]
don't expect accessible recreation on onclick change if accessible already exists
Attachment #8641704 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8641704 [details] [diff] [review]
don't expect accessible recreation on onclick change if accessible already exists

I'd rather test the accessible didn't get recreated, but I'm not sure how to do that.
Attachment #8641704 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8640621 [details] [diff] [review]
recreate accessibles when relevant event listeners change

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1437507758 14400
>#      Tue Jul 21 15:42:38 2015 -0400
># Node ID c883a1dc4e6efd12e3345f9776526b3cfee54383
># Parent  2ee9895e032c492705adaf213706d4260ca172c8
>Bug 1175913 - Subscribe to EventListenerService and recreate accessibles on click listener changes r=tbsaunde
>* * *
>try: -b do -p all -u mochitest-o -t none
>* * *
>try: -b do -p all -u mochitest-o -t none
>
>diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
>--- a/accessible/base/nsAccessibilityService.cpp
>+++ b/accessible/base/nsAccessibilityService.cpp
>@@ -17,16 +17,17 @@
> #include "HTMLLinkAccessible.h"
> #include "HTMLListAccessible.h"
> #include "HTMLSelectAccessible.h"
> #include "HTMLTableAccessibleWrap.h"
> #include "HyperTextAccessibleWrap.h"
> #include "RootAccessible.h"
> #include "nsAccessiblePivot.h"
> #include "nsAccUtils.h"
>+#include "nsArrayUtils.h"
> #include "nsAttrName.h"
> #include "nsEventShell.h"
> #include "nsIURI.h"
> #include "OuterDocAccessible.h"
> #include "Platform.h"
> #include "Role.h"
> #ifdef MOZ_ACCESSIBILITY_ATK
> #include "RootAccessibleWrap.h"
>@@ -269,23 +270,83 @@ nsAccessibilityService::nsAccessibilityS
> 
> nsAccessibilityService::~nsAccessibilityService()
> {
>   NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!");
>   gAccessibilityService = nullptr;
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
>+// nsIListenerChangeListener
>+
>+NS_IMETHODIMP
>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+  uint32_t targetCount;
>+  nsresult rv = aEventChanges->GetLength(&targetCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  for (uint32_t i = 0 ; i < targetCount ; i++) {
>+    nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+
>+    nsCOMPtr<nsIArray> listenerNames;
>+    change->GetChangedListenerNames(getter_AddRefs(listenerNames));
>+
>+    uint32_t changeCount;
>+    rv = listenerNames->GetLength(&changeCount);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    for (uint32_t i = 0 ; i < changeCount ; i++) {
>+      nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+      // We are only interested in event listener changes which may
>+      // make an element accessible or inaccessible.
>+      if (listenerName == nsGkAtoms::onclick ||
>+          listenerName == nsGkAtoms::onmousedown ||
>+          listenerName == nsGkAtoms::onmouseup) {

seems like continue is better than indenting the rest of the function

>+        nsCOMPtr<nsIDOMEventTarget> target;
>+        change->GetTarget(getter_AddRefs(target));
>+        nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+        if (!node || !node->IsHTMLElement()) {
>+          break;
>+        }

I wonder if it would be better to hoist this above the loop

>+
>+        nsIDocument* ownerDoc = node->OwnerDoc();
>+        DocAccessible* document = GetExistingDocAccessible(ownerDoc);
>+
>+        if (document) {
>+            nsString testName;
>+            listenerName->ToString(testName);

unused?

>+
>+            if (nsCoreUtils::HasClickListener(node)) {
>+              if (!document->GetAccessible(node)) {
>+                document->RecreateAccessible(node);

its tempting to just use document->ContentInserted()

>+++ b/accessible/base/nsAccessibilityService.h
>@@ -10,16 +10,18 @@
> 
> #include "mozilla/a11y/DocManager.h"
> #include "mozilla/a11y/FocusManager.h"
> #include "mozilla/a11y/Role.h"
> #include "mozilla/a11y/SelectionManager.h"
> #include "mozilla/Preferences.h"
> 
> #include "nsIObserver.h"
>+#include "nsIEventListenerService.h"
>+#include "nsIArray.h"

isn't a forward decl of nsIArray enough?
Attachment #8640621 - Flags: review?(tbsaunde+mozbugs) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8643462 [details] [diff] [review]
recreate accessibles when relevant event listeners change

carry r=tbsaunde
Attachment #8640621 - Attachment is obsolete: true
Attachment #8643462 - Flags: review+

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/89002bda1f55
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d03329ef85e
https://hg.mozilla.org/mozilla-central/rev/89002bda1f55
https://hg.mozilla.org/mozilla-central/rev/6d03329ef85e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.