New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 783943 link

Starred by 1 user
Status: Fixed
Owner:
csharrison@chromium.org
Closed: Mar 2018
Cc:
isherman@chromium.org
skare@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Improve force-fieldtrial-param parsing error messages for unescaped control chars

Project Member Reported by csharrison@chromium.org, Nov 10 2017 
The subresource_filter component has a few "list" style params that are comma separated.

Here's an example of a command line I want to use to launch Chrome:
chrome --force-fieldtrials=SF/Enabled --force-fieldtrial-params=SF.Enabled:enable_presets/liverun_on_better_ads_violating_sites,liverun_on_phishing_sites --enable-features="SubresourceFilter<SF"

However, this crashes due to the comma:
ERROR:field_trial_util.cc(101)] Experiment and params should be separated by ':'
FATAL:variations_field_trial_creator.cc(366)] Check failed: result. Invalid --force-fieldtrial-params list specified.

We had unit tests for this but they used base::AssociateFieldTrialParams which skips this step.


Looking at AssociateParamsFromString, it looks like the problem is  that a comma already being used by the format:
"Trial1.Group1:k1/v1/k2/v2,Trial2.Group2:k1/v1/k2/v2"

So introducing the comma in the param value makes parsing ambiguous.

An ideal solution for me would be to have a way to encode the comma without updating existing experiments which use this parameter.

Comment 1 by asvitk...@chromium.org, Nov 10 2017

Owner: csharrison@chromium.org
We already support escaping for these and that's what the code that processes field trial testing config uses.

See here for how escaping is done:

https://cs.chromium.org/chromium/src/tools/variations/fieldtrial_util.py?rcl=84a6e8e3bcf56988a496cc9e286fd55800fc5576&l=21

Let me know if that works for you. (If so, we should probably document it better if we don't already.)

Comment 2 by csharrison@chromium.org, Nov 10 2017

Summary: Improve force-fieldtrial-param parsing error messages for unescaped control chars (was: force-fieldtrial-param parsing rejects params with commas)
Alexei: yes it works for me! Thanks a lot :) Yeah ideally we should say something in the message that gets logged. Maybe:

"Invalid --force-fieldtrial-params list specified. Make sure you % encode the following characters :,/."

WDYT?
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4259eddc975104e6bf88ddc29d98e76f582d8f49

commit 4259eddc975104e6bf88ddc29d98e76f582d8f49
Author: Charles Harrison <csharrison@chromium.org>
Date: Mon Mar 12 15:49:02 2018

Better error message for force-fieldtrial-params

Some field trial params implement things like csv or json within a
particular param value. If some control characters are not %-encoded,
it can lead to confusing error messages. This CL adds a hint in the
error message for an invalid --force-fieldtrial-param.

Bug:  783943 
Change-Id: I0247e6ebf6d43c39e4002760d8bcbb053ba3f49a
Reviewed-on: https://chromium-review.googlesource.com/957913
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542495}
[modify] https://crrev.com/4259eddc975104e6bf88ddc29d98e76f582d8f49/components/variations/service/variations_field_trial_creator.cc

Comment 4 by csharrison@chromium.org, Mar 12 2018

Status: Fixed (was: Untriaged)
Calling it fixed. This error would have guided me to the right solution without asking for help :)
►

Sign in to add a comment