352105 - Firefox core dumped when starting with orca.

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Tim Miao]

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.

Comment 1

[Tim Miao]

Attachment: This is the stack trace.

Comment 2

[Ginn Chen]

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.

Comment 3

[Ginn Chen]

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?

Comment 4

[Aaron Leventhal]

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.

Comment 5

[Ginn Chen]

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

[Aaron Leventhal]

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.

Comment 7

[Ginn Chen]

Attachment: 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.

Comment 8

[Aaron Leventhal]

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.

Comment 9

[Ginn Chen]

Attachment: patch v2

patch revised.
please review again in case I missed something

Comment 10

[Aaron Leventhal]

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.