Use a valid /prefetch argument when launching a process.

Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
Committed: https://crrev.com/343068c41c299e65a87d935654f0b81a9935250f
Cr-Commit-Position: refs/heads/master@{#373241}

[fdoray, 2016-01-20 22:26:30]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, jam@chromium.org

[fdoray, 2016-01-20 22:26:31]

gab@: Can you review components/startup_metric_utils/?
jam@: Can you review content/?

For now, prefetch_argument_win.h/.cc are in content_common. I'm postponing the
discussion about they could be used from chrome.exe (as opposed to chrome.dll)
to another CL.

Thanks!

[jam, 2016-01-20 23:17:36]

https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS
File content/public/common/DEPS (right):

https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS#...
content/public/common/DEPS:11:
"+components/startup_metric_utils/common/pre_read_field_trial_utils_win.h"
content shouldn't depend on this component, even temporarily.

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
File content/public/common/prefetch_argument_win.h (right):

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
content/public/common/prefetch_argument_win.h:5: #ifndef
CONTENT_PUBLIC_COMMON_PREFETCH_ARGUMENT_WIN_H_
this is is in content/public but it's not called from outside content. why is
that? my understanding from the email thread was that this needs to be called
from all places that chrome starts processes, but i don't see that done in this
change?

[fdoray, 2016-01-21 01:39:51]

jam@: I replied to your comments.

https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS
File content/public/common/DEPS (right):

https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS#...
content/public/common/DEPS:11:
"+components/startup_metric_utils/common/pre_read_field_trial_utils_win.h"
On 2016/01/20 23:17:36, jam wrote:
> content shouldn't depend on this component, even temporarily.
> 


Could I add the /prefetch argument in
ChromeContentBrowserClient::AppendExtraCommandLineSwitches to avoid changing
content/? I tried to do this in another CL [1], but I was told that:
- This shouldn't be in done chrome/, as all users of content/ should be able to
benefit from this optimization.
- It is better to set a prefetch id at the time a command line is created than
to inspect an existing command line to determine which prefetch id to use [2].

Note: content already depends on this component startup_metric_utils_common.
This component allows us to read the PreRead field trial options from the
registry. The options are stored in the registry because they are needed before
variations are read from the disk [4]. If we read the options from the registry
in some cases and from variations::GetVariationParamValue in other cases, we
could end up using different sets of options in the same process...

[1]
https://codereview.chromium.org/1595633002/diff/60001/chrome/browser/chrome_c...
[2]
https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/main_dl...

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
File content/public/common/prefetch_argument_win.h (right):

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
content/public/common/prefetch_argument_win.h:5: #ifndef
CONTENT_PUBLIC_COMMON_PREFETCH_ARGUMENT_WIN_H_
On 2016/01/20 23:17:36, jam wrote:
> this is is in content/public but it's not called from outside content. why is
> that? my understanding from the email thread was that this needs to be called
> from all places that chrome starts processes, but i don't see that done in
this
> change?

This will be used outside of content [1]. I wanted to add the other call sites
in a separate CL to keep this one small. Would you prefer that I add all the
call sites as part of this CL?

[1] It will be used at least at these places:
GenerateChromeWatcherCommandLine
BrowserProcessImpl::RestartBackgroundInstance
NaClBrokerListener::OnLaunchLoaderThroughBroker
CreateServiceProcessCommandLine
auto_launch_util::EnableBackgroundStartAtLogin

[jam, 2016-01-21 17:19:02]

On 2016/01/21 01:39:51, fdoray wrote:
> jam@: I replied to your comments.
> 
> https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS
> File content/public/common/DEPS (right):
> 
>
https://codereview.chromium.org/1612663002/diff/1/content/public/common/DEPS#...
> content/public/common/DEPS:11:
> "+components/startup_metric_utils/common/pre_read_field_trial_utils_win.h"
> On 2016/01/20 23:17:36, jam wrote:
> > content shouldn't depend on this component, even temporarily.
> > 
> 
> 
> Could I add the /prefetch argument in
> ChromeContentBrowserClient::AppendExtraCommandLineSwitches to avoid changing
> content/? I tried to do this in another CL [1], but I was told that:
> - This shouldn't be in done chrome/, as all users of content/ should be able
to
> benefit from this optimization.
> - It is better to set a prefetch id at the time a command line is created than
> to inspect an existing command line to determine which prefetch id to use [2].
> 
> Note: content already depends on this component startup_metric_utils_common.

I didn't realize this; that's incorrect actually.

In the change that added it,
https://codereview.chromium.org/1413533008/diff/140001/content/renderer/DEPS,
please see the comment in a few lines above:

# Allow inclusion of specific components that we depend on. We may only
# depend on components which we share with the mojo html_viewer.

That needs to be fixed now, since it's setting an incorrect example. It's
probably best to fix it in a separate change, and then we can get back to this
one.



> This component allows us to read the PreRead field trial options from the
> registry. The options are stored in the registry because they are needed
before
> variations are read from the disk [4]. If we read the options from the
registry
> in some cases and from variations::GetVariationParamValue in other cases, we
> could end up using different sets of options in the same process...

When is variations data read? i.e. is it read after we've launched any
processes?

> 
> [1]
>
https://codereview.chromium.org/1595633002/diff/60001/chrome/browser/chrome_c...
> [2]
>
https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
> [3]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re...
> [4]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/main_dl...
> 
>
https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
> File content/public/common/prefetch_argument_win.h (right):
> 
>
https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
> content/public/common/prefetch_argument_win.h:5: #ifndef
> CONTENT_PUBLIC_COMMON_PREFETCH_ARGUMENT_WIN_H_
> On 2016/01/20 23:17:36, jam wrote:
> > this is is in content/public but it's not called from outside content. why
is
> > that? my understanding from the email thread was that this needs to be
called
> > from all places that chrome starts processes, but i don't see that done in
> this
> > change?
> 
> This will be used outside of content [1]. I wanted to add the other call sites
> in a separate CL to keep this one small. Would you prefer that I add all the
> call sites as part of this CL?
> 
> [1] It will be used at least at these places:
> GenerateChromeWatcherCommandLine
> BrowserProcessImpl::RestartBackgroundInstance
> NaClBrokerListener::OnLaunchLoaderThroughBroker
> CreateServiceProcessCommandLine
> auto_launch_util::EnableBackgroundStartAtLogin

[gab, 2016-01-21 20:22:59]

lg % DEPS discussion and a few things below

https://codereview.chromium.org/1612663002/diff/1/content/browser/plugin_proc...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/1612663002/diff/1/content/browser/plugin_proc...
content/browser/plugin_process_host.cc:214:
AddWindowsPrefetchArgument(PREFETCH_ID_DEFAULT, cmd_line);
How about "PREFETCH_ID_OTHER" (i.e. to me "default" is the browser process'
while "other" is clear it's for things we don't care much about).

https://codereview.chromium.org/1612663002/diff/1/content/browser/ppapi_plugi...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/1612663002/diff/1/content/browser/ppapi_plugi...
content/browser/ppapi_plugin_process_host.cc:374: is_broker_ ?
PREFETCH_ID_DEFAULT : PREFETCH_ID_PPAPI, cmd_line);
What made you decide to have a special ID for one but not the other? Add a
comment explaining why?

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
File content/public/common/prefetch_argument_win.h (right):

