815397 - Update Accessibility cycle collector traverse/unlink definitions

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: update them

There are a few categories of changes here:
- A number of cases with ambiguous bases can be handled automatically now.
- Similarly with nsTArrays
- Removed a null check that isn't really needed
- I added automated CC support for AccessibleHashtable, and made these fields use the generic definitions.

- After the other changes, a number of the Traverse/Unlink definitions are very generic and can be replaced with a single macro call. Plus, some existing ones can now be replaced, because INHERITED is supported.


It seems a little odd to me that mDependentIDsHash and mNodeToAccessibleMap are cleared in the Unlink of DocAccessible, but aren't Traversed. Not knowing anything about this code, I'd have expected that either these tables be added to the Traverse, or be moved to the destructor. I left that alone.

I haven't actually tested this patch yet.

Comment 2

[Trevor Saunders (:tbsaunde)]

> - I added automated CC support for AccessibleHashtable, and made these
> fields use the generic definitions.

do we have automated support for generic hash tables? if so we might well want to just get rid of the accessible cache thing all together, if not hopefully someday I'll get around to templating that stuff harder so we can cache things as types other than Accessible*

> It seems a little odd to me that mDependentIDsHash and mNodeToAccessibleMap
> are cleared in the Unlink of DocAccessible, but aren't Traversed. Not
> knowing anything about this code, I'd have expected that either these tables
> be added to the Traverse, or be moved to the destructor. I left that alone.

as I remember it they don't hold any refs, so moving the calls to Shutdown() seems like the right thing to do.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> do we have automated support for generic hash tables? if so we might well
> want to just get rid of the accessible cache thing all together, if not
> hopefully someday I'll get around to templating that stuff harder so we can
> cache things as types other than Accessible*

No, we don't. Hash tables are pretty common, so it would be nice to have, though that won't help with PLDHashTables that don't have templated types.

> as I remember it they don't hold any refs, so moving the calls to Shutdown()
> seems like the right thing to do.

Okay, great, I'll look into that.

Comment 4

[Andrew McCreight [:mccr8]]

mDependentIDsHash has type nsClassHashtable<nsStringHashKey, AttrRelProviderArray>.

The latter is defined like this:
  typedef nsTArray<nsAutoPtr<AttrRelProvider> > AttrRelProviderArray;

AttrRelProvider is not a cycle collected class, though it does contain a field:
  nsCOMPtr<nsIContent> mContent;
So maybe it should be cycle collected?

mNodeToAccessibleMap has type nsDataHashtable<nsPtrHashKey<const nsINode>, Accessible*>, so it doesn't own anything and should be okay.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> mDependentIDsHash has type nsClassHashtable<nsStringHashKey,
> AttrRelProviderArray>.
> 
> The latter is defined like this:
>   typedef nsTArray<nsAutoPtr<AttrRelProvider> > AttrRelProviderArray;
> 
> AttrRelProvider is not a cycle collected class, though it does contain a
> field:
>   nsCOMPtr<nsIContent> mContent;
> So maybe it should be cycle collected?

I guess so though I don't see how that member could ever point to something that isn't certainly alive, those nsIContent*s should only be things where IsInDoc() is true.

Comment 6

[Andrew McCreight [:mccr8]]

Okay, great, I'll just leave it alone then.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Could we assert that the mContent is always in a document which is certainly alive?

Comment 8

[Andrew McCreight [:mccr8]]

I can file a followup for that.

Comment 9

[Andrew McCreight [:mccr8]]

Filed bug 815837.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 685366 [details] [diff] [review]
update them


>+inline void
>+ImplCycleCollectionUnlink(AccessibleHashtable &aCache)
AccessibleHashtable& aCache


> 
>+#define NS_IMPL_CYCLE_COLLECTION_NATIVE_0(_class)                              \
>+ NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)                                 \
>+ NS_IMPL_CYCLE_COLLECTION_UNLINK_0(_class)                                     \
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class)                               \
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
I don't see any need for _0
And _0 case should be also very odd case which means probably a bug elsewhere.
Perhaps better to not add it.

Comment 11

[Andrew McCreight [:mccr8]]

True. I'll not add it, and then put a little comment about why.

Comment 12

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95afbeaf4fe9

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/95afbeaf4fe9