Cleanup in ProcessSingleton.

Cleanup in ProcessSingleton.  These originated as candidate fixes for issue 111361, but the true fix is cpu@'s r119830.

a) Unify code to retrieve HMODULE based on address in base::GetModuleFromAddress.  (cpu@'s r119830 already has the fix to use the DLL's HMODULE 119830 rather than the EXE's HINSTANCE)

b) Try harder to unregister the window class, by trying it first in ProcessSingleton::Cleanup, which gets called before the fast shutdown path calls ExitProcess().  Also, use correct HMODULE to unregister.

c) Avoid any possibility of a race to create the message window when the Create() method is called from NotifyOtherProcessOrCreate() or from outside the class by moving the implementation of Create(), which was always called from the constructor, into the constructor and making Create() just return true or false based on the success of the work already done.

BUG=111361
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121480

[Jói, 2012-01-27 13:44:49]

Ben:  Main reviewer, for familiarity with with ProcessSingleton on Windows.

Evan, Robert:  base/OWNERS and chrome_frame/OWNERS heads-up.

Cheers,
Jói

[grt (UTC plus 2), 2012-01-27 14:20:15]

BUG=111361 rather than 111363?

[Jói, 2012-01-27 15:26:32]

Yes, thanks for catching that Greg.  Updated the description to have
the correct bug number.

Cheers,
Jói


On Fri, Jan 27, 2012 at 2:20 PM,  <grt@chromium.org> wrote:
> BUG=111361 rather than 111363?
>
> http://codereview.chromium.org/9121046/

[Ben Goodger (Google), 2012-01-27 16:16:42]

I'm actually not that familiar with ProcessSingleton. Perhaps I moved it at some
point and now I'm blamed for it.

Replacing myself with cpu per our discussion yesterday at lunch.

cpu: ;-)

[Evan Martin, 2012-01-27 16:17:40]

http://codereview.chromium.org/9121046/diff/1/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util.h#newcode144
base/process_util.h:144: // @returns the module handle to which an address
belongs.
I think we don't use any javadoc-style extraction (the functions above use
"Returns" instead of "@returns").

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:178: MEMORY_BASIC_INFORMATION info = {0};
FWIW, you can do
  MEMORY_BASIC_INFORMATION info = {};
to initialize a struct to all-zeroes, even when the first field is not
initializable from an integer.  (I only learned this recently and am compelled
to share it!)

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:179: ::VirtualQuery(address, &info, sizeof(info));
Shouldn't we be checking the return value?  What if the address is invalid, for
example?

[robertshield, 2012-01-27 16:26:44]

lgtm

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:180: return
reinterpret_cast<HMODULE>(info.AllocationBase);
I know you took this from CF, so I almost feel bad commenting here.
Nevertheless, fwiw, you can also call GetModuleHandleEx with
GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS.

[cpu_(ooo_6.6-7.5), 2012-01-27 19:31:50]

lgtm

You need a new, albeit very test for the new function you added to base.

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:180: return
reinterpret_cast<HMODULE>(info.AllocationBase);
Yes please use Robert's method.

On 2012/01/27 16:26:45, robertshield wrote:
> I know you took this from CF, so I almost feel bad commenting here.
> Nevertheless, fwiw, you can also call GetModuleHandleEx with
> GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS.

[cpu_(ooo_6.6-7.5), 2012-01-27 19:32:51]

I meant: You need a new, albeit very *simple* unit test for the new function ..

[cpu_(ooo_6.6-7.5), 2012-01-28 21:20:18]

stop the presses I think I know what is going on. It is none of the above.

It is actually pretty amazing that it was working before, and I have to wonder
what other places have this error.

See if you can spot the error with this hint: GWLP_USERDATA

I am making a CL, stay tuned.

[Jói, 2012-01-31 15:06:47]

> See if you can spot the error with this hint: GWLP_USERDATA

Nice catch :)

> http://codereview.chromium.org/9121046/

PTAL, I've merged to Carlos' change and updated the change
description.  There are still at least two worth-while fixes in this
change (using the correct HMODULE to UnregisterClass(), and trying
extra hard to unregister) so it should probably go in sooner or later,
although it doesn't seem needed for M18.

Cheers,
Jói



On Sat, Jan 28, 2012 at 9:20 PM,  <cpu@chromium.org> wrote:
> stop the presses I think I know what is going on. It is none of the above.
>
> It is actually pretty amazing that it was working before, and I have to
> wonder
> what other places have this error.
>
> See if you can spot the error with this hint: GWLP_USERDATA
>
> I am making a CL, stay tuned.
>
> http://codereview.chromium.org/9121046/

[Jói, 2012-02-09 16:02:50]

cpu:  PTAL, I forgot to ping this review last week.

Cheers,
Jói


On 2012/01/31 15:06:47, Jói wrote:
> > See if you can spot the error with this hint: GWLP_USERDATA
> 
> Nice catch :)
> 
> > http://codereview.chromium.org/9121046/
> 
> PTAL, I've merged to Carlos' change and updated the change
> description.  There are still at least two worth-while fixes in this
> change (using the correct HMODULE to UnregisterClass(), and trying
> extra hard to unregister) so it should probably go in sooner or later,
> although it doesn't seem needed for M18.
> 
> Cheers,
> Jói
> 
> 
> 
> On Sat, Jan 28, 2012 at 9:20 PM,  <mailto:cpu@chromium.org> wrote:
> > stop the presses I think I know what is going on. It is none of the above.
> >
> > It is actually pretty amazing that it was working before, and I have to
> > wonder
> > what other places have this error.
> >
> > See if you can spot the error with this hint: GWLP_USERDATA
> >
> > I am making a CL, stay tuned.
> >
> > http://codereview.chromium.org/9121046/

[cpu_(ooo_6.6-7.5), 2012-02-09 22:00:50]

lgtm

[commit-bot: I haz the power, 2012-02-10 07:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9121046/22001

[commit-bot: I haz the power, 2012-02-10 07:00:51]

Presubmit check for 9121046-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
base/process_util_win.cc,base/process_util_unittest.cc,base/process_util.h

Presubmit checks took 1.9s to calculate.

[Jói, 2012-02-10 09:11:57]

Evan:  Need your LGTM for base/OWNERS.

Cheers,
Jói

http://codereview.chromium.org/9121046/diff/1/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util.h#newcode144
base/process_util.h:144: // @returns the module handle to which an address
belongs.
On 2012/01/27 16:17:41, Evan Martin wrote:
> I think we don't use any javadoc-style extraction (the functions above use
> "Returns" instead of "@returns").

Done.

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:178: MEMORY_BASIC_INFORMATION info = {0};
On 2012/01/27 16:17:41, Evan Martin wrote:
> FWIW, you can do
>   MEMORY_BASIC_INFORMATION info = {};
> to initialize a struct to all-zeroes, even when the first field is not
> initializable from an integer.  (I only learned this recently and am compelled
> to share it!)

Cool, nice tip :)

http://codereview.chromium.org/9121046/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:179: ::VirtualQuery(address, &info, sizeof(info));
On 2012/01/27 16:17:41, Evan Martin wrote:
> Shouldn't we be checking the return value?  What if the address is invalid,
for
> example?

It is now checked, for the new API that Robert suggested.

[Evan Martin, 2012-02-10 17:02:06]

base lgtm

[commit-bot: I haz the power, 2012-02-10 17:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9121046/22001

[commit-bot: I haz the power, 2012-02-10 18:46:00]

Change committed as 121480