https://codereview.chromium.org/1612663002/diff/1/content/public/common/prefe...
content/public/common/prefetch_argument_win.h:5: #ifndef
CONTENT_PUBLIC_COMMON_PREFETCH_ARGUMENT_WIN_H_
On 2016/01/21 01:39:51, fdoray wrote:
> On 2016/01/20 23:17:36, jam wrote:
> > this is is in content/public but it's not called from outside content. why
is
> > that? my understanding from the email thread was that this needs to be
called
> > from all places that chrome starts processes, but i don't see that done in
> this
> > change?
> 
> This will be used outside of content [1]. I wanted to add the other call sites
> in a separate CL to keep this one small. Would you prefer that I add all the
> call sites as part of this CL?
> 
> [1] It will be used at least at these places:
> GenerateChromeWatcherCommandLine
> BrowserProcessImpl::RestartBackgroundInstance
> NaClBrokerListener::OnLaunchLoaderThroughBroker
> CreateServiceProcessCommandLine
> auto_launch_util::EnableBackgroundStartAtLogin

As much as I like smaller CLs I think it's better to put it all together in this
case. As-is this CL actually makes an invalid revision prefetching wise since
some processes still share prefetch profiles with the browser process. Plus it
introduces unused code and an unused public API.

So I'd vote for putting it all in this CL (and adding a DCHECK that a process
either has /prefetch or is the browser process (no kProcessType)).

