Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 131632 FieldTrials should use a better IPC mechanism than command line in Windows
Starred by 4 users Project Member Reported by jar@chromium.org, Jun 7 2012 Back to list
Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 663918



Sign in to add a comment
Version: all
OS: Possibly most critical to Windows (which has tigher limits on command line length)

What steps will reproduce the problem?
1. Create a lot of field trials in the browser
2. Check to see how many field trials are visible in subprocesses (renderer?)
3. Find that not all are visible
4. ... or worse... find that the renderer is not working!

The list of (already defined) field trials are passed as command line arguments to the subprocesses.  Windows has  limit on the length of the command line.  With sufficiently many field trials, we will exceed that limit, and then we'll either truncate our list (bad), or lose other command line args that need to be set for sub-process startup.

This issue was brought up by Carlos, but it hasn't yet(?) exceeded the limits... but it surely will as various projects create many more field trials.

The long term solution is to probably move most of our sub-process settings (not just field trials) off the command line, and into some other IPC mechanism, such as a shared memory segment.


 
Cc: stevet@chromium.org asvitk...@chromium.org jwd@chromium.org
Good point. Thanks for bringing this up. I believe MAX_PATH is like 256 or something, which really isn't that long.

This is going to create some difficulties since we're going to have to do a little work to guarantee that the other processes receive this roughly at startup time... which we sort of got for free from command line arguments.

Comment 3 by cpu@chromium.org, Jun 9 2012
My suggestion is that we create a read-only shared memory area that has not a long string but a JSON or some other structured tree-based key value system. Then we pass the identifier via command line

Like:

--ptype=renderer --options=@{233423466-4434}

The "@' sign means that the following is a named memory map on windows and whatever on the other platforms.

So on windows you would call http://msdn.microsoft.com/en-us/library/windows/desktop/aa366791(v=vs.85).aspx

with {233423466-4434}

If you are debugging or whatever maybe you can force them to be explicitly passed:

--ptype=renderer --options="<whatever format as a string here>"

I think the shared-memory area makes sense, though I don't think we necessarily want the overhead of using JSON here. I think it makes sense to just use the same format string as we do now, but stick it into shared memory.

We can later revisit using JSON if we decide we need to expand the amount of info we pass, but for now I don't think it's necessary and will just add extra overhead.
One other thing to consider would be whether we're losing any debugging capability by not having these in the command-line.

Currently, it is really easy to tell what trials are active in renderers by checking the command-lines. Will it be just as easy with shared memory?
Comment 6 by stevet@chromium.org, Jun 11 2012
Would logging it in the new process be sufficient?
Logging won't be sufficient for crash reports.

I've filed http://b/6640546 which we probably want to resolve before fixing this.
Labels: OS-Chrome
We think we just hit renderer problems on Chrome OS due to too-long command lines, see issue 154409.  It looks like we crossed 2048 bytes for the command line on some systems that add a lot of flags.

Comment 9 by cpu@chromium.org, Oct 16 2012
asvitkine, debugging concerns aside chrome won't run if you keep increasing the command line length.

Many things, like currently/last active url are visible in the crash/ and they are not in the command line, ask eroman@ for more detail.

Project Member Comment 10 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-Internals Cr-Internals
Cc: georgesak@chromium.org
Labels: -Cr-Internals Cr-Internals-Metrics
Per http://support2.microsoft.com/kb/830473, the limit is 8191 on Windows XP+.
Project Member Comment 13 by bugdroid1@chromium.org, Nov 11 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85e05a7313002a82231cf27186438813da591196

commit 85e05a7313002a82231cf27186438813da591196
Author: georgesak <georgesak@chromium.org>
Date: Tue Nov 11 17:19:43 2014

Added in-place testing for base64 encoding/decoding.

This is used by issue 706073003.

BUG= 131632 

Review URL: https://codereview.chromium.org/694993006

Cr-Commit-Position: refs/heads/master@{#303664}

[modify] https://chromium.googlesource.com/chromium/src.git/+/85e05a7313002a82231cf27186438813da591196/base/base64.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/85e05a7313002a82231cf27186438813da591196/base/base64_unittest.cc

Comment 14 by cpu@chromium.org, May 18 2016
Cc: -cpu@chromium.org
Cc: -stevet@chromium.org -odean@chromium.org -mad@chromium.org -rtenneti@chromium.org
Owner: lawrencewu@chromium.org
Status: Assigned
Project Member Comment 16 by bugdroid1@chromium.org, Oct 13 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c93082dcc33aeaa98f7ab9f2f20ef647448372f

commit 2c93082dcc33aeaa98f7ab9f2f20ef647448372f
Author: lawrencewu <lawrencewu@chromium.org>
Date: Thu Oct 13 13:54:25 2016

Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG= 131632 

Review-Url: https://codereview.chromium.org/2365273004
Cr-Commit-Position: refs/heads/master@{#425013}

[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/base/metrics/field_trial.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/base/metrics/field_trial.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/base/metrics/field_trial_unittest.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/components/nacl/browser/nacl_broker_host_win.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/components/nacl/browser/nacl_process_host.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/app/content_main_runner.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/browser_child_process_host_impl.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/child_process_launcher.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/child_process_launcher.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/gpu/gpu_process_host.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/ppapi_plugin_process_host.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/browser/utility_process_host_impl.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/public/browser/browser_child_process_host.h
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/public/common/content_switches.cc
[modify] https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f/content/public/common/content_switches.h

Project Member Comment 18 by bugdroid1@chromium.org, Nov 2 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/879e4e0e450d21fc6eb22dde4456362cc0fc335d

commit 879e4e0e450d21fc6eb22dde4456362cc0fc335d
Author: creis <creis@chromium.org>
Date: Wed Nov 02 23:05:50 2016

Revert of Enable shared memory for field trials (patchset #2 id:20001 of https://codereview.chromium.org/2469503003/ )

Reason for revert:
Appears to have caused renderer processes to lose knowledge of Finch trials, which led to a large spike in CreateFrame crashes for OOPIFs in https://crbug.com/661617.

Original issue's description:
> Enable shared memory for field trials
>
> This turns on the flag for using shared memory to share field trials between processes. For more context, see:
>
> The design document: go/share-field-trials
>
> The related previous CLs:
> https://codereview.chromium.org/2365273004/
> https://codereview.chromium.org/2412113002/
> https://codereview.chromium.org/2449143002/
> https://codereview.chromium.org/2453093002/
> https://codereview.chromium.org/2449783007/
> https://codereview.chromium.org/2456723004/
> https://codereview.chromium.org/2453933004/
>
> BUG= 131632 
>
> Committed: https://crrev.com/4446189e8148980b9a245cb1157e8a695bac35d1
> Cr-Commit-Position: refs/heads/master@{#429042}

TBR=asvitkine@chromium.org,lawrencewu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 131632 

Review-Url: https://codereview.chromium.org/2471493004
Cr-Commit-Position: refs/heads/master@{#429440}

[modify] https://crrev.com/879e4e0e450d21fc6eb22dde4456362cc0fc335d/base/metrics/field_trial.cc

Blocking: 663918
Status: Fixed
^ That CL has landed a week ago -- I think we can finally close this.
Labels: VerifyIn-58
Labels: VerifyIn-59
Labels: VerifyIn-60
Sign in to add a comment