Fix duplicate refcnt member errors that clang plugin reports on Windows

RESOLVED FIXED in Firefox 53

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ting, Assigned: ting)

Tracking

Trunk
mozilla53
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8817349 [details]
wip
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8817349 - Attachment is obsolete: true
I am not sure do we need

  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WeakJumpListItem)

in

  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(JumpListShortcut)
    NS_INTERFACE_MAP_ENTRY(nsIJumpListShortcut)
  NS_INTERFACE_MAP_END

Comment 6

a year ago
mozreview-review
Comment on attachment 8818217 [details]
Bug 1322459 part 1 - Remove duplicate mRefCnt in CaptureSinkFilter.

https://reviewboard.mozilla.org/r/98358/#review98666
Attachment #8818217 - Flags: review?(ehsan) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8818218 [details]
Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

https://reviewboard.mozilla.org/r/98360/#review98668

::: widget/windows/JumpListItem.h:58
(Diff revision 1)
> +class JumpListItem : public WeakJumpListItem
> +{
> +public:
> +  using WeakJumpListItem::WeakJumpListItem;
> +
> +  NS_DECL_ISUPPORTS

Instead of adding this in a mixin class, you should remove this mixin class altogether, and add the NS_DECL_ISUPPORTS and friends to each individual leaf class in the hierarchy.  Without that, using such mixin classes in the multi-inheritance scenario is risky since you can still end up with a duplicate mRefCnt through two of your base classes.
Attachment #8818218 - Flags: review?(ehsan) → review-

Comment 8

a year ago
mozreview-review
Comment on attachment 8818219 [details]
Bug 1322459 part 3 - Remove duplicate mRefCnt in LSPAnnotationGatherer.

https://reviewboard.mozilla.org/r/98362/#review98670
Attachment #8818219 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #7)
> Instead of adding this in a mixin class, you should remove this mixin class
> altogether, and add the NS_DECL_ISUPPORTS and friends to each individual
> leaf class in the hierarchy.  Without that, using such mixin classes in the
> multi-inheritance scenario is risky since you can still end up with a
> duplicate mRefCnt through two of your base classes.

Do you mean to remove JumpListItem?

If yes, then we won't be able to create an instance of nsIJumpListItem which would be different from original code.

Comment 10

a year ago
No, I mean doing something like this:

1. Remove NS_DECL_ISUPPORTS from JumpListItem.
2. Rename JumpListItem to JumpListItemBase or some such.
3. Create a new class named JumpListItem derived from JumpListItemBase.
4. Add NS_DECL_ISUPPORTS and friends to the new JumpListItem, as well as the rest of the leaf classes (JumpListSeparator, JumpListLink, and JumpListShortcut.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8818218 [details]
Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

https://reviewboard.mozilla.org/r/98360/#review99140

::: widget/windows/JumpListItem.h:58
(Diff revision 2)
> -class JumpListSeparator : public JumpListItem, public nsIJumpListSeparator
> +class JumpListItem : public JumpListItemBase
> +{
> +  ~JumpListItem() {}
> +
> +public:
> +  using JumpListItemBase::JumpListItemBase;

Hm, what's this? :-)  You can't usefully import the constructor name for the base class here, since you wouldn't be able to call that function.  Honestly I'm surprised the compiler even accepts this!!
Attachment #8818218 - Flags: review?(ehsan) → review+
Comment hidden (obsolete)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8818218 [details]
Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

https://reviewboard.mozilla.org/r/98360/#review99140

> Hm, what's this? :-)  You can't usefully import the constructor name for the base class here, since you wouldn't be able to call that function.  Honestly I'm surprised the compiler even accepts this!!

It's inheriting constructors from JumpListItemBase, so JumpListItem still has the constructors in original version.
If there's no other concern with the inheriting constructors, I'll mark the review issue fixed and land the patches, thanks.

Comment 17

a year ago
Neat, I didn't know about this trick!  Looks good.

Comment 18

a year ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b207d21ee5a5
part 1 - Remove duplicate mRefCnt in CaptureSinkFilter. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/fd91d5daa174
part 2 - Remove duplicate mRefCnt in JumpListShortCut. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/0ef8a1d7e4d6
part 3 - Remove duplicate mRefCnt in LSPAnnotationGatherer. r=Ehsan

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b207d21ee5a5
https://hg.mozilla.org/mozilla-central/rev/fd91d5daa174
https://hg.mozilla.org/mozilla-central/rev/0ef8a1d7e4d6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee: nobody → janus926
You need to log in before you can comment on or make changes to this bug.