1390692 - Double checking for nsCycleCollectionISupports in CC macros

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

I was looking over the patch for bug 1384821 and it seems like we end up double checking for nsCycleCollectionISupports in two places, now that NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION checks for both interfaces. NS_IMPL_QUERY_CYCLE_COLLECTION_ISUPPORTS is only needed for nsAgg.h, so maybe I'll move that there for less confusion in the future.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1390692, part 1 - Convert nsAgg to use the new QI macro.

This just does the same thing as the regular CC QI macro.

This gets rid of the last use of NS_IMPL_QUERY_CYCLE_COLLECTION, so
remove it.

Review commit: https://reviewboard.mozilla.org/r/168944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168944/

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1390692 - Remove the unnecessary second QI entry for nsCycleCollectionISupports.

ENTRY_CYCLE_COLLECTION now implements the QI code for both CC
interfaces, so we don't need to separately check for
nsCycleCollectionISupports afterwards.

This patch also renames NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION to
NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION to reflect that it now
handles the case for two interfaces.

It also changes NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED to
be defined in terms of NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION to make
it clearer that they are identical.

Finally, this patch deletes some macros that are no longer used in the
wake of the previous changes.

Review commit: https://reviewboard.mozilla.org/r/168946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168946/

Comment 3

[(no longer active)]

Thanks a lot for this, especially for the first part!  I spent a while trying to understand why nsAgg.h macros look so different and your patch is quite telling.  :-)

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 8897665 [details]
Bug 1390692, part 1 - Convert nsAgg to use the new QI macro.

https://reviewboard.mozilla.org/r/168944/#review176222

-ing this because I'd rather clean this up than keep incorrect code.

::: xpcom/base/nsAgg.h
(Diff revision 1)
>  
>  #define NS_INTERFACE_MAP_BEGIN_AGGREGATED(_class)                           \
>    NS_IMPL_AGGREGATED_QUERY_HEAD(_class)
>  
> -#define NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION_AGGREGATED(_class)          \
> -  NS_IMPL_QUERY_CYCLE_COLLECTION(_class)

I think we should try to remove the cycle collection support for aggregated objects. AFAICT it's only used for nsInMemoryDataSource. Also, trying to understand this I think this line should have been |NS_IMPL_AGGREGATED_QUERY_CYCLE_COLLECTION(\_class)|. This would never have actually worked correctly if we made the outer's CC participant try to traverse/unlink the inner's members.
I don't know what nsInMemoryDataSource is aggregated with, we should figure that out. If any of its outers participate in CC then we can just move NS_IMPL_AGGREGATED_QUERY_CYCLE_COLLECTION's code into the QI implementation for nsInMemoryDataSource, but we need to fix up the outer too to actually use this code, because we're probably leaking. If not then we just us the normal CC macros (but maybe also assert that the outer doesn't participate in CC).

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 8897666 [details]
Bug 1390692 - Remove the unnecessary second QI entry for nsCycleCollectionISupports.

https://reviewboard.mozilla.org/r/168946/#review176224

Hmm, not sure how I missed the second QI entry. Thanks for cleaning this up.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8897666 [details]
Bug 1390692 - Remove the unnecessary second QI entry for nsCycleCollectionISupports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168946/diff/1-2/

Comment 7

[Andrew McCreight [:mccr8]]

I don't know anything about aggregated objects, so I'll just land the changes to remove the extra QI entry and move the macros to nsAgg.h that are only used there. I'll file a new bug about comment 4.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #5)
> Hmm, not sure how I missed the second QI entry. Thanks for cleaning this up.

You were probably focused on the actual interesting part of the patch and not the macro soup. :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a97cc606c12f
Remove the unnecessary second QI entry for nsCycleCollectionISupports. r=peterv

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a97cc606c12f