Rewrite of chrome_launcher.exe. ETW-based perf tests suggest that this is on ...

Rewrite of chrome_launcher.exe. ETW-based perf tests suggest that this is on average about 50% faster than the previous version. 



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

[tommi (sloooow) - chröme, 2010-05-26 17:23:07]

can you run lint on these file and in particular make sure that files have a
trailing \n so that future changes won't break on the trybots

http://codereview.chromium.org/2278003/diff/15001/16004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16004#newcode7
chrome_frame/chrome_launcher.cc:7: #include <Windows.h>
nit: I think we usually include these files using all lower case

http://codereview.chromium.org/2278003/diff/15001/16004#newcode78
chrome_frame/chrome_launcher.cc:78: std::wstring str(input_str);
should we guard against NULL first?

http://codereview.chromium.org/2278003/diff/15001/16004#newcode81
chrome_frame/chrome_launcher.cc:81: const std::wstring::size_type last_char =
str.length();
is this variable used?

http://codereview.chromium.org/2278003/diff/15001/16004#newcode103
chrome_frame/chrome_launcher.cc:103: for (int i = 0; i <
arraysize(kAllowedSwitches); i++) {
nit: ++i

http://codereview.chromium.org/2278003/diff/15001/16004#newcode104
chrome_frame/chrome_launcher.cc:104: size_t arg_length =
wcslen(kAllowedSwitches[i]);
lstrlenW

http://codereview.chromium.org/2278003/diff/15001/16004#newcode109
chrome_frame/chrome_launcher.cc:109: (arg.length() > (arg_length + 2) &&
arg[arg_length+2] == L'=')) {
fix indent

http://codereview.chromium.org/2278003/diff/15001/16004#newcode121
chrome_frame/chrome_launcher.cc:121: args = CommandLineToArgvW(command_line,
&num_args);
check for NULL first?

http://codereview.chromium.org/2278003/diff/15001/16004#newcode126
chrome_frame/chrome_launcher.cc:126: for (int i = 1; i < num_args; i++) {
++i

http://codereview.chromium.org/2278003/diff/15001/16004#newcode151
chrome_frame/chrome_launcher.cc:151: if (CreateProcess(NULL,
const_cast<wchar_t*>(chrome_path.c_str()),
this part of CreateProcess has always been a bit weird to me:

"The Unicode version of this function, CreateProcessW, can modify the contents
of this string. Therefore, this parameter cannot be a pointer to read-only
memory (such as a const variable or a literal string). If this parameter is a
constant string, the function may cause an access violation."

It doesn't say if the string might be modified beyond its current length (I
don't think it does).  But in order to be politically correct, I think we should
call chrome_path.begin() or use &chrome_path[0] here and avoid the cast at
least.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode159
chrome_frame/chrome_launcher.cc:159: _ASSERT(FALSE);
DCHECK

http://codereview.chromium.org/2278003/diff/15001/16004#newcode172
chrome_frame/chrome_launcher.cc:172: GetModuleFileName(NULL, cur_path,
MAX_PATH);
arraysize() instead of MAX_PATH?

http://codereview.chromium.org/2278003/diff/15001/16006
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16006#newcode10
chrome_frame/chrome_launcher_main.cc:10: #include
"chrome_frame/chrome_launcher.h"
I think this should be the first include

http://codereview.chromium.org/2278003/diff/15001/16006#newcode39
chrome_frame/chrome_launcher_main.cc:39: #define arraysize(array)
(sizeof(ArraySizeHelper(array)))
in order to not copy this to many places, can you make a single header file that
has the borrowed chrome code?

http://codereview.chromium.org/2278003/diff/15001/16006#newcode44
chrome_frame/chrome_launcher_main.cc:44: // TODO(robertshield): Populate this
with actual data.
indent

http://codereview.chromium.org/2278003/diff/15001/16006#newcode60
chrome_frame/chrome_launcher_main.cc:60: 
remove empty line.  check for NULL?

http://codereview.chromium.org/2278003/diff/15001/16006#newcode97
chrome_frame/chrome_launcher_main.cc:97: ::ExitProcess(exit_code);
huh, ExitProcess!?  why not allow global destruction etc?
I know this is not your code but do you mind changing this to:

  return exit_code;

http://codereview.chromium.org/2278003/diff/15001/16007
File chrome_frame/chrome_launcher_unittest.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16007#newcode20
chrome_frame/chrome_launcher_unittest.cc:20: CommandLine
good(FilePath(L"dummy.exe"));
maybe check an empty command line too?

http://codereview.chromium.org/2278003/diff/15001/16007#newcode49
chrome_frame/chrome_launcher_unittest.cc:49:
EXPECT_TRUE(chrome_launcher::IsValidArgument(L"--chrome-frame"));
test empty here too?

[amit, 2010-05-26 17:41:59]

Haven't gone into the details yet but a few drive by comments...

- please check input args as much as possible
- this might be a good time to enable crash reporting
- it would be nice if we could collect launch failure statistics (or at least
log errors) in some way here.

[robertshield, 2010-05-26 19:56:57]

Thanks Tommi, please take another look.

FWIW, all the files that rietveld is complaining don't have a newline at the end
actually do.

http://codereview.chromium.org/2278003/diff/15001/16004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16004#newcode7
chrome_frame/chrome_launcher.cc:7: #include <Windows.h>
On 2010/05/26 17:23:07, tommi wrote:
> nit: I think we usually include these files using all lower case

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode78
chrome_frame/chrome_launcher.cc:78: std::wstring str(input_str);
On 2010/05/26 17:23:07, tommi wrote:
> should we guard against NULL first?

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode81
chrome_frame/chrome_launcher.cc:81: const std::wstring::size_type last_char =
str.length();
On 2010/05/26 17:23:07, tommi wrote:
> is this variable used?

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode103
chrome_frame/chrome_launcher.cc:103: for (int i = 0; i <
arraysize(kAllowedSwitches); i++) {
On 2010/05/26 17:23:07, tommi wrote:
> nit: ++i

same diff for ints.. ok, done :-)

http://codereview.chromium.org/2278003/diff/15001/16004#newcode104
chrome_frame/chrome_launcher.cc:104: size_t arg_length =
wcslen(kAllowedSwitches[i]);
On 2010/05/26 17:23:07, tommi wrote:
> lstrlenW

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode109
chrome_frame/chrome_launcher.cc:109: (arg.length() > (arg_length + 2) &&
arg[arg_length+2] == L'=')) {
On 2010/05/26 17:23:07, tommi wrote:
> fix indent

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode121
chrome_frame/chrome_launcher.cc:121: args = CommandLineToArgvW(command_line,
&num_args);
On 2010/05/26 17:23:07, tommi wrote:
> check for NULL first?

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode126
chrome_frame/chrome_launcher.cc:126: for (int i = 1; i < num_args; i++) {
On 2010/05/26 17:23:07, tommi wrote:
> ++i

Done.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode151
chrome_frame/chrome_launcher.cc:151: if (CreateProcess(NULL,
const_cast<wchar_t*>(chrome_path.c_str()),
On 2010/05/26 17:23:07, tommi wrote:
> this part of CreateProcess has always been a bit weird to me:
> 
> "The Unicode version of this function, CreateProcessW, can modify the contents
> of this string. Therefore, this parameter cannot be a pointer to read-only
> memory (such as a const variable or a literal string). If this parameter is a
> constant string, the function may cause an access violation."
> 
> It doesn't say if the string might be modified beyond its current length (I
> don't think it does).  But in order to be politically correct, I think we
should
> call chrome_path.begin() or use &chrome_path[0] here and avoid the cast at
> least.

I seem to remember Raymond Chen pontificating on this at some point. Something
about how this avoided an extra buffer being created once upon a time. I'll use
the &chrome_path[0] thing to remain politically correct. Wouldn't want to be
politically incorrect, no... that would be bad.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode159
chrome_frame/chrome_launcher.cc:159: _ASSERT(FALSE);
On 2010/05/26 17:23:07, tommi wrote:
> DCHECK

No base\logging.h, no DCHECKs.

http://codereview.chromium.org/2278003/diff/15001/16004#newcode172
chrome_frame/chrome_launcher.cc:172: GetModuleFileName(NULL, cur_path,
MAX_PATH);
On 2010/05/26 17:23:07, tommi wrote:
> arraysize() instead of MAX_PATH?

Done.

http://codereview.chromium.org/2278003/diff/15001/16006
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16006#newcode10
chrome_frame/chrome_launcher_main.cc:10: #include
"chrome_frame/chrome_launcher.h"
On 2010/05/26 17:23:07, tommi wrote:
> I think this should be the first include

I believe it is system headers before includes of project files. Note that this
is chrome_launcher_main.cc, not chrome_launcher.cc.

http://codereview.chromium.org/2278003/diff/15001/16006#newcode39
chrome_frame/chrome_launcher_main.cc:39: #define arraysize(array)
(sizeof(ArraySizeHelper(array)))
On 2010/05/26 17:23:07, tommi wrote:
> in order to not copy this to many places, can you make a single header file
that
> has the borrowed chrome code?

Done.

http://codereview.chromium.org/2278003/diff/15001/16006#newcode44
chrome_frame/chrome_launcher_main.cc:44: // TODO(robertshield): Populate this
with actual data.
On 2010/05/26 17:23:07, tommi wrote:
> indent

Done.

http://codereview.chromium.org/2278003/diff/15001/16006#newcode60
chrome_frame/chrome_launcher_main.cc:60: 
On 2010/05/26 17:23:07, tommi wrote:
> remove empty line.  check for NULL?

Done.

http://codereview.chromium.org/2278003/diff/15001/16006#newcode97
chrome_frame/chrome_launcher_main.cc:97: ::ExitProcess(exit_code);
On 2010/05/26 17:23:07, tommi wrote:
> huh, ExitProcess!?  why not allow global destruction etc?
> I know this is not your code but do you mind changing this to:
> 
>   return exit_code;

Done.

http://codereview.chromium.org/2278003/diff/15001/16007
File chrome_frame/chrome_launcher_unittest.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16007#newcode20
chrome_frame/chrome_launcher_unittest.cc:20: CommandLine
good(FilePath(L"dummy.exe"));
On 2010/05/26 17:23:07, tommi wrote:
> maybe check an empty command line too?

Done.

http://codereview.chromium.org/2278003/diff/15001/16007#newcode49
chrome_frame/chrome_launcher_unittest.cc:49:
EXPECT_TRUE(chrome_launcher::IsValidArgument(L"--chrome-frame"));
On 2010/05/26 17:23:07, tommi wrote:
> test empty here too?

Done.

[robertshield, 2010-05-26 20:06:37]

On 2010/05/26 17:41:59, amit wrote:
> Haven't gone into the details yet but a few drive by comments...
> 
> - please check input args as much as possible

Input args are checked. Please look at the implementations of IsValidArgument()
and IsValidCommandLine() to see if they look sane.

> - this might be a good time to enable crash reporting

Crash reporting is enabled in chrome_launcher_main.cc

> - it would be nice if we could collect launch failure statistics (or at least
> log errors) in some way here.

We don't pull in base\logging and I didn't want to add much more code to
chrome_launcher.exe. My thinking is that logging launch failures from inside
chrome frame when it fails to launch chrome_launcher.exe ought to be enough. If
we wanted, we could add additional information in the process return code from
chrome_launcher and have chrome frame log that as well (although I'd rather do
that in a separate CL).

[tommi (sloooow) - chröme, 2010-05-26 20:14:29]

lgtm.

Btw, about the \n thing.  Was it ritvield or the merge process itself that
needed two \n's at eof?  I don't remember, but if you think it's fine as is
that's good enough for me.

http://codereview.chromium.org/2278003/diff/15001/16004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16004#newcode151
chrome_frame/chrome_launcher.cc:151: if (CreateProcess(NULL,
const_cast<wchar_t*>(chrome_path.c_str()),
> I seem to remember Raymond Chen pontificating on this at some point. Something
> about how this avoided an extra buffer being created once upon a time. I'll
use
> the &chrome_path[0] thing to remain politically correct. Wouldn't want to be
> politically incorrect, no... that would be bad.

thanks for not being sarcastic.

http://codereview.chromium.org/2278003/diff/24001/25004#newcode99
chrome_frame/chrome_launcher.cc:99: if (arg.length() < 2) {
gaa!! you said you fixed this!

http://codereview.chromium.org/2278003/diff/24001/25005
File chrome_frame/chrome_launcher.h (right):

http://codereview.chromium.org/2278003/diff/24001/25005#newcode10
chrome_frame/chrome_launcher.h:10: // arraysize macro shamelessly stolen from
base\bsaictypes.h
base\basictypes.h.  we stopped using bsaictypes a while ago.

http://codereview.chromium.org/2278003/diff/24001/25006
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25006#newcode92
chrome_frame/chrome_launcher_main.cc:92: delete breakpad;
nit: you could use scoped_ptr

[amit, 2010-05-26 20:54:41]

Looks awesome. LGTM with a few nits.

http://codereview.chromium.org/2278003/diff/24001/25004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25004#newcode101
chrome_frame/chrome_launcher.cc:101: if (arg.find(kAllowedSwitches[i], 2) == 2)
{
do we need to worry about case here?

http://codereview.chromium.org/2278003/diff/24001/25004#newcode144
chrome_frame/chrome_launcher.cc:144: chrome_path += args;
if (args) just to be safe

http://codereview.chromium.org/2278003/diff/24001/25004#newcode150
chrome_frame/chrome_launcher.cc:150: PROCESS_INFORMATION process_info;
nit: = {0}?

http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] = {0};
nit: * 4? :) Well, especially since the PathAppend does not take the size of the
destination. 

Ideally, we can and should keep track of the size we are appending.  Adding a
little helper should be easy:
AppendPath(const wchar_t* dst, const wchar_t* src, size_t dst_size, size_t
src_size)?

http://codereview.chromium.org/2278003/diff/24001/25007
File chrome_frame/chrome_launcher_unittest.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25007#newcode36
chrome_frame/chrome_launcher_unittest.cc:36: TEST(ChromeLauncher,
TrimWhiteSpace) {
nice tests!

http://codereview.chromium.org/2278003/diff/24001/25008
File chrome_frame/chrome_launcher_utils.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25008#newcode21
chrome_frame/chrome_launcher_utils.cc:21: CommandLine* CreateLaunchCommandLine()
{
The FilePath is mainly for platform independence, which is hardly a concern for
chrome_launcher.exe :) So it's possible to get rid of filePath dependency (and
even CommandLine) and just depend on WIN32 APIs.

Ignore this for now if this is used from more than one place. :)

[robertshield, 2010-05-26 21:51:44]

Thanks for your comments! Please take another look.

@tommi: the files do appear to have newlines at the end of them. I do not know
why rietveld is complaining. I can only assume malevolence on its part.

http://codereview.chromium.org/2278003/diff/15001/16004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/15001/16004#newcode151
chrome_frame/chrome_launcher.cc:151: if (CreateProcess(NULL,
const_cast<wchar_t*>(chrome_path.c_str()),
On 2010/05/26 20:14:29, tommi wrote:
> > I seem to remember Raymond Chen pontificating on this at some point.
Something
> > about how this avoided an extra buffer being created once upon a time. I'll
> use
> > the &chrome_path[0] thing to remain politically correct. Wouldn't want to be
> > politically incorrect, no... that would be bad.
> 
> thanks for not being sarcastic.

you're welcome.

http://codereview.chromium.org/2278003/diff/24001/25004#newcode99
chrome_frame/chrome_launcher.cc:99: if (arg.length() < 2) {
On 2010/05/26 20:14:29, tommi wrote:
> gaa!! you said you fixed this!

I am a treacherous knave! Fixed for realz this time.

http://codereview.chromium.org/2278003/diff/24001/25004#newcode101
chrome_frame/chrome_launcher.cc:101: }
On 2010/05/26 20:54:42, amit wrote:
> do we need to worry about case here?

Since we control both ends of this, I am explicitly allowing only exactly the
right casing. Can you think of a case where I would need to do a case
insensitive match?

http://codereview.chromium.org/2278003/diff/24001/25004#newcode144
chrome_frame/chrome_launcher.cc:144: chrome_path += args;
On 2010/05/26 20:54:42, amit wrote:
> if (args) just to be safe

good idea, done.

http://codereview.chromium.org/2278003/diff/24001/25004#newcode150
chrome_frame/chrome_launcher.cc:150: PROCESS_INFORMATION process_info;
On 2010/05/26 20:54:42, amit wrote:
> nit: = {0}?

Done.

http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] = {0};
On 2010/05/26 20:54:42, amit wrote:
> nit: * 4? :) Well, especially since the PathAppend does not take the size of
the
> destination. 
> 
> Ideally, we can and should keep track of the size we are appending.  Adding a
> little helper should be easy:
> AppendPath(const wchar_t* dst, const wchar_t* src, size_t dst_size, size_t
> src_size)?

The three places we append to this buffer are in the call to GetModuleFileName
in which we will not write more than arraysize(cur_path) / 2 characters and two
calls to PathAppend whereby fixed length strings are appended.

As long as strlen(kBrowserProcessExecutableName) * 2 < MAX_PATH holds true this
should be safe. Would you like me to add an _ASSERT that verifies that this is
so?

http://codereview.chromium.org/2278003/diff/24001/25005
File chrome_frame/chrome_launcher.h (right):

http://codereview.chromium.org/2278003/diff/24001/25005#newcode10
chrome_frame/chrome_launcher.h:10: // arraysize macro shamelessly stolen from
base\bsaictypes.h
On 2010/05/26 20:14:29, tommi wrote:
> base\basictypes.h.  we stopped using bsaictypes a while ago.

thanks. I always get the two mixed up. When we switch to bascitypes.h I'll
really be in trouble.

http://codereview.chromium.org/2278003/diff/24001/25006
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25006#newcode92
chrome_frame/chrome_launcher_main.cc:92: delete breakpad;
On 2010/05/26 20:14:29, tommi wrote:
> nit: you could use scoped_ptr

whaddya know, there's a scoped_ptr in breakpad. cool!

http://codereview.chromium.org/2278003/diff/24001/25007
File chrome_frame/chrome_launcher_unittest.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25007#newcode36
chrome_frame/chrome_launcher_unittest.cc:36: TEST(ChromeLauncher,
TrimWhiteSpace) {
On 2010/05/26 20:54:42, amit wrote:
> nice tests!

thanks!

http://codereview.chromium.org/2278003/diff/24001/25008
File chrome_frame/chrome_launcher_utils.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25008#newcode21
chrome_frame/chrome_launcher_utils.cc:21: CommandLine* CreateLaunchCommandLine()
{
On 2010/05/26 20:54:42, amit wrote:
> The FilePath is mainly for platform independence, which is hardly a concern
for
> chrome_launcher.exe :) So it's possible to get rid of filePath dependency (and
> even CommandLine) and just depend on WIN32 APIs.
> 
> Ignore this for now if this is used from more than one place. :)

This chunk of code is built into npchrome_frame.dll, so we're not taking any
additional dependencies here. Also, using FilePath placates the chrome
deprecation hunters :)

[tommi (sloooow) - chröme, 2010-05-27 02:41:04]

"lgtm"

On Wed, May 26, 2010 at 5:51 PM, <robertshield@chromium.org> wrote:

> Thanks for your comments! Please take another look.
>
> @tommi: the files do appear to have newlines at the end of them. I do not
> know
> why rietveld is complaining. I can only assume malevolence on its part.
>
>
> http://codereview.chromium.org/2278003/diff/15001/16004
>
> File chrome_frame/chrome_launcher.cc (right):
>
> http://codereview.chromium.org/2278003/diff/15001/16004#newcode151
> chrome_frame/chrome_launcher.cc:151: if (CreateProcess(NULL,
> const_cast<wchar_t*>(chrome_path.c_str()),
> On 2010/05/26 20:14:29, tommi wrote:
>
>> > I seem to remember Raymond Chen pontificating on this at some point.
>>
> Something
>
>> > about how this avoided an extra buffer being created once upon a
>>
> time. I'll
>
>> use
>> > the &chrome_path[0] thing to remain politically correct. Wouldn't
>>
> want to be
>
>> > politically incorrect, no... that would be bad.
>>
>
>  thanks for not being sarcastic.
>>
>
> you're welcome.
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode99
> chrome_frame/chrome_launcher.cc:99: if (arg.length() < 2) {
> On 2010/05/26 20:14:29, tommi wrote:
>
>> gaa!! you said you fixed this!
>>
>
> I am a treacherous knave! Fixed for realz this time.
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode101
> chrome_frame/chrome_launcher.cc:101: }
> On 2010/05/26 20:54:42, amit wrote:
>
>> do we need to worry about case here?
>>
>
> Since we control both ends of this, I am explicitly allowing only
> exactly the right casing. Can you think of a case where I would need to
> do a case insensitive match?
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode144
> chrome_frame/chrome_launcher.cc:144: chrome_path += args;
> On 2010/05/26 20:54:42, amit wrote:
>
>> if (args) just to be safe
>>
>
> good idea, done.
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode150
> chrome_frame/chrome_launcher.cc:150: PROCESS_INFORMATION process_info;
> On 2010/05/26 20:54:42, amit wrote:
>
>> nit: = {0}?
>>
>
> Done.
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
> chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] =
> {0};
> On 2010/05/26 20:54:42, amit wrote:
>
>> nit: * 4? :) Well, especially since the PathAppend does not take the
>>
> size of the
>
>> destination.
>>
>
>  Ideally, we can and should keep track of the size we are appending.
>>
> Adding a
>
>> little helper should be easy:
>> AppendPath(const wchar_t* dst, const wchar_t* src, size_t dst_size,
>>
> size_t
>
>> src_size)?
>>
>
> The three places we append to this buffer are in the call to
> GetModuleFileName in which we will not write more than
> arraysize(cur_path) / 2 characters and two calls to PathAppend whereby
> fixed length strings are appended.
>
> As long as strlen(kBrowserProcessExecutableName) * 2 < MAX_PATH holds
> true this should be safe. Would you like me to add an _ASSERT that
> verifies that this is so?
>
>
> http://codereview.chromium.org/2278003/diff/24001/25005
> File chrome_frame/chrome_launcher.h (right):
>
> http://codereview.chromium.org/2278003/diff/24001/25005#newcode10
> chrome_frame/chrome_launcher.h:10: // arraysize macro shamelessly stolen
> from base\bsaictypes.h
> On 2010/05/26 20:14:29, tommi wrote:
>
>> base\basictypes.h.  we stopped using bsaictypes a while ago.
>>
>
> thanks. I always get the two mixed up. When we switch to bascitypes.h
> I'll really be in trouble.
>
>
> http://codereview.chromium.org/2278003/diff/24001/25006
> File chrome_frame/chrome_launcher_main.cc (right):
>
> http://codereview.chromium.org/2278003/diff/24001/25006#newcode92
> chrome_frame/chrome_launcher_main.cc:92: delete breakpad;
> On 2010/05/26 20:14:29, tommi wrote:
>
>> nit: you could use scoped_ptr
>>
>
> whaddya know, there's a scoped_ptr in breakpad. cool!
>
>
> http://codereview.chromium.org/2278003/diff/24001/25007
> File chrome_frame/chrome_launcher_unittest.cc (right):
>
> http://codereview.chromium.org/2278003/diff/24001/25007#newcode36
> chrome_frame/chrome_launcher_unittest.cc:36: TEST(ChromeLauncher,
> TrimWhiteSpace) {
> On 2010/05/26 20:54:42, amit wrote:
>
>> nice tests!
>>
>
> thanks!
>
>
> http://codereview.chromium.org/2278003/diff/24001/25008
> File chrome_frame/chrome_launcher_utils.cc (right):
>
> http://codereview.chromium.org/2278003/diff/24001/25008#newcode21
> chrome_frame/chrome_launcher_utils.cc:21: CommandLine*
> CreateLaunchCommandLine() {
> On 2010/05/26 20:54:42, amit wrote:
>
>> The FilePath is mainly for platform independence, which is hardly a
>>
> concern for
>
>> chrome_launcher.exe :) So it's possible to get rid of filePath
>>
> dependency (and
>
>> even CommandLine) and just depend on WIN32 APIs.
>>
>
>  Ignore this for now if this is used from more than one place. :)
>>
>
> This chunk of code is built into npchrome_frame.dll, so we're not taking
> any additional dependencies here. Also, using FilePath placates the
> chrome deprecation hunters :)
>
>
> http://codereview.chromium.org/2278003/show
>

[amit, 2010-05-27 18:31:47]

http://codereview.chromium.org/2278003/diff/24001/25004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] = {0};
On 2010/05/26 21:51:44, robertshield wrote:
> On 2010/05/26 20:54:42, amit wrote:
> > nit: * 4? :) Well, especially since the PathAppend does not take the size of
> the
> > destination. 
> > 
> > Ideally, we can and should keep track of the size we are appending.  Adding
a
> > little helper should be easy:
> > AppendPath(const wchar_t* dst, const wchar_t* src, size_t dst_size, size_t
> > src_size)?
> 
> The three places we append to this buffer are in the call to GetModuleFileName
> in which we will not write more than arraysize(cur_path) / 2 characters and
two
> calls to PathAppend whereby fixed length strings are appended.
> 
> As long as strlen(kBrowserProcessExecutableName) * 2 < MAX_PATH holds true
this
> should be safe. Would you like me to add an _ASSERT that verifies that this is
> so?

Sorry for not being clear, we should gracefully handle the case if the module
path is greater than MAX_PATH. I was suggesting a bigger buffer as a quick and
easy way to do that.  Also, since now we can expect bigger paths in the buffer,
my suggestion was to guard against crash in PathAppend.

[robertshield, 2010-05-27 19:03:28]

Thanks!

http://codereview.chromium.org/2278003/diff/24001/25004
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] = {0};
On 2010/05/27 18:31:47, amit wrote:
> On 2010/05/26 21:51:44, robertshield wrote:
> > On 2010/05/26 20:54:42, amit wrote:
> > > nit: * 4? :) Well, especially since the PathAppend does not take the size
of
> > the
> > > destination. 
> > > 
> > > Ideally, we can and should keep track of the size we are appending. 
Adding
> a
> > > little helper should be easy:
> > > AppendPath(const wchar_t* dst, const wchar_t* src, size_t dst_size, size_t
> > > src_size)?
> > 
> > The three places we append to this buffer are in the call to
GetModuleFileName
> > in which we will not write more than arraysize(cur_path) / 2 characters and
> two
> > calls to PathAppend whereby fixed length strings are appended.
> > 
> > As long as strlen(kBrowserProcessExecutableName) * 2 < MAX_PATH holds true
> this
> > should be safe. Would you like me to add an _ASSERT that verifies that this
is
> > so?
> 
> Sorry for not being clear, we should gracefully handle the case if the module
> path is greater than MAX_PATH. I was suggesting a bigger buffer as a quick and
> easy way to do that.  Also, since now we can expect bigger paths in the
buffer,
> my suggestion was to guard against crash in PathAppend.
> 
> 
> 

ok, bumped the buffer length to MAX_PATH * 4. As discussed, the calls to
PathAppend are safe since the buffer will now always have MAX_PATH * 2 wchar_ts
of extra free space and kBrowserProcessExecutableName is a constant string of
length < MAX_PATH.

[amit, 2010-05-27 19:04:09]

LGTM

On Thu, May 27, 2010 at 12:03 PM, <robertshield@chromium.org> wrote:

> Thanks!
>
>
>
> http://codereview.chromium.org/2278003/diff/24001/25004
> File chrome_frame/chrome_launcher.cc (right):
>
> http://codereview.chromium.org/2278003/diff/24001/25004#newcode170
> chrome_frame/chrome_launcher.cc:170: wchar_t cur_path[MAX_PATH * 2] =
> {0};
> On 2010/05/27 18:31:47, amit wrote:
>
>> On 2010/05/26 21:51:44, robertshield wrote:
>> > On 2010/05/26 20:54:42, amit wrote:
>> > > nit: * 4? :) Well, especially since the PathAppend does not take
>>
> the size of
>
>> > the
>> > > destination.
>> > >
>> > > Ideally, we can and should keep track of the size we are
>>
> appending.  Adding
>
>> a
>> > > little helper should be easy:
>> > > AppendPath(const wchar_t* dst, const wchar_t* src, size_t
>>
> dst_size, size_t
>
>> > > src_size)?
>> >
>> > The three places we append to this buffer are in the call to
>>
> GetModuleFileName
>
>> > in which we will not write more than arraysize(cur_path) / 2
>>
> characters and
>
>> two
>> > calls to PathAppend whereby fixed length strings are appended.
>> >
>> > As long as strlen(kBrowserProcessExecutableName) * 2 < MAX_PATH
>>
> holds true
>
>> this
>> > should be safe. Would you like me to add an _ASSERT that verifies
>>
> that this is
>
>> > so?
>>
>
>  Sorry for not being clear, we should gracefully handle the case if the
>>
> module
>
>> path is greater than MAX_PATH. I was suggesting a bigger buffer as a
>>
> quick and
>
>> easy way to do that.  Also, since now we can expect bigger paths in
>>
> the buffer,
>
>> my suggestion was to guard against crash in PathAppend.
>>
>
>
>
>
> ok, bumped the buffer length to MAX_PATH * 4. As discussed, the calls to
> PathAppend are safe since the buffer will now always have MAX_PATH * 2
> wchar_ts of extra free space and kBrowserProcessExecutableName is a
> constant string of length < MAX_PATH.
>
>
> http://codereview.chromium.org/2278003/show
>

[xiyuan, 2010-05-28 20:40:32]

chrome_frame_launcher.gyp uses CRLF line-ending and breaks gyp on my dev machine
(cygwin on Windows).