Using STDMETHOD when implementing COM interfaces

[Andrew Scherkus]

If you don't care about Windows code in Chromium, move on!

Context: implementing IMMNotificationClient

https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_device_listener_win.h

Other than chrome_frame, there didn't seem to be much precedence for implementing COM interfaces in Chromium. I noticed two styles:

1) STDMETHOD macro (used in ./chrome_frame)

class Foo : public IComIsFun {

  STDMETHOD_(ULONG, AddRef)();

  STDMETHOD_(ULONG, Release)();

  STDMETHOD(IsFun)(BOOL* fun);

};

STDMETHODIMP_(ULONG) Foo::AddRef() { ... }

STDMETHODIMP_(ULONG) Foo::Release() { ... }

STDMETHODIMP Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

2) Manually expanded macro (used in ./ui and some other places)

class Foo : public IComIsFun {

  ULONG __stdcall AddRef();

  ULONG __stdcall Release();

  HRESULT __stdcall IsFun(BOOL* fun);

};

ULONG Foo::AddRef() { ... }

ULONG Foo::Release() { ... }

HRESULT Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

As someone who doesn't deal with COM I'm inclined towards (2) as I have no idea what the macros mean and they look funny/useless in the .cc file. It seems the primary use for these macros is C/C++ interoperability [1], which isn't a concern for Chromium. That being said, I'm sure some seasoned Windows developers might say otherwise.

So... what say you, seasoned Windows developers? Roll with the COM macros? Use whatever surrounding code is using? Not worth worrying about?

Andrew

[1] http://blogs.msdn.com/b/oldnewthing/archive/2004/10/05/238050.aspx

[Darin Fisher]

I've always preferred #2.  Macros that mask C++ syntax always bother me.

-Darin

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Dominic Mazzoni]

The accessibility code implements COM interfaces too:

content/browser/accessibility/browser_accessibility_win.h

ui/views/accessibility/native_view_accessibility_win.h

One argument in favor of the macros is that it exactly matches the header file and the MSDN documentation. It's a lot easier to spot-check to see that a method you're trying to override is defined as STDMETHODIMP in the docs, rather than trying to remember if STDMETHODIMP returns a LONG or ULONG. Also, LONG isn't very meaningful, it could be anything - to a Windows developer, STDMETHODIMP strongly implies that this is a COM interface that returns an error code (S_OK, etc.).

- Dominic

[Andrew Scherkus]

On Mon, Oct 22, 2012 at 4:20 PM, Darin Fisher <da...@chromium.org> wrote:

I've always preferred #2.  Macros that mask C++ syntax always bother me.

Yeah that's my gut feeling as well.

Thinking this through a bit more... my intent isn't to rewrite a bunch of of .h/cc code so how about the following proposal/guideline:

"Be consistent with surrounding code. Windows-centric code w/ lots of COM and experienced Windows developers writing/reviewing the code? Consider #1. Chromium-centric code w/ a small smattering of COM? Consider #2."

Andrew

[Michael Courage]

I don't particularly like the macros, but #1 is immediately recognizable as a COM interface, and is not going to look funny at all to developers who spend a lot of time with COM.

On Mon, Oct 22, 2012 at 4:20 PM, Darin Fisher <da...@chromium.org> wrote:

[Dmitry Titov]

"Seasoned Windows developers" probably saw many horrible things in their life...  :-)

I think most of those macros, which are very old, were intended to smooth differences between quickly changing compilers/environments of that crazy time and to account for hardware limitations which we don't have these days. A few years back in at least some parts of MS the new code was mostly written in style #2, without "__stdcall".

However, by that time the macro gods invented SAL and PREFix...

[Peter Kasting]

On Mon, Oct 22, 2012 at 4:37 PM, Dominic Mazzoni <dmaz...@google.com> wrote:

One argument in favor of the macros is that it exactly matches the header file and the MSDN documentation. It's a lot easier to spot-check to see that a method you're trying to override is defined as STDMETHODIMP in the docs, rather than trying to remember if STDMETHODIMP returns a LONG or ULONG. Also, LONG isn't very meaningful, it could be anything - to a Windows developer, STDMETHODIMP strongly implies that this is a COM interface that returns an error code (S_OK, etc.).

There are a few semi-related bits from the style guide, though neither directly addresses this case.

(1) "Windows defines many of its own synonyms for primitive types, such as DWORD, HANDLE, etc. It is perfectly acceptable, and encouraged, that you use these types when calling Windows API functions. Even so, keep as close as you can to the underlying C++ types. For example, use const TCHAR * instead of LPCTSTR."

