662243 - Hook up more a11y classes to CC

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Peter Van der Beken [:peterv]]

They probably cause various leaks.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 537516 [details] [diff] [review]
v1

Alexander should look at this.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

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]]

(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 (:asurkov)]

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]]

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]]

So I guess I'll wait for the second review here.

Comment 8

[alexander :surkov (:asurkov)]

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 (:asurkov)]

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]]

(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 (:asurkov)]

Comment on attachment 537516 [details] [diff] [review]
v1

r=me, thank you!

Comment 12

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/8f30c4d06724