If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Double checking for nsCycleCollectionISupports in CC macros

RESOLVED FIXED in Firefox 57

Status

()

Core
XPCOM
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a month ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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.  :-)
(Assignee)

Updated

a month ago
Blocks: 1391005

Comment 4

a month ago
mozreview-review
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).
Attachment #8897665 - Flags: review?(peterv) → review-

Comment 5

a month ago
mozreview-review
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.
Attachment #8897666 - Flags: review?(peterv) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8897665 - Attachment is obsolete: true
(Assignee)

Comment 7

a month ago
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.
(Assignee)

Updated

a month ago
See Also: → bug 1392759
(Assignee)

Comment 8

a month ago
(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

a month ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a97cc606c12f
Remove the unnecessary second QI entry for nsCycleCollectionISupports. r=peterv
https://hg.mozilla.org/mozilla-central/rev/a97cc606c12f
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.