358720 - Crash [@ nsAccessibilityService::GetAccessible]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently #40 in FF2 topcrashers list.

nsAccessibilityService::GetAccessible  [mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1829]
nsAccessibilityService::GetAccessibleInShell  [mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1720]
nsAccessNode::Init  [mozilla/accessible/src/base/nsAccessNode.cpp, line 128]
nsAccessible::Init  [mozilla/accessible/src/base/nsAccessible.cpp, line 419]
nsAccessibilityService::InitAccessible  [mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1743]

Comment 1

[alexander :surkov (:asurkov)]

> nsAccessibilityService::GetAccessible 
> [mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1829]

What do this mean? nsAccessibilityService.cpp haven't so many lines. I can assume only it's not on trunk.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> Currently #40 in FF2 topcrashers list.

(In reply to comment #1)
> > nsAccessibilityService::GetAccessible 
> > [mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1829]
> 
> What do this mean? nsAccessibilityService.cpp haven't so many lines. I can
> assume only it's not on trunk.
> 

Sorry, I can see.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

This should fix it. CreateRootAccessible may return null
nsIAccessible object.

Comment 4

[Aaron Leventhal]

Why does it return null in those cases?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

At least when OOM, but I think also during shutdown when document is
being deleted.

Comment 6

[Aaron Leventhal]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

Null check is fine but where's the stack trace? There's a deeper issue that we shouldn't be trying to create a doc or root accessible for an object getting torn down.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> (From update of attachment 244074 [details] [diff] [review] [edit])
> Null check is fine but where's the stack trace? 

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsAccessibilityService::GetAccessible&vendor=MozillaOrg&product=Firefox2&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid
;)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

Does this need sr+ ?

Comment 9

[Aaron Leventhal]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

This doesn't need sr=.

Comment 10

[Aaron Leventhal]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

Sorry, let me correct that. It needs sr= in order to be approved for branch.

Comment 11

[Ginn Chen]

(In reply to comment #3)
> Created an attachment (id=244074) [edit]
> proposed patch

-      NS_ASSERTION(newAcc, "No root/doc accessible created");
+      NS_WARN_IF_FALSE(newAcc, "No root/doc accessible created");

what's different?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> 
> -      NS_ASSERTION(newAcc, "No root/doc accessible created");
> +      NS_WARN_IF_FALSE(newAcc, "No root/doc accessible created");
> 
> what's different?
> 

Assertion is assertion, warning just a warning :)
Assertions may abort, at least in some tboxes.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Checked in to trunk

Comment 14

[Evan Yan]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

>+  *aFrameHint = nsnull;

why we set *aFrameHint null here? does this have anything to do with this bug?

I don't understand this part of code much, but this change would make this "if()" statement meaningless:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1166

Comment 15

[Evan Yan]

(In reply to comment #14)
> >+  *aFrameHint = nsnull;
> 
> why we set *aFrameHint null here? does this have anything to do with this bug?
> 
that introduced bug 360184

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> (In reply to comment #14)
> > >+  *aFrameHint = nsnull;
> > 
> > why we set *aFrameHint null here? does this have anything to do with this bug?
> > 
> that introduced bug 360184
> 

Sorry, my bad. I didn't realize that aFrameHint is |inout| parameter
http://lxr.mozilla.org/seamonkey/source/accessible/public/nsIAccessibilityService.idl#82

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 244074 [details] [diff] [review]
proposed patch

approved for 1.8 branch, a=dveditz for drivers