Hook up more a11y classes to CC

RESOLVED FIXED in mozilla7

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
They probably cause various leaks.
(Assignee)

Comment 1

6 years ago
Created attachment 537516 [details] [diff] [review]
v1
Attachment #537516 - Flags: review?(bolterbugz)
Comment on attachment 537516 [details] [diff] [review]
v1

Alexander should look at this.
Attachment #537516 - Flags: review?(surkov.alexander)
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?)
(Assignee)

Comment 4

6 years ago
(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).

Updated

6 years ago
Attachment #537516 - Flags: review?(bolterbugz) → review+

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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).
(Assignee)

Comment 7

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

Comment 8

6 years ago
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

6 years ago
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
(Assignee)

Comment 10

6 years ago
(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

6 years ago
Comment on attachment 537516 [details] [diff] [review]
v1

r=me, thank you!
Attachment #537516 - Flags: review?(surkov.alexander) → review+

Comment 12

6 years ago
landed - http://hg.mozilla.org/mozilla-central/rev/8f30c4d06724
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.