203774 - Fix crashes from accessibility rewrite

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The accessibility rearchitecturing caused some new crashes. People who have rare
utilities such as the iFeel mouse are experiencing some of them.

This is a set of fixes designed to fix all of the problems I see in recent
talkback reports.

Comment 1

[Aaron Leventhal]

Attachment: Fix crashes from talkback reports and by looking at code

1. Don't allow accessible tree to be walked from dead node
2. Don't allow dead accessibles to be retrieved for IAccessible getters
3. Check for dead nodes in nsAccessible methods
4. Fix select accessible crashes resulting from late addref in getter
5. Create doc accessible on demand if necessary, when initializing a new
accessible in that document, so the new accessible can be cached
6. Check for null scroll watch timer in scroll watch callback 
7. Fix missing null check in nsAccessNode::GetComputedStyleDeclaration

Comment 2

[Kyle Yuan]

Comment on attachment 122000 [details] [diff] [review]
Fix crashes from talkback reports and by looking at code

r=kyle

Comment 3

[Alec Flett]

before I review this, please answer peterv (and now myself) as to whether bug
149654 recieved approval. There is no evidence in the bug itself and if it
caused these crashes then drivers needs to decide if this bug will land or the
other bug will be backed out.

Comment 4

[Aaron Leventhal]

No one approved bug 149654, during my move to Germany I didn't realize the tree
had changed status.

Bug 149654 did not cause these crashes, those were caused by the accessibility
rewrite in general. Bug 149654 actually fixes some crashes, because it ensures
we shutdown all accessibles, even those that get added in dynamic pages.

Comment 5

[Alec Flett]

Comment on attachment 122000 [details] [diff] [review]
Fix crashes from talkback reports and by looking at code

+  if (!docAccessible) {
+    // No doc accessible yet for this node's document. 
+    // There was probably an accessible event fired before the 
+    // current document was finished loading.
+    // Create a doc accessible so we can cache this node

it sounds like we have code for doing this elsewhere.. (like, after the doc
finishes loading) is this a common pattern that can/should be consolidated into
a helper function?
+	 nsCOMPtr<nsIAccessibilityService> accService = 
+	   do_GetService("@mozilla.org/accessibilityService;1");
+	 NS_ASSERTION(accService, "No accessibility service");
+	 nsCOMPtr<nsIAccessible> accessible;

if this is fatal, and there is a possibility of the service not being there,
should we handle this with a if (!accService)...?

stuff like this:
   if (mNextSibling) {
-    *aAccNextSibling = mNextSibling;
+    NS_ADDREF(*aAccNextSibling = mNextSibling);
   }
   else {
     *aAccNextSibling = new nsHTMLComboboxListAccessible(mParent, mDOMNode,
mWeakShell);
     if (!*aAccNextSibling)
       return NS_ERROR_OUT_OF_MEMORY;
+    NS_ADDREF(*aAccNextSibling);
     nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(*aAccNextSibling));
     accessNode->Init();
   }
-  NS_ADDREF(*aAccNextSibling);
   return NS_OK;


it seems wrong to be distributing the addref throughout the code - can't you
just NS_IF_ADDREF at the end? setting up vtable calls is potentially expensive,
I don't see any need to duplicate calls in this case... (make sure the *retval
is null in each case of course)

(you have this in a whole bunch of methods)

+#ifdef DEBUG
+  printf("\nATTENTION: New doc accessible for weak shell %x\n", mWeakShell);
+#endif

DEBUG_aaronl instead

Comment 6

[Aaron Leventhal]

[My comments in brackets]

(From update of attachment 122000 [details] [diff] [review])
+  if (!docAccessible) {
+    // No doc accessible yet for this node's document. 
+    // There was probably an accessible event fired before the 
+    // current document was finished loading.
+    // Create a doc accessible so we can cache this node
[I've changed this comment to be more accurate, "before the current document has
been asked for by the assistive technology"]

it sounds like we have code for doing this elsewhere.. (like, after the doc
finishes loading) is this a common pattern that can/should be consolidated into
a helper function?
[That won't save any code, because the other place where it happens all of the
necessary variables already happen to exist. I can turn it into a helper for
readability anyway, if that's preferred]

+	 nsCOMPtr<nsIAccessibilityService> accService = 
+	   do_GetService("@mozilla.org/accessibilityService;1");
+	 NS_ASSERTION(accService, "No accessibility service");
+	 nsCOMPtr<nsIAccessible> accessible;

if this is fatal, and there is a possibility of the service not being there,
should we handle this with a if (!accService)...?
[fixed in new patch]

stuff like this:
   if (mNextSibling) {
-    *aAccNextSibling = mNextSibling;
+    NS_ADDREF(*aAccNextSibling = mNextSibling);
   }
   else {
     *aAccNextSibling = new nsHTMLComboboxListAccessible(mParent, mDOMNode,
mWeakShell);
     if (!*aAccNextSibling)
       return NS_ERROR_OUT_OF_MEMORY;
+    NS_ADDREF(*aAccNextSibling);
     nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(*aAccNextSibling));
     accessNode->Init();
   }
-  NS_ADDREF(*aAccNextSibling);
   return NS_OK;


it seems wrong to be distributing the addref throughout the code - can't you
just NS_IF_ADDREF at the end? setting up vtable calls is potentially expensive,
I don't see any need to duplicate calls in this case... (make sure the *retval
is null in each case of course)
(you have this in a whole bunch of methods)
[Splitting this into 2 NS_ADDREFs actually fixes a crash. The reason I need this
in both places is that since it's not addref'd, and there is an
nsCOMPtr<nsIAccessNode> variable there, it gets released when the block ends,
but before my NS_IF_ADDREF, causing the return argument for the getter to be
wrecked. Alternatively, I could use an NS_STATIC_CAST(nsIAccessNode*, blah) and
avoid the nsCOMptr. That would allow me to have just 1 NS_IF_ADDREF at the end
of each method].

+#ifdef DEBUG
+  printf("\nATTENTION: New doc accessible for weak shell %x\n", mWeakShell);
+#endif

DEBUG_aaronl instead
[fixed]

Comment 7

[Aaron Leventhal]

*** Bug 203843 has been marked as a duplicate of this bug. ***

Comment 8

[Aaron Leventhal]

Attachment: With changes after IM'ing with alecf

Comment 9

[Aaron Leventhal]

Comment on attachment 122126 [details] [diff] [review]
With changes after IM'ing with alecf

Carrying Kyle's r=

Comment 10

[Alec Flett]

Comment on attachment 122126 [details] [diff] [review]
With changes after IM'ing with alecf

sr=alecf

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 122126 [details] [diff] [review]
With changes after IM'ing with alecf

a=sspitzer, since we are talking about crash fixes.

Comment 12

[Aaron Leventhal]

checked in