Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.

content/public/test/test_utils.cc

Index: content/public/test/test_utils.cc
diff --git a/content/public/test/test_utils.cc b/content/public/test/test_utils.cc
index a50cc6cda21fed1742c92b5c79c1137dfa4b24fb..f43cce32517ecd76131caeab1cc5d81dfc5b6653 100644
--- a/content/public/test/test_utils.cc
+++ b/content/public/test/test_utils.cc
@@ -4,8 +4,10 @@
 
 #include "content/public/test/test_utils.h"
 
+#include <string>
 #include <utility>
 
+#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
@@ -19,6 +21,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/variations/variations_switches.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/url_schemes.h"
 #include "content/public/browser/browser_child_process_host_iterator.h"
@@ -26,6 +29,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "content/public/test/test_launcher.h"
@@ -206,6 +210,31 @@ void IsolateAllSitesForTesting(base::CommandLine* command_line) {
   command_line->AppendSwitch(switches::kSitePerProcess);
 }
 
+void EnableTopDocumentIsolationForTesting(base::CommandLine* command_line) {

[Alexei Svitkine (slow), 2017/07/07 16:55:06]

Can you use VariationsParamManager here instead?

https://cs.chromium.org/chromium/src/components/variations/variations_params_...

This way, this code doesn't need to make assumptions about how the command line
flags work and how to craft them...

[Łukasz Anforowicz, 2017/07/07 20:41:42]

Thank you very much for this suggestion - this looks very nice.  I've tried
doing this in patchset #12 and #13.  It seems to work fine for
content_browsertests.

Unfortunately, things blow-up in //chrome-layer browser_tests:

1. If I construct variations::testing::VariationParamsManager
   in SubframeTaskTDIBrowserTest::SetUpOnMainThread (in exactly
   the same place where I used to call
   scoped_feature_list_.InitAndEnableFeature), then the following
   DCHECK fires:

[104947:104947:0707/131927.306918:FATAL:field_trial.cc(502)] Check failed:
!global_. 
    #0 0x7fc1bb2fc557 base::debug::StackTrace::StackTrace()
    #1 0x7fc1bb323881 logging::LogMessage::~LogMessage()
    #2 0x7fc1bb3365f7 base::FieldTrialList::FieldTrialList()
    #3 0x000002ea5f4c
variations::testing::VariationParamsManager::VariationParamsManager()
    ...
    #5 0x000001dcc9bf content::EnableTopDocumentIsolationForTesting()
    #6 0x000000c20ed2
task_manager::SubframeTaskTDIBrowserTest::SetUpOnMainThread()
    #7 0x000001d9455c content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
    ...
    #18 0x7fc1b7e808d2 content::ContentMain()
    #19 0x000001d94207 content::BrowserTestBase::SetUp()
    #20 0x0000017d7cde InProcessBrowserTest::SetUp()

2. If I construct variations::testing::VariationParamsManager
   in the constructor of SubframeTaskTDIBrowserTest, then
   the following DCHECK fires:

    [105833:105833:0707/132144.129356:FATAL:field_trial.cc(502)] Check failed:
!global_. 
    #0 0x7f627574d557 base::debug::StackTrace::StackTrace()
    #1 0x7f6275774881 logging::LogMessage::~LogMessage()
    #2 0x7f62757875f7 base::FieldTrialList::FieldTrialList()
    #3 0x000001bad61b ChromeBrowserMainParts::SetupFieldTrials()
    #4 0x000001bb02e8 ChromeBrowserMainParts::PreCreateThreadsImpl()
    #5 0x000001baee8a ChromeBrowserMainParts::PreCreateThreads()
    #6 0x7f6271a61f49 content::BrowserMainLoop::PreCreateThreads()
    #7 0x7f6271e95c77 content::StartupTaskRunner::RunAllTasksNow()
    #8 0x7f6271a625bf content::BrowserMainLoop::CreateStartupTasks()
    #9 0x7f6271a6733e content::BrowserMainRunnerImpl::Initialize()
    #10 0x7f6271a5fa62 content::BrowserMain()
    #11 0x7f62722d24bc content::RunNamedProcessTypeMain()
    #12 0x7f62722d303d content::ContentMainRunnerImpl::Run()
    #13 0x7f62705923e1 service_manager::Main()
    #14 0x7f62722d18d2 content::ContentMain()
    #15 0x000001d941b7 content::BrowserTestBase::SetUp()
    #16 0x0000017d7c8e InProcessBrowserTest::SetUp()

Given above, do you think I should revert test_utils.cc to patchset #11?  FWIW,
I see that the command-line-based approach is used in other browser tests, so
maybe this is good-enough for landing? :-)  See the examples found by 

-
https://cs.chromium.org/search/?q=kForceFieldTrialParams+file:chrome+file:bro...

and by

-
https://cs.chromium.org/search/?q=%22force-fieldtrial-params%22+lang:cpp&type=cs

[Alexei Svitkine (slow), 2017/07/10 18:46:24]

The above check indicates there's already a FieldTrialList when the browser init
code runs. I guess outside of Chrome, there is no such initialization - so we
don't run into this check.

I guess I'm not against the command-line approach in general - especially since
it's used elsewhere - but maybe we can abstract it behind a helper class? Maybe
we can add a bool to VariationsParamManager called .set_use_command_line_flags()
that will then make it do this command-line thing? Then, the API is still the
same and the implementation details of the flags are constrained to be within
metrics code, rather than each test duplicating it.

[Łukasz Anforowicz, 2017/07/10 22:17:39]

I've opened https://crbug.com/740701 to track creating a shared helper.

When working on a CL that would address this, I've run into a problem with an
undesirable dependency (see https://crbug.com/740701#c2), so I hope I can land
the current CL as-is (and that we can independently figure how to move
https://crbug.com/740701 forward).

FWIW, I've added a 2 TODOs to the current CL that reference the other bug (in
content/DEPS and in here - above EnableTopDocumentIsolationForTesting).
I think this is not possible (i.e. I think we need a new, separate API), because
the usage pattern for cmdline-setting-mode needs to be different.  See also
https://crbug.com/740701#c1

+  // Enable top-document-isolation feature (features::kTopDocumentIsolation)
+  // and associate it with a fake "TopDocumentIsolation" trial.
+  command_line->AppendSwitchASCII(
+      switches::kEnableFeatures,
+      std::string(features::kTopDocumentIsolation.name) +
+          "<TopDocumentIsolation");
+
+  // Register a fake TopDocumentIsolation/BrowserTests study group.
+  command_line->AppendSwitchASCII(switches::kForceFieldTrials,
+                                  "TopDocumentIsolation/BrowserTests");
+
+  // Set kTopDocumentIsolationModeParam to TopDocumentIsolationMode::CrossSite.
+  // This is not really needed for content_browsertests (which will go through
+  // ShellContentBrowserClient::ShouldIsolateFrameFromMainContent), but is
+  // needed for browser_tests and other tests that use
+  // ChromeContentBrowserClient::ShouldIsolateFrameFromMainContent.
+  command_line->AppendSwitchASCII(
+      variations::switches::kForceFieldTrialParams,
+      "TopDocumentIsolation.BrowserTests:" +
+          std::string(features::kTopDocumentIsolationModeParam) + "/" +
+          std::to_string(
+              static_cast<int>(features::TopDocumentIsolationMode::CrossSite)));
+}
+
 void ResetSchemesAndOriginsWhitelist() {
   url::Shutdown();
   RegisterContentSchemes(false);