Last Comment Bug 662243 - Hook up more a11y classes to CC
: Hook up more a11y classes to CC
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 637099
  Show dependency treegraph
 
Reported: 2011-06-06 02:49 PDT by Peter Van der Beken [:peterv]
Modified: 2011-06-09 05:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (11.09 KB, patch)
2011-06-06 02:51 PDT, Peter Van der Beken [:peterv]
dbolter: review+
surkov.alexander: review+
Details | Diff | Review

Description Peter Van der Beken [:peterv] 2011-06-06 02:49:31 PDT
They probably cause various leaks.
Comment 1 Peter Van der Beken [:peterv] 2011-06-06 02:51:16 PDT
Created attachment 537516 [details] [diff] [review]
v1
Comment 2 David Bolter [:davidb] 2011-06-06 06:01:42 PDT
Comment on attachment 537516 [details] [diff] [review]
v1

Alexander should look at this.
Comment 3 David Bolter [:davidb] 2011-06-06 08:46:56 PDT
Comment on attachment 537516 [details] [diff] [review]
v1

Thanks for working on this.

>-NS_IMPL_CYCLE_COLLECTION_0(nsAccessNode)
>+NS_IMPL_CYCLE_COLLECTION_1(nsAccessNode, mContent)

Looks ok to me.

>+NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(nsXULTreeItemAccessibleBase)
>+  NS_INTERFACE_TABLE_INHERITED1(nsXULTreeItemAccessibleBase,
>+                                nsXULTreeItemAccessibleBase)
>+NS_INTERFACE_TABLE_TAIL_INHERITING(nsAccessible)

What do these lines do? (How are the NS_INTERFACE_TABLE* macros designed?)
Comment 4 Peter Van der Beken [:peterv] 2011-06-06 11:15:25 PDT
(In reply to comment #3)
> What do these lines do? (How are the NS_INTERFACE_TABLE* macros designed?)

It just implements QI, it's what NS_IMPL_ISUPPORTS_INHERITED1 expands too (I just added CYCLE_COLLECTION).
Comment 5 alexander :surkov 2011-06-06 21:41:11 PDT
Peter, can you give an idea how we can get a cycle? As far as I know DOM element doesn't keep any references for a11y objects, the same should be valid for XUL tree objects.
Comment 6 Peter Van der Beken [:peterv] 2011-06-07 07:42:32 PDT
Sure:

    0x7fffd9c7d190 [nsXPCWrappedJS (nsITreeView) [1/3]]
        --[]-> 0x7fffd3c622d8 [JS Object (Object) (global=7fffd2e6a678)]
        --[parent]-> 0x7fffd2e6a678 [JS Object (Window) (global=7fffd2e6a678)]
        --[gActionsQueue]-> 0x7fffd3cf9f08 [JS Object (Object) (global=7fffd2e6a678)]
        --[mInvokers]-> 0x7fffd9d523b8 [JS Object (Array) (global=7fffd2e6a678)]
        --[element[3]]-> 0x7fffd3c41680 [JS Object (Object) (global=7fffd2e6a678)]
        --[eventSeq]-> 0x7fffd9d525d8 [JS Object (Array) (global=7fffd2e6a678)]
        --[element[2]]-> 0x7fffd3c41618 [JS Object (Object) (global=7fffd2e6a678)]
        --[check]-> 0x7fffe145e370 [JS Object (Function - check) (global=7fffd2e6a678)]
        --[upvars[0]]-> 0x7fffd3cf9d00 [JS Object (Object) (global=7fffd2e6a678)]
        --[ID]-> 0x7fffe1490160 [JS Object (XPCWrappedNative_NoHelper) (global=7fffd2e6a678)]
        --[xpc_GetJSPrivate(obj)]-> 0x7fffd37da8d0 [XPCWrappedNative]
        --[mIdentity]-> 0x7fffd360e880 [nsAccessNode]
        --[mParent]-> 0x7fffcc7d4530 [nsAccessNode]
        --[mParent]-> 0x7fffcc7d4460 [nsAccessNode]

This is from running a11y mochitests, a treeview with missing incoming edges (3 total, 1 known, 2 missing).
Comment 7 Peter Van der Beken [:peterv] 2011-06-09 02:44:24 PDT
So I guess I'll wait for the second review here.
Comment 8 alexander :surkov 2011-06-09 02:50:12 PDT
I'm on it. To prevent from xpcom digging. What's difference between
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED
and
NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED
Comment 9 alexander :surkov 2011-06-09 03:04:23 PDT
Comment on attachment 537516 [details] [diff] [review]
v1

Review of attachment 537516 [details] [diff] [review]:
-----------------------------------------------------------------

I think nsDocAccessible should be fixed too for mDocument member

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +655,5 @@
> +  NS_INTERFACE_TABLE_INHERITED1(nsXULTreeItemAccessibleBase,
> +                                nsXULTreeItemAccessibleBase)
> +NS_INTERFACE_TABLE_TAIL_INHERITING(nsAccessible)
> +NS_IMPL_ADDREF_INHERITED(nsXULTreeItemAccessibleBase, nsAccessible)
> +NS_IMPL_RELEASE_INHERITED(nsXULTreeItemAccessibleBase, nsAccessible)

do you really need to override QueryInterface since query interface for cycle collection interface is handled by base class?

Would simple NS_IMPL_ISUPPORTS_INHERITED work here?

@@ +1098,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(nsXULTreeItemAccessible)
> +NS_INTERFACE_MAP_END_INHERITING(nsXULTreeItemAccessibleBase)
> +NS_IMPL_ADDREF_INHERITED(nsXULTreeItemAccessible, nsXULTreeItemAccessibleBase)
> +NS_IMPL_RELEASE_INHERITED(nsXULTreeItemAccessible, nsXULTreeItemAccessibleBase)

the same

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +818,5 @@
> +                                nsIAccessibleTableCell,
> +                                nsXULTreeGridCellAccessible)
> +NS_INTERFACE_TABLE_TAIL_INHERITING(nsLeafAccessible)
> +NS_IMPL_ADDREF_INHERITED(nsXULTreeGridCellAccessible, nsLeafAccessible)
> +NS_IMPL_RELEASE_INHERITED(nsXULTreeGridCellAccessible, nsLeafAccessible)

