409474 - Round of crash fixes in a11y

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Working on these crash reports:
http://crash-stats.mozilla.com/?do_query=1&version=Firefox%3A3.0b3pre&version=SeaMonkey%3A2.0a1pre&version=Sunbird%3A0.6a1&version=Thunderbird%3A3.0a1pre&query_search=stack&query_type=contains&query=Accessi&date=&range_value=3&range_unit=weeks

Comment 1

[Aaron Leventhal]

Attachment: 1) A null check, and 2) make sure that we hold on to doc accessible when events are fired on a delay (kung fu death grip)

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 294293 [details] [diff] [review]
1) A null check, and 2) make sure that we hold on to doc accessible when events are fired on a delay (kung fu death grip)

>Index: accessible/src/base/nsCaretAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsCaretAccessible.cpp,v
>retrieving revision 1.56
>diff -p -u -5 -r1.56 nsCaretAccessible.cpp
>--- accessible/src/base/nsCaretAccessible.cpp	27 Nov 2007 18:08:50 -0000	1.56
>+++ accessible/src/base/nsCaretAccessible.cpp	21 Dec 2007 22:44:43 -0000
>@@ -165,10 +165,11 @@ NS_IMETHODIMP nsCaretAccessible::NotifyS
>   NS_ENSURE_TRUE(mRootAccessible, NS_ERROR_FAILURE);
> 
>   mLastUsedSelection = do_GetWeakReference(aSel);
> 
>   nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
>+  NS_ENSURE_TRUE(doc, NS_OK);

If aDoc is null here I would like to add NS_ENSURE_ARG(aDoc)

>+  if (mEventsToFire.Count()) {
>+    // Doc being shut down before events fired,
>+    // so make sure we release the kung fu death grip which is always
>+    // there when there are still events left to be fired
>+    mEventsToFire.Clear();
>+    NS_RELEASE_THIS();

Your idea is to addref the object to prevent crach, nevertheless you release the object even queue is not empty and callback may be called, right?

Comment 3

[Aaron Leventhal]

I can do NS_ENSURE_ARG_POINTER(aDoc)

> Your idea is to addref the object to prevent crach, nevertheless you release
> the object even queue is not empty and callback may be called, right?
This is in Shutdown()

A couple of lines before we do:
if (mFireEventTimer) {
    mFireEventTimer->Cancel();
    mFireEventTimer = nsnull;
  }
  
Let me combine the code a little so it's more obvious that it won't happen.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 294293 [details] [diff] [review]
1) A null check, and 2) make sure that we hold on to doc accessible when events are fired on a delay (kung fu death grip)

r=me

Comment 5

[Aaron Leventhal]

Attachment: More obvious that we won't release and event timer still fires

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 294320 [details] [diff] [review]
More obvious that we won't release and event timer still fires


>+  ClearCache(mAccessNodeCache);
>+
>+  mDocument = nsnull;
>+
>+  nsHyperTextAccessibleWrap::Shutdown();
>+
>   if (mFireEventTimer) {

one point now you prevent callbacks after base Shutdown, not sure if make a difference but some kind of doubts.

in any way I like this approach more and r=me, again :)

Comment 7

[Aaron Leventhal]

> not sure if make a difference but some kind of doubts.
I'm sure this patch will make a difference. I see bad doc accessible pointers used with FlushPendingEvents() in quite a few different stacks.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294320 [details] [diff] [review]
More obvious that we won't release and event timer still fires

a=beltzner for 1.9

Comment 9

[Marco Zehe (:MarcoZ)]

Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3