And also, I'd like to see a browser_test that we indeed do generate multiple
prefetching profiles (i.e. a test that fails before and passes with this CL).
Feels like a simple browser_test that verifies in its body that the expected .pf
files were updated on disk should be sufficient.

[fdoray, 2016-01-21 21:17:59]

Description was changed from

==========
Use a valid /prefetch argument when launching a process from content.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launched from content (except when the feature is disabled by
the PreRead field trial).

BUG=577698
==========

to

==========
Use valid /prefetch arguments when launching a processes.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

[fdoray, 2016-01-22 15:21:24]

Description was changed from

==========
Use valid /prefetch arguments when launching a processes.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

to

==========
Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

[fdoray, 2016-01-26 17:04:20]

gab@: Can you take a look at what I did? I'm working on the browser test, will
upload when it's done.

Instead of having an helper function AddWindowsPrefetchArgument(int,
CommandLine*), I decided to define multiple kPrefetchArgument* constants and
call cmd_line->AppendArg() directly where it's needed.

Why?
- I wouldn't have been possible to add the helper function to
//content/public/common:static_switches. The helper function needs base, and
static_switches cannot depend on base because it is linked statically with other
targets (symbols are defined multiple times in these targets if static_switches
depends on base).
- I wouldn't have been possible to add the helper function to
//content/public/common. chrome.exe needs the helper function but cannot depend
on //content/public/common, as explained here [1].

Note that the helper function couldn't have been responsible to check whether
the PreRead field trial options:
- content cannot depend on startup_metric_utils_common.
- The helper function cannot use ContentBrowserClient, because we need to add a
/prefetch argument at places where none is available.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_exe....

[gab, 2016-01-26 21:40:26]

So many dependency rules at play here.. sigh..!

I think this works with a bit more explicit documentation as hihglighted in
comments below.

