Firefox core dumped when starting with orca.

RESOLVED FIXED

Status

()

Firefox
Disability Access
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Tim Miao, Assigned: Ginn Chen)

Tracking

({access, crash})

Trunk
x86
SunOS
access, crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

11.53 KB, text/plain
Details
11.72 KB, patch
Aaron Leventhal
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
This bug makes firefox crashes, please fix it.

Steps to reproduce:
1. Enable the desktop assistive support.
2. Run orca 1.0.0.
3. Invoke firefox trunk build.

Expected result:
Firefox starts up successfully.

Actual result:
Firefox crashes.

Additional info:
This bug can be found with Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a1) Gecko/20060911 Minefield/3.0a1 and orca 1.0.0 on solaris nevada build 46.
Firefox will not crahs when the orca is not running.

Stacktrace:
See the attacked file please.
(Reporter)

Comment 1

12 years ago
Created attachment 237707 [details]
This is the stack trace.
(Reporter)

Updated

12 years ago
Summary: Firefox core dumped when working with orca. → Firefox core dumped when starting with orca.
(Assignee)

Comment 2

12 years ago
Similar to Bug 348197
nsAccessNode.cpp:504, gGlobalDocAccessibleCache.Get returns null for accessNode
nsDocAccessible::Init() is not yet called for that mWeakShell.

Very easy to reproduce.
Blocks: 342901
Keywords: crash
(Assignee)

Comment 3

12 years ago
chrome:://.../browser.xul EMBEDS the document frame.
But the accessible object of the document frame is not created yet.

Orca tries to get relations for every atkobject.

Aaron,
do you think we should add a null check or create document accessible here?

Assignee: nobody → aaronleventhal

Comment 4

12 years ago
Good question. I think we always create the doc accessible as soon as the content is starting to load, from this code:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#161

Why has the doc accessible not already been created? Is this happening even before the first document begins to load? If so, then trying to create the doc accessible too early might not be helpful -- it might get torn down anyway when the real new document is ready.

We should definitely null check at least. But you will have to investigate the answers to the questions above before we can decide whether to also do a GetAccessibleFor() instead of getting the cached doc accessible.

Updated

12 years ago
Assignee: aaronleventhal → ginn.chen
(Assignee)

Comment 5

12 years ago
Sometimes if the doc is not completely loaded, if will crash.
I can reproduce it with at-poke.

Another issue for EMBEDS:
Start firefox, browse www.sun.com in first tab, browse www.ibm.com in second tab.
Wait till them loaded.
Start at-poke, the top level accessible has relation EMBEDS "IBM United States". 
Switch to first tab, try again, it still EMBEDS "IBM United States".
Close second tab, try again, it still EMBEDS "IBM United States".

And sometimes I saw it has a EMBEDS relation but label is empty.

Comment 6

12 years ago
Wow, good catch.

I think it's okay to create the doc accessible if it doesn't already exist. We'll still need the null check. And of course, find out why it's reporting the wrong tab. That's very bad.
(Assignee)

Comment 7

12 years ago
Created attachment 238187 [details] [diff] [review]
patch

1) Issue in comment #5 is caused by refRelationSetCB, it doesn't refresh the relations, also we missed a g_object_unref.

2) Sort includes in nsRootAccessible.cpp

3) Change to GetAccessibleFor and add a null check.
Attachment #238187 - Flags: review?(aaronleventhal)

Comment 8

12 years ago
Comment on attachment 238187 [details] [diff] [review]
patch

Could you file a follow-up bug to make GetAccessibleRelated() return an array of accessibles, and map that into ATK?
For MSAA/accNavigate we'll just return the first for now.
In fact it might be less duplicated just to fix that now with this bug, but I'll leave that up to you.

static +GetDOMNodeFromDocShellTreeItem(nsIDocShellTreeItem *aTreeItem)
Should be already_AddRefed<>

Another possibility is just to add a new optional boolean argument to nsAccessNode::GetDocAccessibleFor(nsISupports*, PRBool aCanCreate)

I like putting these static helpers on nsAccessNode so that we have easy access to them and know where to find them. I should have done that for GetContentDocShell(). Could you do that for both static helpers, perhaps put them next to GetDocShellTreeItemFor(nsIDOMNode*) ? 

Can you make it take an nsISupports instead of an nsIDocShellTreeItem? That way you can pass in a tree item or a docshell, etc. Call it GetDOMNodeForContainer().

Also, I suggest null checking the node that's returned before using GetAccessibleFor() on it.


Also, are there any places in mozilla/accessible that can benefit from the new helper?


I don't think the temporary accessible is needed any longer.
Perhaps:    GetAccService()->GetAccessibleFor(node, aRelated);  
            return NS_OK;

    nsCOMPtr<nsIAccessible> accessible; 
    GetAccService()->GetAccessibleFor(node, getter_AddRefs(accessible));  
    if (accessible) {  
      accessible->QueryInterface(NS_GET_IID(nsIAccessible),
                                 (void**)aRelated);

No further review needed unless you think it's a good idea.
Attachment #238187 - Flags: review?(aaronleventhal) → review+

Updated

12 years ago
Depends on: 345780
(Assignee)

Comment 9

12 years ago
Created attachment 238354 [details] [diff] [review]
patch v2

patch revised.
please review again in case I missed something
Attachment #238187 - Attachment is obsolete: true
Attachment #238354 - Flags: review?(aaronleventhal)

Comment 10

12 years ago
Comment on attachment 238354 [details] [diff] [review]
patch v2

Have you checked to make sure the new MAI code doesn't leak?

Nit: why 2 methods here instead of 1 with an optional aCanCreate argument which defaults to fauls?
     static already_AddRefed<nsIAccessibleDocument> GetDocAccessibleFor(nsISupports *aContainer);
+    static already_AddRefed<nsIAccessibleDocument> GetDocAccessibleFor(nsISupports *aContainer, PRBool aCanCreate);

Nit: the GetDocAccessibleFor() used in GetAccessibleRelated() should never return null now. You could assert that if you want, but no big deal.

I guess I'm in a picky mood today :) Sorry.
Attachment #238354 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
No longer depends on: 345780
You need to log in before you can comment on or make changes to this bug.