Fix double-initialization of CComModule.

Fix double-initialization of CComModule.

This should allow ATL to work in all build configurations by
dynamically creating a CComModule only if needed.

BUG=102736
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109953

[dmazzoni, 2011-11-04 06:33:32]

I think this is the right solution. Note that this has been working fine for a
while to allow renderer_accessibility_browsertest.cc to work in different build
configurations - it should work for views/ and
browser_views_accessibility_browsertest.cc too.

[cpu_(ooo_6.6-7.5), 2011-11-04 17:25:34]

http://codereview.chromium.org/8467002/diff/3001/base/win/atl_module.cc
File base/win/atl_module.cc (right):

http://codereview.chromium.org/8467002/diff/3001/base/win/atl_module.cc#newco...
base/win/atl_module.cc:18: // Make sure COM is initialized for this thread; it's
safe to call twice.
not really. It is safe it is the same exact COM mode. I rather handle
initialization of COM elsewhere.

[dmazzoni, 2011-11-04 18:26:54]

On 2011/11/04 17:25:34, cpu wrote:
> http://codereview.chromium.org/8467002/diff/3001/base/win/atl_module.cc
> File base/win/atl_module.cc (right):
> 
>
http://codereview.chromium.org/8467002/diff/3001/base/win/atl_module.cc#newco...
> base/win/atl_module.cc:18: // Make sure COM is initialized for this thread;
it's
> safe to call twice.
> not really. It is safe it is the same exact COM mode. I rather handle
> initialization of COM elsewhere.

Good point. I'll move CoInitialize out of this change.

[rvargas (doing something else), 2011-11-04 21:53:03]

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc
File base/win/atl_module.cc (right):

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc#newco...
base/win/atl_module.cc:15: static CComModule module;
Shouldn't this should be CAtlModule? The constructor would initialize
_pAtlModule internally.

However, this whole thing will just initialize ATL for base.dll, and we
certainly don't need that.

[dmazzoni, 2011-11-04 22:34:47]

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc
File base/win/atl_module.cc (right):

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc#newco...
base/win/atl_module.cc:15: static CComModule module;
On 2011/11/04 21:53:03, rvargas wrote:
> Shouldn't this should be CAtlModule? The constructor would initialize
> _pAtlModule internally.

Is there any other good reason? Otherwise it just looks like magic to anyone who
doesn't know CAtlModule. I think it's more clear to see the line of code setting
_pAtlModule to &module, it makes it obvious to anyone new searching through the
code where this happens.

> However, this whole thing will just initialize ATL for base.dll, and we
> certainly don't need that.

Oops, you're totally right about that. Fixed, take another look.

[rvargas (doing something else), 2011-11-04 22:52:14]

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc
File base/win/atl_module.cc (right):

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc#newco...
base/win/atl_module.cc:15: static CComModule module;
On 2011/11/04 22:34:47, Dominic Mazzoni wrote:
> On 2011/11/04 21:53:03, rvargas wrote:
> > Shouldn't this should be CAtlModule? The constructor would initialize
> > _pAtlModule internally.
> 
> Is there any other good reason? Otherwise it just looks like magic to anyone
who
> doesn't know CAtlModule. I think it's more clear to see the line of code
setting
> _pAtlModule to &module, it makes it obvious to anyone new searching through
the
> code where this happens.

It should feel like black magic already: what is _pAtlModule and why do we care
about it?

My point was that there is more initialization that we need than just setting
this pointer, and all relevant stuff happens inside the constructor of the
object (including setting the global), and we are overwriting a value that is
internal to ATL. In fact, creating a CComModule initializes not only _pAtlModule
but also _pModule (which is probably a good thing, so ignore my comment about
changing the type).

So I think it is better just to leave the constructor do its thing and add a
comment for the curious.


> 
> > However, this whole thing will just initialize ATL for base.dll, and we
> > certainly don't need that.
> 
> Oops, you're totally right about that. Fixed, take another look.

http://codereview.chromium.org/8467002/diff/10003/views/accessibility/native_...
File views/accessibility/native_view_accessibility_win.cc (right):

http://codereview.chromium.org/8467002/diff/10003/views/accessibility/native_...
views/accessibility/native_view_accessibility_win.cc:27: ::CoInitialize(NULL); 
// Safe to call more than once.
I'm missing the corresponding CoUninitialize

[dmazzoni, 2011-11-04 23:01:30]

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc
File base/win/atl_module.cc (right):

http://codereview.chromium.org/8467002/diff/7001/base/win/atl_module.cc#newco...
base/win/atl_module.cc:15: static CComModule module;
On 2011/11/04 22:52:14, rvargas wrote:
> My point was that there is more initialization that we need than just setting
> this pointer, and all relevant stuff happens inside the constructor of the
> object (including setting the global), and we are overwriting a value that is
> internal to ATL. In fact, creating a CComModule initializes not only
_pAtlModule
> but also _pModule (which is probably a good thing, so ignore my comment about
> changing the type).
> 
> So I think it is better just to leave the constructor do its thing and add a
> comment for the curious.