Thanks for working on this,
Gab

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:170: DCHECK_EQ(storage.size(),
arg_append_index);
Don't think this DCHECK is necessary, it's pretty obvious.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:207:
DCHECK(!startup_metric_utils::GetPreReadOptions().prefetch_argument ||
Reading this here makes me realize that |prefetch_argument| doesn't feel like a
bool, but instead like some kind of property, whereas |use_prefetch_argument|
would be obvious and not require the reader to double-check that API while
reading this code.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:207:
DCHECK(!startup_metric_utils::GetPreReadOptions().prefetch_argument ||
Add a comment about what this DCHECK verifies, e.g.:

// Confirm that an explicit prefetch profile is used for all process types
except for the browser process (apart from background mode which is assigned an
explicit prefetch profile as well). Any new process type will have to assign
itself a prefetch profile (see foo.h for details).

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:209:
!command_line->HasSwitch(switches::kNoStartupWindow)) ||
This will DCHECK on any existing install which uses background mode as the
registry isn't updated by this CL (only future commands dropped in  registry).

Perhaps it's okay to ignore background mode in this DCHECK since it's mostly
meant to catch regressions for new process types (or catch ones we may have
forgotten in this one).

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/prefetch_arg...
File chrome/app/prefetch_argument_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/prefetch_arg...
chrome/app/prefetch_argument_win.cc:12: const size_t kPrefetchArgumentSize =
arraysize(kPrefetchArgumentPrefix);
arraysize() is a compile-time constant, no need to extract it in a constant,
it's just as efficient to inline it everywhere

https://codereview.chromium.org/1612663002/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:172:
command_line_.AppendArg(switches::kPrefetchArgumentRenderer);
This is a unittest, I would guess it doesn't go through the DCHECK in main, no?
If so, I'd say don't put it here.

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:90:
arguments.push_back("/prefetch:8");
I hate having to use hardcoded strings, why can't this one refer to the constant
like other components do?

At least add a comment reference to kPrefetchArgumentOther?

https://codereview.chromium.org/1612663002/diff/80001/components/startup_metr...
File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc
(right):

https://codereview.chromium.org/1612663002/diff/80001/components/startup_metr...
components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:34:
const base::char16 kNoPrefetchArgumentVariationName[] = L"NoPrefetchArgument";
It may not be clear to a reader that the reason some of those are "negative" is
that they all change the default, i.e. the default is the inverse of the
variation and thus variations that cancel something we do by default are
negative.

How about describing this in the meta comment for these variable names?

https://codereview.chromium.org/1612663002/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1612663002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:755: // Returns true if
processes should be launched with a /prefetch:# argument.
Also refer to a place where you explain the details of /prefetch (e.g.
kPrefetchArgument* section of the switches.cc file?) -- i.e. think of any
content embedder seeing this new API and wondering what to do with it.

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:971: // argument. Also, # must always
be in [1, 8]; otherwise it is ignored by the
s/Also, /Note: /

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:971: // argument. Also, # must always
be in [1, 8]; otherwise it is ignored by the
"Note that the browser process has no /prefetch:# argument; as such all other
processes must have one in order to avoid polluting its profile."

(I see now that this is mentioned on kPrefetchArgumentOther, but still think
it's worth mentioning it here?)

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:977: 
I would really really like for all of this logic to live in one place as much as
possible...

I guess now we're only splitting off two prefetch IDs which live in Chrome.

I understand how it's hard/impossible-without-breaking-dependency-rules but I'm
worried that we use some profiles in chrome/ and it's not documented here that
those shouldn't be used by content/.

Perhaps adding a comment here like this would help:

// /prefetch:5 is unused to this date and can be claimed by a new process type.
// /prefetch:6 and /prefetch:7 are reserved for content embedders and are not to
be used by content itself.

(and obviously change chrome from using 5/6 to 6/7)

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:982: // interfering with the prefetch
profile of the browser process.
How about:

"However having a valid prefetch argument for the process types is required to
prevent them from interfering with the prefetch profile of the browser process."

(i.e. add "is required" to make this statement stronger)

[fdoray, 2016-01-27 20:12:19]

gab@: ptal

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:170: DCHECK_EQ(storage.size(),
arg_append_index);
On 2016/01/26 21:40:25, gab wrote:
> Don't think this DCHECK is necessary, it's pretty obvious.

Done.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:207:
DCHECK(!startup_metric_utils::GetPreReadOptions().prefetch_argument ||
On 2016/01/26 21:40:25, gab wrote:
> Add a comment about what this DCHECK verifies, e.g.:
> 
> // Confirm that an explicit prefetch profile is used for all process types
> except for the browser process (apart from background mode which is assigned
an
> explicit prefetch profile as well). Any new process type will have to assign
> itself a prefetch profile (see foo.h for details).

Done.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:207:
DCHECK(!startup_metric_utils::GetPreReadOptions().prefetch_argument ||
On 2016/01/26 21:40:25, gab wrote:
> Reading this here makes me realize that |prefetch_argument| doesn't feel like
a
> bool, but instead like some kind of property, whereas |use_prefetch_argument|
> would be obvious and not require the reader to double-check that API while
> reading this code.

Done.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:209:
!command_line->HasSwitch(switches::kNoStartupWindow)) ||
On 2016/01/26 21:40:25, gab wrote:
> This will DCHECK on any existing install which uses background mode as the
> registry isn't updated by this CL (only future commands dropped in  registry).
> 
> Perhaps it's okay to ignore background mode in this DCHECK since it's mostly
> meant to catch regressions for new process types (or catch ones we may have
> forgotten in this one).

Done. Note: According to my debugging, the background mode autorun entry is
updated by [1] every time Chrome is launched. With my previous DCHECK, the user
only had to run Chrome once in non-background mode to eliminate the failure (and
I guess few people have an autorun entry for a debug build of Chrome?).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/prefetch_arg...
File chrome/app/prefetch_argument_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/prefetch_arg...
chrome/app/prefetch_argument_win.cc:12: const size_t kPrefetchArgumentSize =
arraysize(kPrefetchArgumentPrefix);
On 2016/01/26 21:40:26, gab wrote:
> arraysize() is a compile-time constant, no need to extract it in a constant,
> it's just as efficient to inline it everywhere

Done.

https://codereview.chromium.org/1612663002/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:172:
command_line_.AppendArg(switches::kPrefetchArgumentRenderer);
On 2016/01/26 21:40:26, gab wrote:
> This is a unittest, I would guess it doesn't go through the DCHECK in main,
no?
> If so, I'd say don't put it here.

Done.

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:90:
arguments.push_back("/prefetch:8");
On 2016/01/26 21:40:26, gab wrote:
> I hate having to use hardcoded strings, why can't this one refer to the
constant
> like other components do?
> 
> At least add a comment reference to kPrefetchArgumentOther?

Done. I was scared of adding the crashpad->content_common dependency. We'll see
what the owners think about it :) I can't add a crashpad->static_switches
dependency, because I get "symbols defined multiple times" errors when crashpad
is linked with targets that already depend on content_common.

https://codereview.chromium.org/1612663002/diff/80001/components/startup_metr...
File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc
(right):

https://codereview.chromium.org/1612663002/diff/80001/components/startup_metr...
components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:34:
const base::char16 kNoPrefetchArgumentVariationName[] = L"NoPrefetchArgument";
On 2016/01/26 21:40:26, gab wrote:
> It may not be clear to a reader that the reason some of those are "negative"
is
> that they all change the default, i.e. the default is the inverse of the
> variation and thus variations that cancel something we do by default are
> negative.
> 
> How about describing this in the meta comment for these variable names?

Done.

https://codereview.chromium.org/1612663002/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1612663002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:755: // Returns true if
processes should be launched with a /prefetch:# argument.
On 2016/01/26 21:40:26, gab wrote:
> Also refer to a place where you explain the details of /prefetch (e.g.
> kPrefetchArgument* section of the switches.cc file?) -- i.e. think of any
> content embedder seeing this new API and wondering what to do with it.

Done.

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:971: // argument. Also, # must always
be in [1, 8]; otherwise it is ignored by the
On 2016/01/26 21:40:26, gab wrote:
> s/Also, /Note: /

Done.

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:971: // argument. Also, # must always
be in [1, 8]; otherwise it is ignored by the
On 2016/01/26 21:40:26, gab wrote:
> "Note that the browser process has no /prefetch:# argument; as such all other
> processes must have one in order to avoid polluting its profile."
> 
> (I see now that this is mentioned on kPrefetchArgumentOther, but still think
> it's worth mentioning it here?)

Done.

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:977: 
On 2016/01/26 21:40:26, gab wrote:
> I would really really like for all of this logic to live in one place as much
as
> possible...
> 
> I guess now we're only splitting off two prefetch IDs which live in Chrome.
> 
> I understand how it's hard/impossible-without-breaking-dependency-rules but
I'm
> worried that we use some profiles in chrome/ and it's not documented here that
> those shouldn't be used by content/.
> 
> Perhaps adding a comment here like this would help:
> 
> // /prefetch:5 is unused to this date and can be claimed by a new process
type.
> // /prefetch:6 and /prefetch:7 are reserved for content embedders and are not
to
> be used by content itself.
> 
> (and obviously change chrome from using 5/6 to 6/7)

Done.

https://codereview.chromium.org/1612663002/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:982: // interfering with the prefetch
profile of the browser process.
On 2016/01/26 21:40:26, gab wrote:
> How about:
> 
> "However having a valid prefetch argument for the process types is required to
> prevent them from interfering with the prefetch profile of the browser
process."
> 
> (i.e. add "is required" to make this statement stronger)

Done.

[fdoray, 2016-01-28 22:11:06]

gab@ and jam@: Can you take another look at this CL? Thanks.

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/components/crash/conten...
components/crash/content/app/crashpad_win.cc:90:
arguments.push_back("/prefetch:8");
On 2016/01/27 20:12:19, fdoray wrote:
> On 2016/01/26 21:40:26, gab wrote:
> > I hate having to use hardcoded strings, why can't this one refer to the
> constant
> > like other components do?
> > 
> > At least add a comment reference to kPrefetchArgumentOther?
> 
> Done. I was scared of adding the crashpad->content_common dependency. We'll
see
> what the owners think about it :) I can't add a crashpad->static_switches
> dependency, because I get "symbols defined multiple times" errors when
crashpad
> is linked with targets that already depend on content_common.

Unfortunately, I can't use the constant here.
- The constant is defined in //content/public/common and
//content/public/common:static_switches. 
- //components/html_viewer depends on crashpad and
//content/public/common:static_switches -> symbols defined multiple times if
crashpad depends on //content/public/common
- //chrome/browser depends on crashpad and //content/public/common -> symbols
defined multiple times if crashpad depends on
//content/public/common:static_switches

[gab, 2016-01-29 12:46:25]

lvg, still need to do a full pass when I get to the office, but looking at the
diff I think I'm happy with this

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:209:
!command_line->HasSwitch(switches::kNoStartupWindow)) ||
On 2016/01/27 20:12:19, fdoray wrote:
> On 2016/01/26 21:40:25, gab wrote:
> > This will DCHECK on any existing install which uses background mode as the
> > registry isn't updated by this CL (only future commands dropped in 
registry).
> > 
> > Perhaps it's okay to ignore background mode in this DCHECK since it's mostly
> > meant to catch regressions for new process types (or catch ones we may have
> > forgotten in this one).
> 
> Done. Note: According to my debugging, the background mode autorun entry is
> updated by [1] every time Chrome is launched. With my previous DCHECK, the
user
> only had to run Chrome once in non-background mode to eliminate the failure
(and
> I guess few people have an autorun entry for a debug build of Chrome?).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...

