1322459 - Fix duplicate refcnt member errors that clang plugin reports on Windows

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: wip

Comment 2

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1322459 part 1 - Remove duplicate mRefCnt in CaptureSinkFilter.

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

Comment 3

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

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

Comment 4

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1322459 part 3 - Remove duplicate mRefCnt in LSPAnnotationGatherer.

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

Comment 5

[Ting-Yu Chou [:ting] (away)]

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

[(no longer active)]

Comment on attachment 8818217 [details]
Bug 1322459 part 1 - Remove duplicate mRefCnt in CaptureSinkFilter.

https://reviewboard.mozilla.org/r/98358/#review98666

Comment 7

[(no longer active)]

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.

Comment 8

[(no longer active)]

Comment on attachment 8818219 [details]
Bug 1322459 part 3 - Remove duplicate mRefCnt in LSPAnnotationGatherer.

https://reviewboard.mozilla.org/r/98362/#review98670

Comment 9

[Ting-Yu Chou [:ting] (away)]

(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

[(no longer active)]

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 11

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8818218 [details]
Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

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

Comment 12

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8818219 [details]
Bug 1322459 part 3 - Remove duplicate mRefCnt in LSPAnnotationGatherer.

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

Comment 13

[(no longer active)]

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!!

Comment 14

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8818218 [details]
Bug 1322459 part 2 - Remove duplicate mRefCnt in JumpListShortCut.

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

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

It's inheriting constructors from JumpListItemBase, so JumpListItem still has the constructors in original version.

Comment 15

[Ting-Yu Chou [:ting] (away)]

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.

Comment 16

[Ting-Yu Chou [:ting] (away)]

If there's no other concern with the inheriting constructors, I'll mark the review issue fixed and land the patches, thanks.

Comment 17

[(no longer active)]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b207d21ee5a5
https://hg.mozilla.org/mozilla-central/rev/fd91d5daa174
https://hg.mozilla.org/mozilla-central/rev/0ef8a1d7e4d6