(2) "Do not use any nonstandard extensions, like #pragma and __declspec, unless you absolutely must. Using __declspec(dllimport) and __declspec(dllexport) is allowed; however, you must use them through macros such as DLLIMPORT and DLLEXPORT, so that someone can easily disable the extensions if they share the code."

(1) suggests that in general we'd prefer to avoid layering macros in when unnecessary.  (2) suggests the opposite but only applies for code that can be reused outside the context of Windows, which isn't going to apply for COM implementations.

Therefore, I'd suggest that "ULONG __stdcall AddRef()" is more style-guide-compliant than "STDMETHOD_(ULONG, AddRef)();".  It's certainly what I'd ask people to do in reviews.

Incidentally, I have an unrelated question, since someone was asking me to review their COM implementation.  If a class implements one or more COM interfaces, must it be refcounted (and thus consumed externally via scoped_refptr<> and the like)?  Also, if yes, then when do we need to override AddRef/Release, and when do those overrides need to use interlocked instructions?  (The context for anyone who truly cares is the TsfEventRouter and http://codereview.chromium.org/11235023/ .)

PK

[Daniel Cheng]

I don't think you can use base/memory/ref_counted.h since AddRef and Release would only differ on the return type, right? Also in the review you linked, the destructor is public... that should probably be fixed.

As for thread-safety, I guess it depends on how the COM object is consumed. If you're 100% sure it's never used on more than one thread, I guess it's OK to not use interlocked functions.

Daniel

--

[Peter Kasting]

On Mon, Oct 22, 2012 at 6:14 PM, Daniel Cheng <dch...@chromium.org> wrote:

I don't think you can use base/memory/ref_counted.h since AddRef and Release would only differ on the return type, right?

Reading ref_counted.h, I'm pretty sure a scoped_refptr<> can be used with any object that implements AddRef() and Release() methods, even if those methods' return types differ from those in RefCountedBase.

That still leaves my original question: do callers need to consume this as a refcounted object to begin with?  If all implementations of COM interfaces must be refcounted, can we somehow at least not override AddRef() and Release() ourselves, and can we restrict the use of a scoped_refptr<> to an outer class that contains an inner, refcounted class that does the actual implementation?

PK

[Robert Shield]

Wrt COM code in general, one does not simply write their own COM code from scratch. The boilerplate provided by ATL or libraries written by in-house COM gurus as well as standard conventions followed by COM code prevent all manner of nasty bugs caused by not getting the life cycle or threading models right.

I don't have any specific opinions on STDMETHOD/STDMETHODIMPL other than they are conventional to folk who write COM code and as Michael mentioned make an interface immediately recognizable as a COM interface.

On Mon, Oct 22, 2012 at 9:36 PM, Peter Kasting <pkas...@google.com> wrote:

On Mon, Oct 22, 2012 at 6:14 PM, Daniel Cheng <dch...@chromium.org> wrote:

I don't think you can use base/memory/ref_counted.h since AddRef and Release would only differ on the return type, right?

Reading ref_counted.h, I'm pretty sure a scoped_refptr<> can be used with any object that implements AddRef() and Release() methods, even if those methods' return types differ from those in RefCountedBase.

That still leaves my original question: do callers need to consume this as a refcounted object to begin with?  

Yes. We have base/win/scoped_comptr.h for this.

In the CL you referenced, if someone QI()'s, then Release()s the object, then OmniboxViewWin is left with a dangling pointer to freed memory. 

 

If all implementations of COM interfaces must be refcounted, can we somehow at least not override AddRef() and Release() ourselves, and can we restrict the use of a scoped_refptr<> to an outer class that contains an inner, refcounted class that does the actual implementation?

This is a big part of what ATL's CComObject/CComObjectRootEx provide (see e.g. inheritors of CComObjectRootEx in src/chrome_frame). CComObjectRootEx also makes it easy to specify the threading model of the object being implemented, providing appropriate internal implementations for AddRef/Release for each model. 

[Peter Kasting]

On Mon, Oct 22, 2012 at 7:17 PM, Robert Shield <robert...@chromium.org> wrote:

On Mon, Oct 22, 2012 at 9:36 PM, Peter Kasting <pkas...@google.com> wrote:

That still leaves my original question: do callers need to consume this as a refcounted object to begin with?  

Yes. We have base/win/scoped_comptr.h for this.

Hmm, I always thought of that as a tool to consume Microsoft-provided COM objects.  I guess I haven't worked with enough cases where we implement our own.

In the CL you referenced, if someone QI()'s, then Release()s the object, then OmniboxViewWin is left with a dangling pointer to freed memory. 

Well, not right now, since it's holding the pointer as a scoped_refptr.  But it would be if it were holding a raw pointer.

This solidifies my thought that the class in question here should probably be implemented as a simple, non-refcounted public interface (which provides the one or two functions outsiders need to access) that holds a member pointer to a refcounted object that implements the COM interfaces in question.  (I'd go with a simpler single class that did everything if consumers needed more access/power, since plumbing all the APIs through would become annoying.)

If all implementations of COM interfaces must be refcounted, can we somehow at least not override AddRef() and Release() ourselves, and can we restrict the use of a scoped_refptr<> to an outer class that contains an inner, refcounted class that does the actual implementation?

This is a big part of what ATL's CComObject/CComObjectRootEx provide (see e.g. inheritors of CComObjectRootEx in src/chrome_frame). CComObjectRootEx also makes it easy to specify the threading model of the object being implemented, providing appropriate internal implementations for AddRef/Release for each model.

Thanks for this.  I'll recommend going this route.

PK 

[Tommi]

+1 For using the macros.  That's the standard and FWIW the #2 approach also means using Windows specific macros (DWORD, REFIID, et al).

COM is riddled with these macros and types that are specific to COM.  IID, HRESULT, S_OK, CLASSID, FAILED(), SUCCEEDED(), the list goes on and on.  All of these are very familiar and immediately recognizable by anyone with COM experience.  The same goes for STDMETMOD (header file declaration with __stdcall and the virtual keyword), STDMETHODIMPL (use in the source file, doesn't have the virtual keyword), macros.  There isn't a lot of COM code in Chromium, so I don't see the harm in keeping this specific code in line with what developers that write and maintain it are used to.  It is what it is :-)

--

[Jói Sigurðsson]

+1 for using the macros (STDMETHODIMP et al).

Developers who aren't used to COM should be deadly afraid of it,
mostly because of the harder-to-enforce reference counting rules* and
also because of alien concepts such as apartments and the FTM
(free-threaded marshaller, not to be confused with FSM, the Flying
Spaghetti Monster).

Having funny-looking macros that you don't understand in the method
definitions helps achieve the appropriate level of fear and ensure you
find some grizzly battle-scarred COM veteran to review.

Cheers,
Jói


* Since COM interfaces can't pass actual scoped_refptr thingies, which
make refcounting work like magic in the rest of the Chromium code, you
need to manually keep a bunch of rules in mind when writing and
reviewing COM code.

[Alex Pakhunov]

+1 for using the macros (STDMETHODIMP et al). They really make the COM code easy to recognize.

Since COM interfaces can't pass actual scoped_refptr thingies, which make refcounting work like magic in the rest of the Chromium code, you need to manually keep a bunch of rules in mind when writing and reviewing COM code.

This actually is a good thing (in a way). It helps to avoid mixing COM specific code with the rest of code leaving a bit less room for error.

Alex.

[Andrew Scherkus]

On Tue, Oct 23, 2012 at 8:24 AM, Alex Pakhunov <alex...@google.com> wrote:

+1 for using the macros (STDMETHODIMP et al). They really make the COM code easy to recognize.

Since COM interfaces can't pass actual scoped_refptr thingies, which make refcounting work like magic in the rest of the Chromium code, you need to manually keep a bunch of rules in mind when writing and reviewing COM code.

This actually is a good thing (in a way). It helps to avoid mixing COM specific code with the rest of code leaving a bit less room for error.

Hmm... it looks like we've got heavy support from battle-scarred COM developers that using STDMETHODIMP et al is simply what you gotta do when writing COM code. We also now know who to +cc on COM code reviews ;)

Darin: is your preference for #2 a mild or strong preference?

Andrew

[Darin Fisher]

On Tue, Oct 23, 2012 at 10:59 AM, Andrew Scherkus <sche...@chromium.org> wrote:

On Tue, Oct 23, 2012 at 8:24 AM, Alex Pakhunov <alex...@google.com> wrote:

+1 for using the macros (STDMETHODIMP et al). They really make the COM code easy to recognize.

Since COM interfaces can't pass actual scoped_refptr thingies, which make refcounting work like magic in the rest of the Chromium code, you need to manually keep a bunch of rules in mind when writing and reviewing COM code.

This actually is a good thing (in a way). It helps to avoid mixing COM specific code with the rest of code leaving a bit less room for error.

Hmm... it looks like we've got heavy support from battle-scarred COM developers that using STDMETHODIMP et al is simply what you gotta do when writing COM code. We also now know who to +cc on COM code reviews ;)

Darin: is your preference for #2 a mild or strong preference?

mild personal preference.  when in rome...