Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928453002:
Add MarkHttpAsDangerous test and correct other MarkNonSecureAs tests (Closed)
DescriptionAdd MarkHttpAsDangerous test and correct other MarkNonSecureAs tests
In ssl_browser_tests.cc, we have various tests that CreateFieldTrial
with kMarkHttpAsDangerous. This field trial is overridden by the
fieldtrial_testing_config.json settings and therefore these calls
are ineffective. This CL overrides the fieldtrial_testing settings.
We also didn't have a test that actually tests that the setting
MarkHttpAsDangerous marks HTTP as dangerous. This CL adds such
a test.
BUG=729703
Review-Url: https://codereview.chromium.org/2928453002
Cr-Commit-Position: refs/heads/master@{#477456}
Committed: https://chromium.googlesource.com/chromium/src/+/22f6d5b3389c0b066ae841d12889373818cabfc0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review feedback #
Total comments: 6
Patch Set 3 : Change tests to use command line flag #
Messages
Total messages: 29 (9 generated)
elawrence@chromium.org changed reviewers: + estark@chromium.org
PTAL? I noticed these problems when adding tests for the upcoming HTTPBadForIncognito change.
lgtm if you delete the config from fieldtrial_testing_config.json as well. Thanks for catching and fixing this! https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:702: class SSLUITestIgnoringFieldTrialConfig : public SSLUITest { I think we should do what you've done here, but also delete the MarkNonSecureAs config from fieldtrial_testing_config.json. I say that because it's testing an old config that's no longer used (it's the default now) so we don't need it anymore and should delete it. But we will probably replace it soon with a Phase 2 config, so I think it's good to keep the changes you've made to these tests so that we don't introduce the same problem again. https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:1324: scoped_refptr<base::FieldTrial> trial = nit: maybe ASSERT_TRUE(trial) after each of these? It looks like this returns null if the field trial already exists.
Thanks for the fast review! https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:702: class SSLUITestIgnoringFieldTrialConfig : public SSLUITest { On 2017/06/05 21:07:33, estark wrote: > I think we should do what you've done here, but also delete the MarkNonSecureAs > config from fieldtrial_testing_config.json. I say that because it's testing an > old config that's no longer used (it's the default now) so we don't need it > anymore and should delete it. But we will probably replace it soon with a Phase > 2 config, so I think it's good to keep the changes you've made to these tests so > that we don't introduce the same problem again. Done.
https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:1324: scoped_refptr<base::FieldTrial> trial = On 2017/06/05 21:07:33, estark wrote: > nit: maybe ASSERT_TRUE(trial) after each of these? It looks like this returns > null if the field trial already exists. Done; though using the more explicit (x!=nullptr) to match surrounding code and the Google Test style.
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2928453002/#ps20001 (title: "Address review feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
elawrence@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in fieldtrial_testing_config.json? thanks!
isherman@chromium.org: Please review changes in fieldtrial_testing_config.json? thanks!
fieldtrial_testing_config.json lgtm
On 2017/06/05 23:15:05, Ilya Sherman wrote: > fieldtrial_testing_config.json lgtm Actually, sorry, not LGTM. Is this feature no longer being developed, or are you simply having trouble testing it correctly when the fieldtrial testing config is present? If the latter, then removing the config is not the right solution.
On 2017/06/05 23:16:24, Ilya Sherman wrote: > On 2017/06/05 23:15:05, Ilya Sherman wrote: > > fieldtrial_testing_config.json lgtm > > Actually, sorry, not LGTM. Is this feature no longer being developed, or are > you simply having trouble testing it correctly when the fieldtrial testing > config is present? If the latter, then removing the config is not the right > solution. The feature is launched to stable already and enabled by default, so we don't need the field trial config anymore.
On 2017/06/05 23:18:12, estark wrote: > On 2017/06/05 23:16:24, Ilya Sherman wrote: > > On 2017/06/05 23:15:05, Ilya Sherman wrote: > > > fieldtrial_testing_config.json lgtm > > > > Actually, sorry, not LGTM. Is this feature no longer being developed, or are > > you simply having trouble testing it correctly when the fieldtrial testing > > config is present? If the latter, then removing the config is not the right > > solution. > > The feature is launched to stable already and enabled by default, so we don't > need the field trial config anymore. I see, thanks. I have some follow-up questions inline, then: https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:709: variations::switches::kDisableFieldTrialTestingConfig); Is this still needed given that you are removing the config? https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:1326: "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous); If the code is launched to stable, then why do you need to test different field trial states?
https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:709: variations::switches::kDisableFieldTrialTestingConfig); On 2017/06/05 23:21:12, Ilya Sherman wrote: > Is this still needed given that you are removing the config? An important caveat for this CL is that we will very shortly (e.g. this week) be adding new states to mark-non-secure-as/MarkNonsecureAs as a part of our HTTPBad Phase 2 work. Soon after we introduce the new states, we will likely reintroduce MarkNonsecureAs override in the fieldtrial_testing_config.json file with a new "default" setting that we expect will eventually be the M61 shipping default value. As a consequence, yes, we should have this call here to ensure that tests based on this fixture work as intended and do not vary based on the JSON. https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:1326: "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous); On 2017/06/05 23:21:12, Ilya Sherman wrote: > If the code is launched to stable, then why do you need to test different field > trial states? Marking of HTTP is controlled by chrome://flags#mark-non-secure-as and a matching field trial named MarkNonsecureAs. In Chrome versions prior to 56, this setting had as many as four states. In Chrome 60, it now has two states (Default, MarkHttpAsDangerous). The existing tests use the field trial group name to exercise the different codepaths that respect the states. The fieldtrial_testing_config.json file currently reiterates the default state of the feature. The fieldtrial_testing_config file overrides any settings set in the test, so reiterating the default state in the JSON makes the CreateFieldTrial calls ineffective. To fix that, in the first version of this CL, I set the command line flag for the relevant test fixtures to ignore the JSON. Emily pointed out that we should also remove the redundant declaration in the JSON anyway (as it has no real purpose at this time).
https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:709: variations::switches::kDisableFieldTrialTestingConfig); On 2017/06/06 01:05:17, elawrence wrote: > On 2017/06/05 23:21:12, Ilya Sherman wrote: > > Is this still needed given that you are removing the config? > > An important caveat for this CL is that we will very shortly (e.g. this week) be > adding new states to mark-non-secure-as/MarkNonsecureAs as a part of our HTTPBad > Phase 2 work. Soon after we introduce the new states, we will likely reintroduce > MarkNonsecureAs override in the fieldtrial_testing_config.json file with a new > "default" setting that we expect will eventually be the M61 shipping default > value. As a consequence, yes, we should have this call here to ensure that tests > based on this fixture work as intended and do not vary based on the JSON. I see. I'm not very thrilled about this choice, as it basically means that these tests are opting out of coverage by *all other field trials*. That means that if there's any interaction between those trials and the code under test here, it won't be caught by the bots. Could you instead register the appropriate field trial during SetUp()?
https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2928453002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:709: variations::switches::kDisableFieldTrialTestingConfig); > these tests are opting out of coverage by *all other field trials*. That means > that if there's any interaction between those trials and the code under test > here, it won't be caught by the bots. Yeah, that's not great. > Could you instead register the appropriate field trial during SetUp()? Unfortunately, that doesn't appear to work, as the Field Trials system isn't initialized at that point and calling CreateFieldTrial() fails the DHECK(global_) test at the start of the function. I suppose we can change the tests so that they don't use field trials at all (instead using the command-line flag to control the feature).
Patchset #3 (id:40001) has been deleted
I've switched over to using the command line flags to control the feature. Please have another look?
On 2017/06/06 21:48:07, elawrence wrote: > I've switched over to using the command line flags to control the feature. > Please have another look? Thanks. This LGTM so as to not block you -- though I'm still investigating whether there's a better way to configure field trials that are also controlled by the testing config. And, as Alexei mentioned, you might want to use the FeatureList API for trials going forward.
Still lgtm. This field trial predates the Feature API but agree that we should switch over at some point, that would make life a lot easier.
The CQ bit was checked by elawrence@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496787212170420,
"parent_rev": "7200acac17a321b76b0f410f1833b7ce6daccfd8", "commit_rev":
"22f6d5b3389c0b066ae841d12889373818cabfc0"}
Message was sent while issue was closed.
Description was changed from ========== Add MarkHttpAsDangerous test and correct other MarkNonSecureAs tests In ssl_browser_tests.cc, we have various tests that CreateFieldTrial with kMarkHttpAsDangerous. This field trial is overridden by the fieldtrial_testing_config.json settings and therefore these calls are ineffective. This CL overrides the fieldtrial_testing settings. We also didn't have a test that actually tests that the setting MarkHttpAsDangerous marks HTTP as dangerous. This CL adds such a test. BUG=729703 ========== to ========== Add MarkHttpAsDangerous test and correct other MarkNonSecureAs tests In ssl_browser_tests.cc, we have various tests that CreateFieldTrial with kMarkHttpAsDangerous. This field trial is overridden by the fieldtrial_testing_config.json settings and therefore these calls are ineffective. This CL overrides the fieldtrial_testing settings. We also didn't have a test that actually tests that the setting MarkHttpAsDangerous marks HTTP as dangerous. This CL adds such a test. BUG=729703 Review-Url: https://codereview.chromium.org/2928453002 Cr-Commit-Position: refs/heads/master@{#477456} Committed: https://chromium.googlesource.com/chromium/src/+/22f6d5b3389c0b066ae841d12889... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/22f6d5b3389c0b066ae841d12889... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
