Base: Don't delete the previous command line when calling

Base: Don't delete the previous command line when calling
CommandLine::Init()

In the multi-DLL build, we can end up initializing the object
twice at startup (once in chrome.exe and once in chrome.dll).
It should be harmless.

BUG=76996
TEST=base_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92380

[msw, 2011-06-28 23:19:42]

Adding Evan. Can you explain the problem a little better for my benefit?

http://codereview.chromium.org/7273053/diff/1/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/7273053/diff/1/base/command_line.cc#newcode171
base/command_line.cc:171: if (current_process_commandline_) {
I don't think this should fail silently. There might be some clients relying on
this behavior, even if it's incorrect. If this is the right behavior change, add
a DCHECK or a CHECK.

[rvargas (doing something else), 2011-06-28 23:57:35]

The problem is a race for the shared build on Windows (at least):

We init a CommandLine on chrome.exe, and there is some code that depends on it
(async)

We also have a CommandLine on chrome.dll, but when we have a shared base.dll
module, the second call to Init silently deletes the object created from the
exe... and of course, there are users of it (because there is no way to know
that nobody is using the command line object anymore*), so we crash accessing a
deleted object.

I searched for users of Init and Reset, and it looks like the change is OK, but
I don't fully understand the reasoning behind Reset... and there's a lot of
callers of Init.

I would be open to have a DCHECK when BASE_DLL is not defined, but only as a
temporary measure to detect code that may be relying on the old behavior.

(*) This means that calling Init more than once without complete control of what
goes on in between is just a bug.

http://codereview.chromium.org/7273053/diff/1/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/7273053/diff/1/base/command_line.cc#newcode171
base/command_line.cc:171: if (current_process_commandline_) {
On 2011/06/28 23:19:42, msw wrote:
> I don't think this should fail silently. There might be some clients relying
on
> this behavior, even if it's incorrect. If this is the right behavior change,
add
> a DCHECK or a CHECK.

I was going to CHECK... except that it will defeat the purpose of the change,
because it happens for a build configuration. And I don't want to add code
specifically for one configuration when I'm just following the documented
behavior.

[msw, 2011-06-29 01:14:24]

Hmm, this might be acceptable, since all the inits on Windows are presumably
setting the same value with ::GetCommandLineW() and hopefully not making
conflicting modifications. But it's scary that we'll have different behavior on
different builds. Perhaps we could make a singleton CommandLine for the entire
executable when we don't have a shared base.dll?
http://msdn.microsoft.com/en-us/library/7t44f2x1(v=vs.80).aspx

Otherwise, it could be prudent to rename the function or include the
build-specific DCHECK temporarily for any test failures this may cause. I'm not
sufficiently enlightened to be the sole reviewer.

[rvargas (doing something else), 2011-07-11 18:47:45]

Evan,

Cold you take a look?

[Evan Martin, 2011-07-11 18:52:24]

I still don't quite understand.  Why are we calling Init() more than once? 
Don't we just call it in WinMain()?

[rvargas (doing something else), 2011-07-11 18:57:35]

On 2011/07/11 18:52:24, Evan Martin wrote:
> I still don't quite understand.  Why are we calling Init() more than once? 
> Don't we just call it in WinMain()?

It is called from chrome.exe!wWinMain and chrome.dll!ChromeMain. The former is
used for asynchronous breakpad initialization and the latter for everything
else.

[Evan Martin, 2011-07-11 19:03:15]

On 2011/07/11 18:57:35, rvargas wrote:
> On 2011/07/11 18:52:24, Evan Martin wrote:
> > I still don't quite understand.  Why are we calling Init() more than once? 
> > Don't we just call it in WinMain()?
> 
> It is called from chrome.exe!wWinMain and chrome.dll!ChromeMain. The former is
> used for asynchronous breakpad initialization and the latter for everything
> else.

Ah, I see.  And in the non-DLL case, the two modules end up with separate copies
of the CommandLine, but in the DLL case they end up sharing it?  If you don't
mind, I'm gonna update the review description to summarize my understanding, and
let you fix it as you see fit.

LGTM

[rvargas (doing something else), 2011-07-11 22:35:35]

On 2011/07/11 19:03:15, Evan Martin wrote:
> On 2011/07/11 18:57:35, rvargas wrote:
> > On 2011/07/11 18:52:24, Evan Martin wrote:
> > > I still don't quite understand.  Why are we calling Init() more than once?

> > > Don't we just call it in WinMain()?
> > 
> > It is called from chrome.exe!wWinMain and chrome.dll!ChromeMain. The former
is
> > used for asynchronous breakpad initialization and the latter for everything
> > else.
> 
> Ah, I see.  And in the non-DLL case, the two modules end up with separate
copies
> of the CommandLine, but in the DLL case they end up sharing it?  If you don't

That's correct.


> mind, I'm gonna update the review description to summarize my understanding,
and
> let you fix it as you see fit.
> 
> LGTM

Thanks.