Hmmm, are you sure this code kicks in on every startup? FWICT the way it's
written it's only meant to kick in on state change.

https://codereview.chromium.org/1612663002/diff/100001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/100001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:210: // content_switches.cc).
s/)/ for details)/

https://codereview.chromium.org/1612663002/diff/100001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1612663002/diff/100001/content/public/browser...
content/public/browser/content_browser_client.h:756: // See the
kPrefetchArgument* constants in content_switches.cc.
s/./ for details./

[fdoray, 2016-01-29 15:33:30]

gab@: Done. PTAL.

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:209:
!command_line->HasSwitch(switches::kNoStartupWindow)) ||
On 2016/01/29 12:46:25, gab wrote:
> On 2016/01/27 20:12:19, fdoray wrote:
> > On 2016/01/26 21:40:25, gab wrote:
> > > This will DCHECK on any existing install which uses background mode as the
> > > registry isn't updated by this CL (only future commands dropped in 
> registry).
> > > 
> > > Perhaps it's okay to ignore background mode in this DCHECK since it's
mostly
> > > meant to catch regressions for new process types (or catch ones we may
have
> > > forgotten in this one).
> > 
> > Done. Note: According to my debugging, the background mode autorun entry is
> > updated by [1] every time Chrome is launched. With my previous DCHECK, the
> user
> > only had to run Chrome once in non-background mode to eliminate the failure
> (and
> > I guess few people have an autorun entry for a debug build of Chrome?).
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...
> 
> Hmmm, are you sure this code kicks in on every startup? FWICT the way it's
> written it's only meant to kick in on state change.

