Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

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

	Lorien Hu (:lsocks) Assignee
	Comment 1 • 9 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.

	Lorien Hu (:lsocks) Assignee
	Comment 2 • 9 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.

	Lorien Hu (:lsocks) Assignee
	Comment 5 • 9 years ago

forgot to rename test2 when I renamed the tests

	Trevor Saunders (:tbsaunde)
	Comment 6 • 9 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 8 • 9 years ago

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

	Trevor Saunders (:tbsaunde)
	Comment 10 • 9 years ago

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?

	Lorien Hu (:lsocks) Assignee
	Comment 11 • 9 years ago

carry r=tbsaunde

	Lorien Hu (:lsocks) Assignee
	Comment 12 • 9 years ago

Fix test and only recreate for HTML accessibles.

	Trevor Saunders (:tbsaunde)
	Comment 14 • 9 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 15 • 9 years ago

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?

	Lorien Hu (:lsocks) Assignee
	Comment 16 • 9 years ago

carry r=tbsaunde

	Pulsebot
	Comment 17 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/89002bda1f55
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d03329ef85e

	Carsten Book [:Tomcat]
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/89002bda1f55
https://hg.mozilla.org/mozilla-central/rev/6d03329ef85e