Didn't realize it initialized it. Fixed.

[dmazzoni, 2011-11-05 06:18:46]

http://codereview.chromium.org/8467002/diff/10003/views/accessibility/native_...
File views/accessibility/native_view_accessibility_win.cc (right):

http://codereview.chromium.org/8467002/diff/10003/views/accessibility/native_...
views/accessibility/native_view_accessibility_win.cc:27: ::CoInitialize(NULL); 
// Safe to call more than once.
On 2011/11/04 22:52:15, rvargas wrote:
> I'm missing the corresponding CoUninitialize

Actually since content/ definitely calls CoInitialize and all of the
accessibility tests do, this shouldn't be needed. I just tested it and COM is
already initialized when this code is reached, even in a multi-dll build, so I
deleted it.

[rvargas (doing something else), 2011-11-06 18:27:48]

LGTM

[dmazzoni, 2011-11-06 19:23:43]

Darin, could I get approval for base/ ?

[cpu_(ooo_6.6-7.5), 2011-11-08 02:23:47]

lgtm. sorry for the delay.

[Nico, 2011-11-09 18:00:57]

We're trying to get rid of ATL, and this seems to be used mostly in tests – can
you put this some place not base? Is there a way to not use atl for this in
general?

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h
File base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h#newco...
base/win/atl_module.h:29: static CComModule module;
This adds an exit time destructor. Use CR_DEFINE_STATIC_LOCAL instead.