the same
Comment 10 Peter Van der Beken [:peterv] 2011-06-09 03:31:04 PDT
(In reply to comment #8)
> I'm on it. To prevent from xpcom digging. What's difference between
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED
> and
> NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED

See http://hg.mozilla.org/mozilla-central/annotate/fd50260987f4/xpcom/glue/nsISupportsImpl.h#l480.

(In reply to comment #9)
> I think nsDocAccessible should be fixed too for mDocument member

OK.

> 
> ::: accessible/src/xul/nsXULTreeAccessible.cpp
> @@ +655,5 @@
> > +  NS_INTERFACE_TABLE_INHERITED1(nsXULTreeItemAccessibleBase,
> > +                                nsXULTreeItemAccessibleBase)
> > +NS_INTERFACE_TABLE_TAIL_INHERITING(nsAccessible)
> > +NS_IMPL_ADDREF_INHERITED(nsXULTreeItemAccessibleBase, nsAccessible)
> > +NS_IMPL_RELEASE_INHERITED(nsXULTreeItemAccessibleBase, nsAccessible)
> 
> do you really need to override QueryInterface since query interface for
> cycle collection interface is handled by base class?

Well, in this case QI was already overridden here (using NS_IMPL_ISUPPORTS_INHERITED1). But I guess your question is if the overridden QI needs to handle cycle collection. It needs to because nsXULTreeItemAccessibleBase has its own cycle collection code, so it needs to return its own CC participant instead of the participant of the base class, otherwise we'll only traverse/unlink the members of the base class.

> Would simple NS_IMPL_ISUPPORTS_INHERITED work here?

No.
Comment 11 alexander :surkov 2011-06-09 03:36:38 PDT
Comment on attachment 537516 [details] [diff] [review]
v1

r=me, thank you!
Comment 12 alexander :surkov 2011-06-09 05:27:34 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/8f30c4d06724

Note You need to log in before you can comment on or make changes to this bug.