When I have a background app installed,
BackgroundModeManager::OnApplicationListChanged is called and the registry is
updated on every startup.

Stack:
  BackgroundModeManager::EnableLaunchOnStartup
  BackgroundModeManager::OnClientsChanged
  BackgroundModeManager::OnApplicationListChanged
  BackgroundApplicationListModel::Update
  BackgroundApplicationListModel::Observe
  content::NotificationServiceImpl::Notify
  BackgroundContentsService::SendChangeNotification
  BackgroundContentsService::Observe
  content::NotificationServiceImpl::Notify
  ExtensionService::SetReadyAndNotifyListeners
  ExtensionService::Init
  ...

https://codereview.chromium.org/1612663002/diff/100001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/100001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:210: // content_switches.cc).
On 2016/01/29 12:46:25, gab wrote:
> s/)/ for details)/

Done.

https://codereview.chromium.org/1612663002/diff/100001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1612663002/diff/100001/content/public/browser...
content/public/browser/content_browser_client.h:756: // See the
kPrefetchArgument* constants in content_switches.cc.
On 2016/01/29 12:46:25, gab wrote:
> s/./ for details./

Done.

[gab, 2016-01-29 19:39:28]

Just did a full pass, LGTM w/ a few minor comments :-)!

(still kind of sad about the whole dependency story, but I think you came up
with the best thing possible given the constraints)