Also, I'm not sure if a static variable in a static function will refer to the
same object in all translation units if the function is not marked as "inline"
("A static local variable in an extern inline function always refers to the same
object.", but nothing about static locals in static functions; 7.1.2.-4-) – I'd
put the function definition into a cc file for that reason

[dmazzoni, 2011-11-09 18:21:04]

Yes, I'd love to get rid of ATL. It's not quite that simple - for accessibility,
we need to implement a COM server for about a dozen different interfaces that
derive from IDispatch. ATL lets you do this in 10 lines. Without ATL it'd be
~10,000 lines of boilerplate code. Maybe we should write a replacement - but
this is not a trivial exercise and this change makes things slightly saner in
the meantime.

The reason we initialize ATL in tests isn't only because the test might use ATL,
it's because the accessibilty code being tested uses ATL and if there's no ATL
module defined anywhere, the code that uses ATL will crash.

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h
File base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h#newco...
base/win/atl_module.h:29: static CComModule module;
On 2011/11/09 18:00:57, Nico wrote:
> This adds an exit time destructor. Use CR_DEFINE_STATIC_LOCAL instead.

The destructor needs to be called, or CComModule can't clean up its state, i.e.
cleanly terminate connections with other processes.

> Also, I'm not sure if a static variable in a static function will refer to the
> same object in all translation units if the function is not marked as "inline"
> ("A static local variable in an extern inline function always refers to the
same
> object.", but nothing about static locals in static functions; 7.1.2.-4-) –
I'd
> put the function definition into a cc file for that reason

Putting in in a .cc file wouldn't work, the whole reason we need this is so that
in a multi-dll build, every dll that needs it gets an ATL module.

There are no other translation units that will need to access this variable. The
CComModule constructor sets a global variable _pAtlModule and points it to this
object. As soon as one CComModule is registered for that dll, any time this
function is executed again the conditional will fail and the static local will
never be constructed in the first place.

[Nico, 2011-11-09 18:37:22]

It looks like this could live in ui as well, which would be less central than
base.

Most of the 10000s of lines that ATL saves us are probably pretty repetitive;
could they be generated by some kind of script?

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h
File base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h#newco...
base/win/atl_module.h:29: static CComModule module;
On 2011/11/09 18:21:04, Dominic Mazzoni wrote:
> On 2011/11/09 18:00:57, Nico wrote:
> > This adds an exit time destructor. Use CR_DEFINE_STATIC_LOCAL instead.
> 
> The destructor needs to be called, or CComModule can't clean up its state,
i.e.
> cleanly terminate connections with other processes.

I'm currently trying to remove all exit time destructors from the code base.
Exit time destructors in base/ will become a compiler error today. (in ui/ too.)

Can you add an explicit DestroyATLModule() call or something like that?

> > Also, I'm not sure if a static variable in a static function will refer to
the
> > same object in all translation units if the function is not marked as
"inline"
> > ("A static local variable in an extern inline function always refers to the
> same
> > object.", but nothing about static locals in static functions; 7.1.2.-4-) –
> I'd
> > put the function definition into a cc file for that reason
> 
> Putting in in a .cc file wouldn't work, the whole reason we need this is so
that
> in a multi-dll build, every dll that needs it gets an ATL module.

Ah! This might deserve a comment.

> There are no other translation units that will need to access this variable.
The
> CComModule constructor sets a global variable _pAtlModule and points it to
this
> object. As soon as one CComModule is registered for that dll, any time this
> function is executed again the conditional will fail and the static local will
> never be constructed in the first place.

What if multiple threads call this function at the same time?

[Nico, 2011-11-09 18:47:18]

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h
File base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/12011/base/win/atl_module.h#newco...
base/win/atl_module.h:29: static CComModule module;
> I'm currently trying to remove all exit time destructors from the code base.
> Exit time destructors in base/ will become a compiler error today. (in ui/
too.)

(The error part is actually only true for platforms where we build with clang,
i.e. mac, linux, and chromeos. But windows will join that list eventually.)

[dmazzoni, 2011-11-09 18:52:41]

On Wed, Nov 9, 2011 at 10:37 AM, <thakis@chromium.org> wrote:

> It looks like this could live in ui as well, which would be less central
> than
> base.
>

I'm fine with ui/base/win instead.


> Most of the 10000s of lines that ATL saves us are probably pretty
> repetitive;
> could they be generated by some kind of script?


Yes, that's probably our best bet.

- Dominic

[dmazzoni, 2011-11-14 07:18:44]

+sky for approval of ui/base/win

For context, this is only intended to be a quick fix so that accessibility
doesn't fail on a subset of the possible build configurations. The longer-term
plan is still to get rid of ATL.

[sky, 2011-11-14 16:12:22]

Rubber stamp LGTM

[Nico, 2011-11-14 16:21:13]

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h
File ui/base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h#ne...
ui/base/win/atl_module.h:25: // rather than in the "base" module.
"ui"

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h#ne...
ui/base/win/atl_module.h:29: static CComModule module;
Does CComModule have a destructor? Can you think of a way to not let this
introduce an exit destructor call? (If the destructor is not important, maybe
you can use CR_DEFINE_STATIC_LOCAL)

[dmazzoni, 2011-11-14 16:27:41]

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h
File ui/base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h#ne...
ui/base/win/atl_module.h:25: // rather than in the "base" module.
On 2011/11/14 16:21:13, Nico wrote:
> "ui"

Done.

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h#ne...
ui/base/win/atl_module.h:29: static CComModule module;
On 2011/11/14 16:21:13, Nico wrote:
> Does CComModule have a destructor? Can you think of a way to not let this
> introduce an exit destructor call? (If the destructor is not important, maybe
> you can use CR_DEFINE_STATIC_LOCAL)

My understanding is that the destructor is useful - it cleanly shuts down COM
connections with other processes. But I honestly don't know if there'd be any
harm in not calling it.

If it makes you feel any better, this code will never execute in a
statically-linked release build of chrome.exe, because everything is in one
module and ATL is initialized once in "content".

Because of that, I think it's safer to just let the destructor get called in
multi-dll builds and tests.

[Nico, 2011-11-14 16:34:28]

Thanks for the quick reply!

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h
File ui/base/win/atl_module.h (right):

http://codereview.chromium.org/8467002/diff/26001/ui/base/win/atl_module.h#ne...
ui/base/win/atl_module.h:29: static CComModule module;
> My understanding is that the destructor is useful - it cleanly shuts down COM
> connections with other processes. But I honestly don't know if there'd be any
> harm in not calling it.

Can we try if that works then?

> If it makes you feel any better, this code will never execute in a
> statically-linked release build of chrome.exe, because everything is in one
> module and ATL is initialized once in "content".

Good to know. But since the compiler can probably not prove that, i'd guess
it'll still emits code for "if static was initialized then call destructor" 
that runs at exit.

(Also, if we ever get to a state were I can make exit time destructors
compile-time errors on windows, this will need changing too.)

[dmazzoni, 2011-11-14 16:59:12]

On 2011/11/14 16:34:28, Nico wrote:
> Can we try if that works then?

Done. Just tested it and I didn't notice any difference. If I start having
random occasional crashes on exit, though, I'm going to add back the destructor.
:)

> (Also, if we ever get to a state were I can make exit time destructors
> compile-time errors on windows, this will need changing too.)

How about we just get rid of ATL before then?

[Nico, 2011-11-14 17:03:42]

LGTM

> How about we just get rid of ATL before then?

From what I can tell, nobody's really working on that. I'm actively (but slowly)
working on the warning for windows, so chances are I'll be faster than the ATL
removal.

[dmazzoni, 2011-11-14 17:06:36]

On Mon, Nov 14, 2011 at 9:03 AM, <thakis@chromium.org> wrote:

> How about we just get rid of ATL before then?
>>
>
I'm working on removing ATL for accessibility. I think that's actually the
main use of ATL left in chrome.exe, or at least one of the trickier ones to
get rid of.

- Dominic

[commit-bot: I haz the power, 2011-11-14 19:31:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/8467002/33002

[commit-bot: I haz the power, 2011-11-14 20:40:09]

Try job failure for 8467002-33002 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...