1347040 - nsISupports::QueryInterface hides IUnknown::QueryInterface and vice versa

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[(Away)]

grep for -Woverloaded-virtual (ignore DOMException.h) in: https://public-artifacts.taskcluster.net/ZdJ85s5bRF6VCZ_fkskBaw/0/public/logs/live_backing.log

e.g.

02:24:10     INFO -  z:/task_1489456234/build/src/accessible/windows/msaa\DocAccessibleWrap.h(21,3):  warning: 'mozilla::a11y::DocAccessibleWrap::QueryInterface' hides overloaded virtual function [-Woverloaded-virtual]
02:24:10     INFO -    DECL_IUNKNOWN_INHERITED
02:24:10     INFO -    ^
02:24:10     INFO -  z:/task_1489456234/build/src/accessible/windows/msaa/IUnknownImpl.h(76,35):  note: expanded from macro 'DECL_IUNKNOWN_INHERITED'
02:24:10     INFO -  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID, void**);              \
02:24:10     INFO -                                    ^
02:24:10     INFO -  z:/task_1489456234/build/src/accessible/generic/DocAccessible.h(46,3):  note: hidden overloaded virtual function 'mozilla::a11y::DocAccessible::QueryInterface' declared here: type mismatch at 1st parameter ('const nsIID &' (aka 'const nsID &') vs 'const IID &' (aka 'const _GUID &'))
02:24:10     INFO -    NS_DECL_ISUPPORTS_INHERITED
02:24:10     INFO -    ^
02:24:10     INFO -  z:/task_1489456234/build/src/obj-firefox/dist/include\nsISupportsImpl.h(949,14):  note: expanded from macro 'NS_DECL_ISUPPORTS_INHERITED'
02:24:10     INFO -    NS_IMETHOD QueryInterface(REFNSIID aIID,                                    \
02:24:10     INFO -               ^

Comment 1

[Trevor Saunders (:tbsaunde)]

yeah, this is known, there are some pretty old bugs about it.  However it isn't particularly simple to fix since you need to rearrange a bunch of inheritance.

I guess this is a clang-cl build? it would probably make sense to disable this warning there like we do for mingw gcc at AccessibleWrap.h:21.

Comment 2

[(Away)]

Yep, it's a clang-cl build.

I'm not thrilled about disabling the warning since it pretty much guarantees we'll never fix this.

Out of curiosity, why does it require re-arranging inheritance? Don't you just need to add the missing QI's?

Comment 3

[Trevor Saunders (:tbsaunde)]

> I'm not thrilled about disabling the warning since it pretty much guarantees
> we'll never fix this.

well, until we have another reason to or we happen to by finally working on this class design, but that's understandable.

> Out of curiosity, why does it require re-arranging inheritance? Don't you
> just need to add the missing QI's?

that might work, but I'm kind of weary of touching that code because we don't have good tests for the platform integration stuff, and the argument types are so similar that I worry about accidentally changing which QI gets called in a given spot.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm not the best person to triage this bug... speculating it should go in our backlog (P3)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

Attachment: Bug 1347040 - Prevent -Woverloaded-virtual of AccessibleWrap.h when __clang__ defined

After switching compiler from VC++ to clang-cl on Windows, I see the following
warnings when I write patches for editor/libeditor:

DocAccessibleWrap.h(21,3):  warning:
 'mozilla::a11y::DocAccessibleWrap::QueryInterface' hides overloaded virtual
 function
RootAccessible.h(22,3):  warning:
 'mozilla::a11y::RootAccessible::QueryInterface' hides overloaded virtual
function

Those classes declare and implement nsISupports::QueryInterface() but
their base classes have IUnknown::QueryInterface() too.  This patch adds
|using BaseClass::QueryInterface;| to avoid these warnings.

And I also see this warning when I rebase to the latest mozilla-central:

RootAccessibleWrap.h:52:3:  warning:
 'mozilla::a11y::RootAccessibleWrap::QueryInterface' hides overloaded virtual
 function

This class declare and implement IUnknown::QueryInterface() but its base class
has nsISupports::QueryInterface() too.  This patch adds same line to avoid
this warning.

Comment 6

[Masatoshi Kimura [:emk]]

This change may lead accidental use IUnknown::QueryInterface() from RootAccessible or nsISupports::QueryInterface() RootAccessibleWrap. Currently we are confident about that there is no such usage because it would have caused a compile error. Is this change really desired? It would be better to suppress [-Woverloaded-virtual] IMO.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> This change may lead accidental use IUnknown::QueryInterface() from
> RootAccessible or nsISupports::QueryInterface() RootAccessibleWrap.
> Currently we are confident about that there is no such usage because it
> would have caused a compile error. Is this change really desired? It would
> be better to suppress [-Woverloaded-virtual] IMO.

Hmm, or should make them private? I assume that nobody does QI in their instances.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd03f4403e290804f3cd5136e9adc9fbba666ac

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

I wonder, if the other QueryInterface() is accidentally matched, is that mean that the compiler won't allow the overload?

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> I wonder, if the other QueryInterface() is accidentally matched, is that
> mean that the compiler won't allow the overload?

Sorry, I don't understand your question.

Currently we don't have to remember that we cannot pass REFIID to RootAccessible::QueryInterface() or cannot pass REFNSIID to RootAccessibleWrap::QueryInterface() because compilers will produce errors for us. With this patch, compilers will no longer detect the error.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

(In reply to Masatoshi Kimura [:emk] from comment #10)
> Currently we don't have to remember that we cannot pass REFIID to
> RootAccessible::QueryInterface() or cannot pass REFNSIID to
> RootAccessibleWrap::QueryInterface() because compilers will produce errors
> for us. With this patch, compilers will no longer detect the error.

I thought that you worry about that when developers want to call nsISupport::QueryInterface(), IUnknown::QueryInterface() may be called with something wrong reason, e.g., future compiler's bugs or changes.

But I understand, you worry about mistakes of people who hack around here. I believe that making the QueryInterface() methods which are currently hidden should be non-public members of them even though they can be accessed via base class pointer/reference. I think that we should not disable warnings as far as possible.

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #11)
> But I understand, you worry about mistakes of people who hack around here. I
> believe that making the QueryInterface() methods which are currently hidden
> should be non-public members of them even though they can be accessed via
> base class pointer/reference.

It is not possible to make IUnknown::QueryInterface() private in RootAccessible because RootAccessibleWrap (that derives RootAccessible) have to override IUnknown::QueryInterface().

> I think that we should not disable warnings as
> far as possible.

Clang does not enable this warning by default (needs -Wall). Not all warnings indicate a real problem. Sometimes it even makes things worse to blindly change the code semantics only for making the compiler silent. This warning would have been useful in C++03 era, but now the `override` keyword will catch the error more reliably.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

I found this:
https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/accessible/windows/msaa/AccessibleWrap.h#23-28
I'll update the patch.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=435cc137fcb06732528cd38add150ae3d8ec8cf1

Comment 15

[James Teh [:Jamie]]

Comment on attachment 9004143 [details]
Bug 1347040 - Prevent -Woverloaded-virtual of AccessibleWrap.h when __clang__ defined

James Teh [:Jamie] has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f6244fb0d66
Prevent -Woverloaded-virtual of AccessibleWrap.h when __clang__ defined r=Jamie

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/0f6244fb0d66