@jam for OWNERS

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/80001/chrome/app/chrome_exe_m...
chrome/app/chrome_exe_main_win.cc:209:
!command_line->HasSwitch(switches::kNoStartupWindow)) ||
On 2016/01/29 15:33:30, fdoray wrote:
> On 2016/01/29 12:46:25, gab wrote:
> > On 2016/01/27 20:12:19, fdoray wrote:
> > > On 2016/01/26 21:40:25, gab wrote:
> > > > This will DCHECK on any existing install which uses background mode as
the
> > > > registry isn't updated by this CL (only future commands dropped in 
> > registry).
> > > > 
> > > > Perhaps it's okay to ignore background mode in this DCHECK since it's
> mostly
> > > > meant to catch regressions for new process types (or catch ones we may
> have
> > > > forgotten in this one).
> > > 
> > > Done. Note: According to my debugging, the background mode autorun entry
is
> > > updated by [1] every time Chrome is launched. With my previous DCHECK, the
> > user
> > > only had to run Chrome once in non-background mode to eliminate the
failure
> > (and
> > > I guess few people have an autorun entry for a debug build of Chrome?).
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...
> > 
> > Hmmm, are you sure this code kicks in on every startup? FWICT the way it's
> > written it's only meant to kick in on state change.
> 
> When I have a background app installed,
> BackgroundModeManager::OnApplicationListChanged is called and the registry is
> updated on every startup.
> 
> Stack:
>   BackgroundModeManager::EnableLaunchOnStartup
>   BackgroundModeManager::OnClientsChanged
>   BackgroundModeManager::OnApplicationListChanged
>   BackgroundApplicationListModel::Update
>   BackgroundApplicationListModel::Observe
>   content::NotificationServiceImpl::Notify
>   BackgroundContentsService::SendChangeNotification
>   BackgroundContentsService::Observe
>   content::NotificationServiceImpl::Notify
>   ExtensionService::SetReadyAndNotifyListeners
>   ExtensionService::Init
>   ...

Interesting, this is certainly unexpected for me and I doubt this is
intentional. The code handling the notification really seems to be handling a
change in state. 

