Closed Bug 358720 Opened 13 years ago Closed 13 years ago

Crash [@ nsAccessibilityService::GetAccessible]

Categories

(Core :: Disability Access APIs, defect, critical)

1.8 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

(Keywords: access, fixed1.8.1.1)

Attachments

(1 file)

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]
> 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.
(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.
Version: Trunk → 1.8 Branch
Attached patch proposed patchSplinter Review
This should fix it. CreateRootAccessible may return null
nsIAccessible object.
Assignee: aaronleventhal → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #244074 - Flags: review?(aaronleventhal)
Why does it return null in those cases?
Keywords: access
At least when OOM, but I think also during shutdown when document is
being deleted.
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.
Attachment #244074 - Flags: review?(aaronleventhal) → review+
Flags: blocking1.8.1.1?
Comment on attachment 244074 [details] [diff] [review]
proposed patch

Does this need sr+ ?
Attachment #244074 - Flags: superreview?(neil)
Comment on attachment 244074 [details] [diff] [review]
proposed patch

This doesn't need sr=.
Attachment #244074 - Flags: superreview?(neil)
Comment on attachment 244074 [details] [diff] [review]
proposed patch

Sorry, let me correct that. It needs sr= in order to be approved for branch.
Attachment #244074 - Flags: superreview?(neil)
(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?
(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.

Attachment #244074 - Flags: superreview?(neil) → superreview+
Attachment #244074 - Flags: approval1.8.1.1?
Checked in to trunk
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Depends on: 360184
(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

(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 on attachment 244074 [details] [diff] [review]
proposed patch

approved for 1.8 branch, a=dveditz for drivers
Attachment #244074 - Flags: approval1.8.1.1? → approval1.8.1.1+
Keywords: fixed1.8.1.1
No longer depends on: 360184
You need to log in before you can comment on or make changes to this bug.