In particular, BackgroundContentsService::Observe() is reacting to
NOTIFICATION_EXTENSIONS_READY_DEPRECATED :-(.

Anyways, in this case it works out in our favor I guess...

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:162: // Remove the /prefetch:# argument as it
is not supported by Crashpad.
Is it really /prefetch or any argument that starts with / ?

I'd assume the later, if I'd say check for the generic case instead of
hardcoding "/prefetch" in yet another place.

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
File chrome/app/prefetch_argument_win.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
chrome/app/prefetch_argument_win.cc:10: bool
HasValidWindowsPrefetchArgument(const base::CommandLine& command_line) {
This is only used for a DCHECK, how about moving it to an anonymous method in
that file?

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
chrome/app/prefetch_argument_win.cc:19: arg[arraysize(kPrefetchArgumentPrefix) -
1] >= L'1' &&
return arg[arraysize(kPrefetchArgumentPrefix) - 1] >= L'1' &&
arg[arraysize(kPrefetchArgumentPrefix) - 1] >= L'1' &&;

(i.e. if we find an arg that matches but isn't 1-8, we shouldn't keep looking)

https://codereview.chromium.org/1612663002/diff/180001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1262: // kPrefetchArgument* constants defined
in content_switches.cc.
s/These constants must.../Use profiles 6 and 7 as documented on
kPrefetchArgument* in content_switches.cc./

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
content/public/common/content_switches.cc:977: // ignored by the// Windows
prefetcher.
rm "//" mid sentence.

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
content/public/common/content_switches.cc:982: 
I'd remove this empty line to make it even clearer that the follow-up comments
are about the next IDs.

[fdoray, 2016-02-01 13:28:26]

jam@: Can you review this CL?
gab@: All done.

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:162: // Remove the /prefetch:# argument as it
is not supported by Crashpad.
On 2016/01/29 19:39:28, gab wrote:
> Is it really /prefetch or any argument that starts with / ?
> 
> I'd assume the later, if I'd say check for the generic case instead of
> hardcoding "/prefetch" in yet another place.

Done.

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
File chrome/app/prefetch_argument_win.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
chrome/app/prefetch_argument_win.cc:10: bool
HasValidWindowsPrefetchArgument(const base::CommandLine& command_line) {
On 2016/01/29 19:39:28, gab wrote:
> This is only used for a DCHECK, how about moving it to an anonymous method in
> that file?

Done.

https://codereview.chromium.org/1612663002/diff/180001/chrome/app/prefetch_ar...
chrome/app/prefetch_argument_win.cc:19: arg[arraysize(kPrefetchArgumentPrefix) -
1] >= L'1' &&
On 2016/01/29 19:39:28, gab wrote:
> return arg[arraysize(kPrefetchArgumentPrefix) - 1] >= L'1' &&
> arg[arraysize(kPrefetchArgumentPrefix) - 1] >= L'1' &&;
> 
> (i.e. if we find an arg that matches but isn't 1-8, we shouldn't keep looking)

Done.

https://codereview.chromium.org/1612663002/diff/180001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1262: // kPrefetchArgument* constants defined
in content_switches.cc.
On 2016/01/29 19:39:28, gab wrote:
> s/These constants must.../Use profiles 6 and 7 as documented on
> kPrefetchArgument* in content_switches.cc./

Done.

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
content/public/common/content_switches.cc:977: // ignored by the// Windows
prefetcher.
On 2016/01/29 19:39:28, gab wrote:
> rm "//" mid sentence.

Done.

https://codereview.chromium.org/1612663002/diff/180001/content/public/common/...
content/public/common/content_switches.cc:982: 
On 2016/01/29 19:39:28, gab wrote:
> I'd remove this empty line to make it even clearer that the follow-up comments
> are about the next IDs.

Done.

[jam, 2016-02-01 18:04:43]

content looks good now, but I see content/renderer still depends on
components/startup_metric_utils/common. can that be removed before landing this
please?

[fdoray, 2016-02-01 21:53:37]

On 2016/02/01 18:04:43, jam wrote:
> content looks good now, but I see content/renderer still depends on
> components/startup_metric_utils/common. can that be removed before landing
this
> please?

Done here: https://codereview.chromium.org/1656983002/

[jam, 2016-02-02 23:51:54]

lgtm

[fdoray, 2016-02-03 03:00:13]

fdoray@chromium.org changed reviewers:
+ wfh@chromium.org

[fdoray, 2016-02-03 03:00:14]

+wfh: Can you review changes in content/common/sandbox_win.cc? Thanks.

[Will Harris, 2016-02-03 04:09:54]

On 2016/02/03 03:00:14, fdoray wrote:
> +wfh: Can you review changes in content/common/sandbox_win.cc? Thanks.

I was going to suggest ContentBrowserClient::ShouldUseWindowsPrefetchArgument
instead take a content::ProcessType and return a string of the prefetch
argument, so you can delegate the whole thing to the embedder, similar to the
app container code.

...but then I read the tale of dependency woe and regret in the previous code
review comments, and it seems like all these options have already been
considered and condemned to dependency hell/purgatory... so...

lgtm for content/common/sandbox_win.cc

p.s. did you consider applying a prefetch argument to the crashpad process?

[fdoray, 2016-02-03 13:51:19]

On 2016/02/03 04:09:54, Will Harris wrote:
> On 2016/02/03 03:00:14, fdoray wrote:
> > +wfh: Can you review changes in content/common/sandbox_win.cc? Thanks.
> 
> I was going to suggest ContentBrowserClient::ShouldUseWindowsPrefetchArgument
> instead take a content::ProcessType and return a string of the prefetch
> argument, so you can delegate the whole thing to the embedder, similar to the
> app container code.
> 
> ...but then I read the tale of dependency woe and regret in the previous code
> review comments, and it seems like all these options have already been
> considered and condemned to dependency hell/purgatory... so...
> 
> lgtm for content/common/sandbox_win.cc
> 
> p.s. did you consider applying a prefetch argument to the crashpad process?

thanks for the review!

I added a /prefetch argument to the crashpad process. I wasn't sure if it was
the process that needed it the most, but we may as well use all the available
/prefetch arguments.

[fdoray, 2016-02-03 13:51:27]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-02-03 13:51:27]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
jam@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1612663002/#ps300001
(title: "rebase + prefetch argument for crashpad")

[commit-bot: I haz the power, 2016-02-03 13:51:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1612663002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1612663002/300001

[commit-bot: I haz the power, 2016-02-03 15:46:07]

Description was changed from

==========
Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

to

==========
Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

[commit-bot: I haz the power, 2016-02-03 15:46:08]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-02-03 15:47:16]

Description was changed from

==========
Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698
==========

to

==========
Use a valid /prefetch argument when launching a process.

The /prefetch:# argument that is currently added to the command line of
processes launched with content::StartSandboxedProcess is ignored by
Windows because # doesn't respect the [1, 8] constraint.

This CL ensures that a valid /prefetch:# argument is used for every
process launch (except when the feature is disabled by the PreRead
field trial).

BUG=577698

Committed: https://crrev.com/343068c41c299e65a87d935654f0b81a9935250f
Cr-Commit-Position: refs/heads/master@{#373241}
==========

[commit-bot: I haz the power, 2016-02-03 15:47:17]

Patchset 16 (id:??) landed as
https://crrev.com/343068c41c299e65a87d935654f0b81a9935250f
Cr-Commit-Position: refs/heads/master@{#373241}