Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946113002:
Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. (Closed)
|
Created:
3 years, 6 months ago by Łukasz Anforowicz Modified:
3 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.
FrameIsAd heuristics
====================
After this CL, TopDocumentIsolation will by default only isolate
cross-site frames that also match FrameIsAd heuristics. This behavior
is controllable via Finch, via chrome://flags and via command line
(see below for details).
Impact on browser tests
=======================
Browser tests should not depend on the FrameIsAd heuristics. This is
taken care of by EnableTopDocumentIsolationForTesting, which enables
the mode that isolates all frames that are cross-site from the main
frame.
chrome://flags changes
======================
This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the
following variations are present (3. and 4. are introduced by this CL):
1. Default
2. Enabled
3. Enabled (CrossSite - isolate all frames from sites other than
the top-level frame)
4. Enabled (Ads - isolate only cross-site ads detected by heuristics)
5. Disabled
Today variations 1 and 5 turn off TDI mode (because the default for
kTopDocumentIsolation base::Feature is to be disabled). Variation 3
enables TDI and isolates all cross-site frames (this is what browser
tests use when enabling TDI via EnableTopDocumentIsolationForTesting).
Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a
better name for variation 2 would be "Enabled (unspecified isolation
mode)", but the "Enabled" string is hardcoded in the chrome://flags
code.
I've manually tested that if a user enabled TDI in chrome://flags before
this CL, then it will stay enabled after this CL (variation 2).
Command line
============
After this CL, the user can control TDI with the following command line
switches:
- When no extra cmdline switches are present, then TDI is controlled via
chrome://flags and/or Finch experiments.
- Cmdline flags to disable TDI:
--disable-features=top-document-isolation
- Cmdline flags to enable TDI using unspecified / default isolation mode:
--enable-features=top-document-isolation
- Cmdline flags to enable TDI using specific isolation mode (1 for
cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all
possible values):
--enable-features="top-document-isolation<TopDocumentIsolation" \
--force-fieldtrials=TopDocumentIsolation/Cmdline \
--force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1
BUG=733303
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2946113002
Cr-Commit-Position: refs/heads/master@{#488372}
Committed: https://chromium.googlesource.com/chromium/src/+/c1dd61f01ced0c94ccc2efe373a93c696bb98c2f
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Make SubframeTaskTDIBrowserTest test suite pass (overriding ChromeContentBrowserClient). #Patch Set 4 : Backpedal and use trial params via GetFieldTrialParamByFeature... #Patch Set 5 : Rebasing... #
Total comments: 15
Patch Set 6 : Use FOR_EACH_TDI_MODE(V) macro. #
Total comments: 25
Patch Set 7 : Addressing CR feedback from creis@. #
Total comments: 3
Patch Set 8 : Consolidate //content/.../DEPS for components/variations into //content/DEPS. #Patch Set 9 : Rebasing and git-cl-format #
Total comments: 6
Patch Set 10 : Tweaks in content_features.h: tweaking comments, enum value names, mode descriptions. #Patch Set 11 : Doh... need to also change s/Default/Unspecified/ in chrome_content_browser_client.cc #
Total comments: 18
Patch Set 12 : Addressed CR feedback from asvitkine@. #Patch Set 13 : #include tweaks AND moving //components/variations:test_support to public_deps. #Patch Set 14 : Rebasing (which also helps here with content/browser/renderer_host/DEPS). #Patch Set 15 : Going back to cmdline-based approach for tests. #Patch Set 16 : Removing unnecessary includes. #Patch Set 17 : Fixing //content/DEPS... (and an accidental rebase along the way... :-o). #Patch Set 18 : Rebasing on top of r486814 #
Total comments: 1
Patch Set 19 : Moving to base::SupportsUserData associated with content::NavigationHandle #Patch Set 20 : Fixing build and deduplicating AdType enum in unittests. #Patch Set 21 : Undo accidental removal of components/task_scheduler_util/browser/DEPS #
Total comments: 12
Patch Set 22 : Fixing unit test failures. #Patch Set 23 : Addressing CR feedback from jkarlin@ and csharrison@ (and also small self-review tweaks.) #
Total comments: 5
Patch Set 24 : Addressing 2 more CR comments from csharrison@. #
Total comments: 16
Patch Set 25 : Addressing CR feedback from jkarlin@. #Patch Set 26 : Rebasing... #
Total comments: 10
Patch Set 27 : Remove the late-enough DCHECK #
Total comments: 2
Patch Set 28 : Addressing CR feedback from jkarlin@ and creis@. #Messages
Total messages: 164 (120 generated)
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. BUG=733303 ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: 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...)
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::. TODO / DO NOT SUBMIT - need to verify. browser_tests need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present: 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::. TODO / DO NOT SUBMIT - need to verify. browser_tests need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present: 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::. TODO / DO NOT SUBMIT - need to verify. browser_tests need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present: 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - no cmdline switches - TDI controlled via chrome://flags and/or Finch - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" --force-fieldtrials=TopDocumentIsolation/Cmdline --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::. TODO / DO NOT SUBMIT - need to verify. browser_tests need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present: 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - no cmdline switches - TDI controlled via chrome://flags and/or Finch - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" --force-fieldtrials=TopDocumentIsolation/Cmdline --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation/Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for ads, 2 for xsite; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch and via chrome://flags (see below). Convenient website for testing the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is automatically true for content_browsertests (because of ShellContentBrowserClient::ShouldIsolateFrameFromMainContent). browser_tests (or other tests exercising ChromeContentBrowserClient) need to explicitly opt into xsite isolation mode (see the changes in TODO / DO NOT SUBMIT for an example). chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lukasza@chromium.org changed reviewers: + creis@chromium.org
creis@, could you PTAL? https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1082: '0' + static_cast<int>(features::TopDocumentIsolationMode::Ads), '\0'}; My opinion is that the code above is better than 1) hardcoding "1" (or was it "2") here and/or 2) introducing ugly C++ templates (e.g. see https://repl.it/JJAt/0). FWIW, I am asking for a second opinion at cxx@chromium.org DL (https://groups.google.com/a/chromium.org/d/topic/cxx/W4pyzIQ1ksc/discussion). https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1097: {"(isolate all frames from site other than the top-level frame)", The string above is concatenated with "Enabled " string. See the CL description for the 5 variations/options that this CL makes available for TopDocumentIsolation's entry in chrome://flags. https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h (right): https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:10: #include <vector> IWYU fixes (and other "out of blue" changes, like adding "explicit" keyword here and there) have been pointed out by "git cl lint". https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/ads_detection.h:15: bool IsAdFrame(base::StringPiece frame_name, const GURL& frame_url); This function used to be hidden inside an anonymous namespace inside chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc. https://codereview.chromium.org/2946113002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.cc (left): https://codereview.chromium.org/2946113002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.cc:98: return false; I am not sure what the default should be here. It doesn't matter that much for browser tests and chrome, only for other embedders. I think |false| is okay. https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... content/public/common/content_features.h:80: Default = 0, Hmmm... maybe this should be spelled kDefault... https://google.github.io/styleguide/cppguide.html#Enumerator_Names https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... content/public/common/content_features.h:87: }; I don't really know what is the best place for declaring/defining 1) the kTopDocumentIsolationModeParam string/const char* and 2) the TopDocumentIsolationMode enum. I think having them in content_features.h is okay (although maybe I should move them to the end of this file). One alternative would be to introduce content/public/browser/top_document_isolation_feature.h and move 1) kTopDocumentIsolation base::Feature, 2) kTopDocumentIsolationModeParam, 3) TopDocumentIsolationMode enum, 4) EnableTopDocumentIsolationForTesting (and maybe even 5. IsTopDocumentIsolationEnabled) into this new compilation unit - this is done in a few other places in Chromium like: - chrome/browser/prerender/prerender_field_trial.h - chrome/browser/experiments/memory_ablation_experiment.h (the only file in this directory :-P) - components/omnibox/browser/omnibox_field_trial.h https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEP... content/public/test/DEPS:10: "+components/variations/variations_switches.h", Needed in this CL by EnableTopDocumentIsolationForTesting from content/public/test/test_utils.cc. I think this dependency is okay for *test* code. https://codereview.chromium.org/2946113002/diff/80001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2946113002/diff/80001/content/test/BUILD.gn#n... content/test/BUILD.gn:253: "//components/variations", Needed in this CL by EnableTopDocumentIsolationForTesting from content/public/test/test_utils.cc.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie, my appologies for last minute churn - in the last patchset: 1. I've started using FOR_EACH_TDI_MODE macro as suggested on cxx@chromium.org. 2. I am hardcoding less strings. https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... content/public/test/test_utils.cc:223: "TopDocumentIsolation/BrowserTests"); I think it is okay to hardcode "TopDocumentIsolation" and "BrowserTests" strings, because they are local to this function. I *think* it is okay if we use a string other than "TopDocumentIsolation" 1) here, 2) in about_flags.cc (as the last argument of the FEATURE_WITH_PARAMS_VALUE_TYPE macro), and 3) in Finch configs (in google3 repo). OTOH, I am open to declaring and using a kTopDocumentIsolationTrialName = "TopDocumentIsolation" constant. https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... content/public/test/test_utils.cc:235: static_cast<int>(features::TopDocumentIsolationMode::Xsite))); In the latest patchset, I am not hardcoding a "1" above.
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (isolate ads detected by heuristics) 4. Enabled (isolate all frames from site other than the top-level frame) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 4 enables TDI and isolates all xsite frames. Variations 2 and 3 enable TDI and use FrameIsAd heuristics; probably a better name for variation 2 would be "Enabled (default isolation)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (Xsite - isolate all frames from sites other than the top-level frame) 4. Enabled (Ads - isolate ads detected by heuristics) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 3 enables TDI and isolates all xsite frames (this is what browser tests use when enabling TDI via EnableTopDocumentIsolationForTesting). Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a better name for variation 2 would be "Enabled (default isolation mode)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! This is just the sort of thing I was hoping we could do. A few thoughts below, but there's a couple places we'll probably want to consult with other experts. https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2946113002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1082: '0' + static_cast<int>(features::TopDocumentIsolationMode::Ads), '\0'}; On 2017/06/30 15:28:36, Łukasz A. wrote: > My opinion is that the code above is better than 1) hardcoding "1" (or was it > "2") here and/or 2) introducing ugly C++ templates (e.g. see > https://repl.it/JJAt/0). FWIW, I am asking for a second opinion at > mailto:cxx@chromium.org DL > (https://groups.google.com/a/chromium.org/d/topic/cxx/W4pyzIQ1ksc/discussion). Looks like you got this resolved. https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEP... content/public/test/DEPS:10: "+components/variations/variations_switches.h", On 2017/06/30 15:28:37, Łukasz A. wrote: > Needed in this CL by EnableTopDocumentIsolationForTesting from > content/public/test/test_utils.cc. I think this dependency is okay for *test* > code. I don't have a good sense. We should double check with jam@ on this. https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1364: case features::TopDocumentIsolationMode::Default: I thought "Default" meant disabled? Shouldn't we return false here? https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.cc:16: if (base::StartsWith(frame_name, "google_ads_iframe", Sanity check: Any page can trigger TDI by naming its cross-site frame "google_ads_iframe"? :) I suppose false positives aren't a big deal? https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (GetContentClient()->browser()->ShouldIsolateFrameFromMainContent( Hmm. It feels a little odd to have the embedder make the ultimate decision about using TDI even thought content/ knows about IsTopDocumentIsolationEnabled. I suppose that's why it was phrased as an override before. I guess I see the logic, though, since the ad-specific mode is easier to express that way than as an override for the rest of cross-site iframes. I'm ok with it. https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.cc:98: return false; Shouldn't this be true? Otherwise it seems like embedders will get a non-functioning TDI mode by default. https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.h:262: // to |url| and the main content is in the |main_frame_site_instance|). Let's clarify that (1) the dest_url is already assumed to be cross-site (right?), and (2) the default should be to return true unless the embedder needs to override TopDocumentIsolation in some cases. https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.h:263: virtual bool ShouldIsolateFrameFromMainContent( I'm finding it confusing that the name doesn't reference either cross-site frames or TDI, which makes it sound like it's called much more generally. If you want to stick with the inversion from the previous behavior, maybe we could call it ShouldIsolateFrameForTopDocumentIsolation and change dest_url to cross_site_dest_url? (I also thought about ShouldIsolateCrossSiteFrameFromMainContent, but couldn't work TDI into that very well.) https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:80: V(Xsite, 1, "isolate all frames from sites other than the top-level frame") \ We don't really use the term Xsite anywhere in Chrome. Would it be a problem to say CrossSite (here and in CL description)? https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:84: Default = 0, Sorry, I'm getting confused about the difference between these names and values and those in the CL description (which had 5 different choices). How do we end up with 5 values in about:flags, and how do we map between them? Maybe that's worth explaining in a comment? https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... content/public/test/test_utils.cc:223: "TopDocumentIsolation/BrowserTests"); On 2017/06/30 16:50:20, Łukasz A. wrote: > I think it is okay to hardcode "TopDocumentIsolation" and "BrowserTests" > strings, because they are local to this function. > > I *think* it is okay if we use a string other than "TopDocumentIsolation" 1) > here, 2) in about_flags.cc (as the last argument of the > FEATURE_WITH_PARAMS_VALUE_TYPE macro), and 3) in Finch configs (in google3 > repo). > > OTOH, I am open to declaring and using a kTopDocumentIsolationTrialName = > "TopDocumentIsolation" constant. I think this may be more of a question for the Features folks-- I'm not sure what the right practice is here.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback Charlie. Can you take another look please? https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEP... content/public/test/DEPS:10: "+components/variations/variations_switches.h", On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > On 2017/06/30 15:28:37, Łukasz A. wrote: > > Needed in this CL by EnableTopDocumentIsolationForTesting from > > content/public/test/test_utils.cc. I think this dependency is okay for *test* > > code. > > I don't have a good sense. We should double check with jam@ on this. Ok. I'll make a note for this. https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1364: case features::TopDocumentIsolationMode::Default: On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > I thought "Default" meant disabled? Shouldn't we return false here? TDI state has 2 parts: part1: is the feature enabled / disabled / in the default enabled-or-disabled state part2: what mode do we use for selecting frames that go into the tdi process: xsite / ads / default (= ads). In the CL description I've listed 5 states that TDI exposes in chrome://flags after this CL. Let me repeat these 5 state below, annotating them with part1 and part2 values. 1. "Default" (part1 = disabled, part2 = n/a) 2. "Enabled" (part1 = enabled, part2 = default) 3. "Enabled (Xsite - ...)" (part1 = enabled, part2 = xsite) 4. "Enabled (Ads - ...)" (part1 = enabled, part2 = ads) 5. Disabled (part1 = disabled, part2 = n/a) The "Default" in the code above comes from TopDocumentIsolation*Mode* enum and corresponds to part2 - it is the "default mode" (only consulted when TDI is enabled), but not the "default disabled-vs-enabled state". Does the explanation above help? Is there something I should do in the code to make it easier to understand? https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.cc:16: if (base::StartsWith(frame_name, "google_ads_iframe", On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > Sanity check: Any page can trigger TDI by naming its cross-site frame > "google_ads_iframe"? :) Correct. Thanks for raising this angle - I honestly haven't thought about this. > I suppose false positives aren't a big deal? I think that yes, false positives aren't a big deal. https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (GetContentClient()->browser()->ShouldIsolateFrameFromMainContent( On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > Hmm. It feels a little odd to have the embedder make the ultimate decision > about using TDI even thought content/ knows about IsTopDocumentIsolationEnabled. > I suppose that's why it was phrased as an override before. > > I guess I see the logic, though, since the ad-specific mode is easier to express > that way than as an override for the rest of cross-site iframes. I'm ok with > it. Do you think that maybe the base::Feature for TopDocumentIsolation should also move to the //chrome layer? This is doable, although we would have to change how //content-layer browser tests opt into TDI (they would have to override ContentBrowserClient rather than tweak command line flags). https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.cc:98: return false; On 2017/06/30 23:28:46, Charlie Reis (OOO til July 5) wrote: > Shouldn't this be true? Otherwise it seems like embedders will get a > non-functioning TDI mode by default. Makes sense - I wasn't quite sure what should be returned here. Done. https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.h:262: // to |url| and the main content is in the |main_frame_site_instance|). On 2017/06/30 23:28:47, Charlie Reis (OOO til July 5) wrote: > Let's clarify that > (1) the dest_url is already assumed to be cross-site (right?) Correct. I've added this information to the comment. > and > (2) the default should be to return true unless the embedder needs > to override TopDocumentIsolation in some cases. I am not sure about this part. Returning false for non-ads seems pretty natural. I think it is okay to leave the decision entirely up to the embedder (without guiding toward selecting any specific set of frames to isolate). https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.h:263: virtual bool ShouldIsolateFrameFromMainContent( On 2017/06/30 23:28:48, Charlie Reis (OOO til July 5) wrote: > I'm finding it confusing that the name doesn't reference either cross-site > frames or TDI, which makes it sound like it's called much more generally. > > If you want to stick with the inversion from the previous behavior, maybe we > could call it ShouldIsolateFrameForTopDocumentIsolation and change dest_url to > cross_site_dest_url? > > (I also thought about ShouldIsolateCrossSiteFrameFromMainContent, but couldn't > work TDI into that very well.) Good point. Done. (and thanks for suggesting the name) https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:80: V(Xsite, 1, "isolate all frames from sites other than the top-level frame") \ On 2017/06/30 23:28:49, Charlie Reis (OOO til July 5) wrote: > We don't really use the term Xsite anywhere in Chrome. Would it be a problem to > say CrossSite (here and in CL description)? Thanks for catching "Xsite". I've changed this to "CrossSite". I think I still want to keep the mode description above though. I didn't want to say that this mode isolates all cross-site frames, because this sounds very much like --site-per-process and adding the "from top-level frame" qualifier might not help much (i.e. if top-level is from a.com, then cross-site subframes from b.com and c.com would both be put into the same TDI process). WDYT? Would you like to see a different description ("isolate all cross-site frames from the top-level frame"?)? https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:84: Default = 0, On 2017/06/30 23:28:48, Charlie Reis (OOO til July 5) wrote: > Sorry, I'm getting confused about the difference between these names and values > and those in the CL description (which had 5 different choices). How do we end > up with 5 values in about:flags, and how do we map between them? Maybe that's > worth explaining in a comment? I've added a comment to the code above. chrome://flags gives 3 entries for free: - Default (whether the feature is disabled or enabled - this depends on what we say in base::Feature definition for kTopDocumentIsolation) - Disabled - Enabled (this corresponds to "Mode::Default" enum value above) we explicitly declare 2 more entries in about_flags.cc - one for each of the modes covered by FOR_EACH_TDI_MODE - they correspond to "Enabled (<name> - <description>)" entries in chrome://flags. https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/test/te... content/public/test/test_utils.cc:223: "TopDocumentIsolation/BrowserTests"); On 2017/06/30 23:28:49, Charlie Reis (OOO til July 5) wrote: > On 2017/06/30 16:50:20, Łukasz A. wrote: > > I think it is okay to hardcode "TopDocumentIsolation" and "BrowserTests" > > strings, because they are local to this function. > > > > I *think* it is okay if we use a string other than "TopDocumentIsolation" 1) > > here, 2) in about_flags.cc (as the last argument of the > > FEATURE_WITH_PARAMS_VALUE_TYPE macro), and 3) in Finch configs (in google3 > > repo). > > > > OTOH, I am open to declaring and using a kTopDocumentIsolationTrialName = > > "TopDocumentIsolation" constant. > > I think this may be more of a question for the Features folks-- I'm not sure > what the right practice is here. Ok. https://codereview.chromium.org/2946113002/diff/120001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/120001/content/public/common/... content/public/common/content_features.h:98: Charlie - do you have any thoughts on the location of the declarations (kTopDocumentIsolationModeParam const and TopDocumentIsolationMode enum) above? Are they okay here? Would you move them to another header? Some of my earlier thoughts are in https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c...
lukasza@chromium.org changed reviewers: + jam@chromium.org
jam@ - could you PTAL (as a //content OWNER) at the dependency changes being introduced by this CL? In particular: - content/public/*test*/DEPS - content/*test*/BUILD.gn https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/test/DEP... content/public/test/DEPS:10: "+components/variations/variations_switches.h", On 2017/07/01 00:10:53, Łukasz A. wrote: > On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > > On 2017/06/30 15:28:37, Łukasz A. wrote: > > > Needed in this CL by EnableTopDocumentIsolationForTesting from > > > content/public/test/test_utils.cc. I think this dependency is okay for > *test* > > > code. > > > > I don't have a good sense. We should double check with jam@ on this. > > Ok. I'll make a note for this. jam@ - could you PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2946113002/diff/120001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/120001/content/public/test/DE... content/public/test/DEPS:10: "+components/variations/variations_switches.h", this is already used in content, see https://cs.chromium.org/search/?q=file:content+file:deps+components/variation... can you just move this to content/DEPS and remove the other entries in content/../deps?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
https://codereview.chromium.org/2946113002/diff/120001/content/public/test/DEPS File content/public/test/DEPS (right): https://codereview.chromium.org/2946113002/diff/120001/content/public/test/DE... content/public/test/DEPS:10: "+components/variations/variations_switches.h", On 2017/07/05 15:06:13, jam wrote: > this is already used in content, see > https://cs.chromium.org/search/?q=file:content+file:deps+components/variation... > > can you just move this to content/DEPS and remove the other entries in > content/../deps? Ugh - good point, I should have checked and noticed that this dependency is already present elsewhere in //content. Done.
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Thanks! LGTM with nits if we can rename one of the "Default" values. Also, don't forget to update the CL description. https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1364: case features::TopDocumentIsolationMode::Default: On 2017/07/01 00:10:53, Łukasz A. wrote: > On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > > I thought "Default" meant disabled? Shouldn't we return false here? > > TDI state has 2 parts: > part1: is the feature enabled / disabled / in the default enabled-or-disabled > state > part2: what mode do we use for selecting frames that go into the tdi process: > xsite / ads / default (= ads). > > In the CL description I've listed 5 states that TDI exposes in chrome://flags > after this CL. Let me repeat these 5 state below, annotating them with part1 > and part2 values. > > 1. "Default" (part1 = disabled, part2 = n/a) > 2. "Enabled" (part1 = enabled, part2 = default) > 3. "Enabled (Xsite - ...)" (part1 = enabled, part2 = xsite) > 4. "Enabled (Ads - ...)" (part1 = enabled, part2 = ads) > 5. Disabled (part1 = disabled, part2 = n/a) > > The "Default" in the code above comes from TopDocumentIsolation*Mode* enum and > corresponds to part2 - it is the "default mode" (only consulted when TDI is > enabled), but not the "default disabled-vs-enabled state". > > > Does the explanation above help? Is there something I should do in the code to > make it easier to understand? Thanks, I understand now. Yes, we should try to make it easier for others to understand at first glance. See my later suggestion about renaming one of the "Default" labels, and if that doesn't work out, maybe we can include some of your reply here in the declaration of the enum. https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2946113002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1490: if (GetContentClient()->browser()->ShouldIsolateFrameFromMainContent( On 2017/07/01 00:10:53, Łukasz A. wrote: > On 2017/06/30 23:28:45, Charlie Reis (OOO til July 5) wrote: > > Hmm. It feels a little odd to have the embedder make the ultimate decision > > about using TDI even thought content/ knows about > IsTopDocumentIsolationEnabled. > > I suppose that's why it was phrased as an override before. > > > > I guess I see the logic, though, since the ad-specific mode is easier to > express > > that way than as an override for the rest of cross-site iframes. I'm ok with > > it. > > Do you think that maybe the base::Feature for TopDocumentIsolation should also > move to the //chrome layer? This is doable, although we would have to change > how //content-layer browser tests opt into TDI (they would have to override > ContentBrowserClient rather than tweak command line flags). I think I'd rather keep it in content. Among other things, there's a fair amount of DefaultSubframeProcess machinery that feels like it belongs within RenderProcessHostImpl. Also, if it proves to be a useful mode down the line, this would make it easier for other embedders to use it. https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/browser... content/public/browser/content_browser_client.h:262: // to |url| and the main content is in the |main_frame_site_instance|). On 2017/07/01 00:10:53, Łukasz A. wrote: > On 2017/06/30 23:28:47, Charlie Reis (OOO til July 5) wrote: > > Let's clarify that > > (1) the dest_url is already assumed to be cross-site (right?) > > Correct. I've added this information to the comment. > > > and > > (2) the default should be to return true unless the embedder needs > > to override TopDocumentIsolation in some cases. > > I am not sure about this part. Returning false for non-ads seems pretty > natural. > > I think it is okay to leave the decision entirely up to the embedder (without > guiding toward selecting any specific set of frames to isolate). > Ok. My goal was to convey that a vanilla TDI mode (of isolating all cross-site iframes) would return true here rather than needing to do any kind of interesting work like checking if it's cross-site. I think the current wording is sufficient for that. https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:80: V(Xsite, 1, "isolate all frames from sites other than the top-level frame") \ On 2017/07/01 00:10:53, Łukasz A. wrote: > On 2017/06/30 23:28:49, Charlie Reis (OOO til July 5) wrote: > > We don't really use the term Xsite anywhere in Chrome. Would it be a problem > to > > say CrossSite (here and in CL description)? > > Thanks for catching "Xsite". I've changed this to "CrossSite". > > I think I still want to keep the mode description above though. I didn't want > to say that this mode isolates all cross-site frames, because this sounds very > much like --site-per-process and adding the "from top-level frame" qualifier > might not help much (i.e. if top-level is from a.com, then cross-site subframes > from b.com and c.com would both be put into the same TDI process). > > WDYT? Would you like to see a different description ("isolate all cross-site > frames from the top-level frame"?)? Oh, I'm fine with the mode description here in the code. I was referring to the Xsite mentions in the CL description. :) https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:79: // If TopDocumentIsolation is enabled, the mode for selecting *which* frames to nit: Let's be a bit more specific: If the kTopDocumentIsolation base::Feature is enabled, the values of this enum represent the mode for selecting *which* frames to isolate. https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:84: Default = 0, Are there any constraints on this name? I'm wondering if we can say something like Unspecified to distinguish it from the "Default" value from base::Feature. That would help cut down on my confusion. https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:91: V(Ads, 2, "isolate ads detected by heuristics") nit: "isolate only cross-site ads detected by heurisitcs"
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are new): 1. Default 2. Enabled 3. Enabled (Xsite - isolate all frames from sites other than the top-level frame) 4. Enabled (Ads - isolate ads detected by heuristics) 5. Disabled Today variations 1 and 5 turn off TDI mode. Variation 3 enables TDI and isolates all xsite frames (this is what browser tests use when enabling TDI via EnableTopDocumentIsolationForTesting). Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a better name for variation 2 would be "Enabled (default isolation mode)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - TDI controlled via chrome://flags and/or Finch experiments: no cmdline switches - TDI disabled: --disable-features=top-document-isolation - TDI enabled (default isolation): --enable-features=top-document-isolation - TDI enabled (specific isolation mode - 1 for xsite, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are introduced by this CL): 1. Default 2. Enabled 3. Enabled (CrossSite - isolate all frames from sites other than the top-level frame) 4. Enabled (Ads - isolate only cross-site ads detected by heuristics) 5. Disabled Today variations 1 and 5 turn off TDI mode (because the default for kTopDocumentIsolation base::Feature is to be disabled). Variation 3 enables TDI and isolates all cross-site frames (this is what browser tests use when enabling TDI via EnableTopDocumentIsolationForTesting). Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a better name for variation 2 would be "Enabled (unspecified isolation mode)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - When no extra cmdline switches are present, then TDI is controlled via chrome://flags and/or Finch experiments. - Cmdline flags to disable TDI: --disable-features=top-document-isolation - Cmdline flags to enable TDI using unspecified / default isolation mode: --enable-features=top-document-isolation - Cmdline flags to enable TDI using specific isolation mode (1 for cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... content/public/common/content_features.h:87: }; On 2017/06/30 15:28:37, Łukasz A. wrote: > I don't really know what is the best place for declaring/defining 1) the > kTopDocumentIsolationModeParam string/const char* and 2) the > TopDocumentIsolationMode enum. I think having them in content_features.h is > okay (although maybe I should move them to the end of this file). One > alternative would be to introduce > content/public/browser/top_document_isolation_feature.h and move 1) > kTopDocumentIsolation base::Feature, 2) kTopDocumentIsolationModeParam, 3) > TopDocumentIsolationMode enum, 4) EnableTopDocumentIsolationForTesting (and > maybe even 5. IsTopDocumentIsolationEnabled) into this new compilation unit - > this is done in a few other places in Chromium like: > > - chrome/browser/prerender/prerender_field_trial.h > - chrome/browser/experiments/memory_ablation_experiment.h (the only file in this > directory :-P) > - components/omnibox/browser/omnibox_field_trial.h I'm not sure either. I guess it feels a bit out of place in this file (which only has base::Features), so your top_document_isolation_feature.h file could make sense. Worth checking with people who know Features a bit more.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Charlie! https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/100001/content/public/common/... content/public/common/content_features.h:80: V(Xsite, 1, "isolate all frames from sites other than the top-level frame") \ On 2017/07/06 20:02:29, Charlie Reis (slow) wrote: > On 2017/07/01 00:10:53, Łukasz A. wrote: > > On 2017/06/30 23:28:49, Charlie Reis (OOO til July 5) wrote: > > > We don't really use the term Xsite anywhere in Chrome. Would it be a > problem > > to > > > say CrossSite (here and in CL description)? > > > > Thanks for catching "Xsite". I've changed this to "CrossSite". > > > > I think I still want to keep the mode description above though. I didn't want > > to say that this mode isolates all cross-site frames, because this sounds very > > much like --site-per-process and adding the "from top-level frame" qualifier > > might not help much (i.e. if top-level is from a.com, then cross-site > subframes > > from b.com and c.com would both be put into the same TDI process). > > > > WDYT? Would you like to see a different description ("isolate all cross-site > > frames from the top-level frame"?)? > > Oh, I'm fine with the mode description here in the code. I was referring to the > Xsite mentions in the CL description. :) Oh, right :-). I've updated the CL description. https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:79: // If TopDocumentIsolation is enabled, the mode for selecting *which* frames to On 2017/07/06 20:02:29, Charlie Reis (slow) wrote: > nit: Let's be a bit more specific: > If the kTopDocumentIsolation base::Feature is enabled, the values of this enum > represent the mode for selecting *which* frames to isolate. Done. I've also tweaked the comments for kTopDocumentIsolationModeParam and for TopDocumentIsolationMode::Unspecified. https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:84: Default = 0, On 2017/07/06 20:02:29, Charlie Reis (slow) wrote: > Are there any constraints on this name? I'm wondering if we can say something > like Unspecified to distinguish it from the "Default" value from base::Feature. > That would help cut down on my confusion. No constraints. I've renamed to Unspecified. https://codereview.chromium.org/2946113002/diff/160001/content/public/common/... content/public/common/content_features.h:91: V(Ads, 2, "isolate ads detected by heuristics") On 2017/07/06 20:02:29, Charlie Reis (slow) wrote: > nit: "isolate only cross-site ads detected by heurisitcs" Done.
lukasza@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@, could you PTAL? While in theory I already have l-g-t-m from owners, I would appreciate a quick review and feedback on the following: - //chrome/browser/about_flags.cc - //chrome/browser/chrome_content_browser_client.cc (for usage of base::GetFieldTrialParamByFeatureAsInt) - //content/public/test/test_utils.cc (mostly as an FYI for thoughts on whether something like this can be packaged in a reusable API and shared via //components/variations[/test_utils.h?]) - //content -> //components/variations dependency (already l-g-t-m-ed by jam@ from the //content side) - whether to keep the changes in //content/public/common/content_features.h or extract them into a separate compilation unit (e.g. top_document_isolation_trial.h). https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/80001/content/public/common/c... content/public/common/content_features.h:87: }; On 2017/07/06 20:40:35, Charlie Reis (slow) wrote: > On 2017/06/30 15:28:37, Łukasz A. wrote: > > I don't really know what is the best place for declaring/defining 1) the > > kTopDocumentIsolationModeParam string/const char* and 2) the > > TopDocumentIsolationMode enum. I think having them in content_features.h is > > okay (although maybe I should move them to the end of this file). One > > alternative would be to introduce > > content/public/browser/top_document_isolation_feature.h and move 1) > > kTopDocumentIsolation base::Feature, 2) kTopDocumentIsolationModeParam, 3) > > TopDocumentIsolationMode enum, 4) EnableTopDocumentIsolationForTesting (and > > maybe even 5. IsTopDocumentIsolationEnabled) into this new compilation unit - > > this is done in a few other places in Chromium like: > > > > - chrome/browser/prerender/prerender_field_trial.h > > - chrome/browser/experiments/memory_ablation_experiment.h (the only file in > this > > directory :-P) > > - components/omnibox/browser/omnibox_field_trial.h > > I'm not sure either. I guess it feels a bit out of place in this file (which > only has base::Features), so your top_document_isolation_feature.h file could > make sense. Worth checking with people who know Features a bit more. asvitkine@ - WDYT / what would you do here? Keep the changes as-is (slightly updated in the most recent patchset) VS move them into a separate/new compilation unit (top_document_isolation_trial.h in the same directory?). Hmmm... I guess the move can happen in a separate CL, to keep the current one self-contained / clean.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2946113002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1348: main_frame_site_instance)) Nit: {} https://codereview.chromium.org/2946113002/diff/200001/content/DEPS File content/DEPS (right): https://codereview.chromium.org/2946113002/diff/200001/content/DEPS#newcode28 content/DEPS:28: "+components/variations", I do wonder if we shouldn't perhaps restrict this more - i.e. only allow specific parts of the component to be includable? I suspect it should only be a handful of files, so maybe we could just list them here with a comment mentioning that only that functionality is supported to be used from content. https://codereview.chromium.org/2946113002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1463: } else { Nit: No need for else if there's a return above. Just move the one below outside the block. https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.cc:275: // Finch Experiment Param (see base::GetFieldTrialParamValueByFeature) In source code, we don't usually use the term "Finch". You can use Variation param or Field trial param. https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.h:82: // (e.g. "1" for TopDocumentIsolationMode::CrossSite). Nit: Remove extra space before "1". https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.h:86: // enum represent the mode for selecting *which* frames to isolate. Can you add a comment that mentions these are specified server-side and therefore should never be renumbered. https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... content/public/test/test_utils.cc:213: void EnableTopDocumentIsolationForTesting(base::CommandLine* command_line) { 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
asvitkine@ - thanks for the review. I think I've addressed most feedback, including the DEPS. OTOH, while I really like the VariationParamsManager suggestion, I've run into some trouble there. Can you take another look please? https://codereview.chromium.org/2946113002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2946113002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1348: main_frame_site_instance)) On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > Nit: {} Done. https://codereview.chromium.org/2946113002/diff/200001/content/DEPS File content/DEPS (right): https://codereview.chromium.org/2946113002/diff/200001/content/DEPS#newcode28 content/DEPS:28: "+components/variations", On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > I do wonder if we shouldn't perhaps restrict this more - i.e. only allow > specific parts of the component to be includable? I suspect it should only be a > handful of files, so maybe we could just list them here with a comment > mentioning that only that functionality is supported to be used from content. Good point. I've made changes that I think should address this concern. I've included a long set of notes below, but after making the changes I've made, I am not sure if they are that interesting. These are the includes I see after the CL: $ git grep include.*components.variations -- content/ content/browser/memory/memory_condition_observer.cc:#include "components/variations/variations_associated_data.h" content/browser/renderer_host/render_view_host_impl.cc:#include "components/variations/variations_associated_data.h" content/ppapi_plugin/ppapi_thread.h:#include "components/variations/child_process_field_trial_syncer.h" content/public/test/test_utils.cc:#include "components/variations/variations_switches.h" content/renderer/render_thread_impl.h:#include "components/variations/child_process_field_trial_syncer.h" I see the following groups of related functionality: - components/variations/child_process_field_trial_syncer.h (variations::ChildProcessFieldTrialSyncer) - For now I moved it back to content/renderer/DEPS and content/ppapi_plugin/DEPS and made it specific to this particular header. - This is a field in both content::RenderThreadImpl and content::PpapiThread. - Maybe this field should be moved to content::ChildThreadImpl? - Such move would help hide/encapsulate the dependency on components/variations/child_process_field_trial_syncer.h - This might require moving field_trial_syncer_.InitFieldTrialObserving(...) call from RenderThreadImpl::Init to the constructor of ChildThreadImpl. - This might require having ChildThreadImpl's constructor pass base::CommandLine::ForCurrentProcess as an argument for field_trial_syncer_.InitFieldTrialObserving (like RenderThreadImpl::Init does today; a little bit unlike PpapiThread does today [although ultimately the value of MainFunctionParams::command_line also comes from base::CommandLine::ForCurrentProcess - called in ContentMainRunnerImpl::Run] - I am not sure what to do about RenderThreadImpl::SetFieldTrialGroup. Maybe this can also move to ChildThreadImpl. - I wonder if not having SetFieldTrialGroup on PpapiThread might indicate a bug in propagation of field trial parameters to the ppapi process. - components/variations/variations_associated_data.h (GetVariations... functions) - This seems unused in memory_condition_observer.cc I've removed the include here. - This seems removable in render_view_host_impl.cc. OTOH, https://crbug.com/681160 is fixed and there is no active bug to track the removal... FWIW, I opened a new bug and moved this to "specific_include_rules" in content/browser/renderer_host/DEPS. - components/variations/variations_switches.h - This is the addition in this CL needed for testing. - As you suggested I am trying to use VariationsParamManager instead. - I've added a narrow dependency from //content's browser tests and test utils. https://codereview.chromium.org/2946113002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1463: } else { On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > Nit: No need for else if there's a return above. Just move the one below outside > the block. Done. https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.cc:275: // Finch Experiment Param (see base::GetFieldTrialParamValueByFeature) On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > In source code, we don't usually use the term "Finch". You can use Variation > param or Field trial param. Done. https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... File content/public/common/content_features.h (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.h:82: // (e.g. "1" for TopDocumentIsolationMode::CrossSite). On 2017/07/07 16:55:06, Alexei Svitkine (slow) wrote: > Nit: Remove extra space before "1". Ooops. How did that happen? I wanted to blame "git cl format", but I cannot repro. https://codereview.chromium.org/2946113002/diff/200001/content/public/common/... content/public/common/content_features.h:86: // enum represent the mode for selecting *which* frames to isolate. On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > Can you add a comment that mentions these are specified server-side and > therefore should never be renumbered. Done. https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... content/public/test/test_utils.cc:213: void EnableTopDocumentIsolationForTesting(base::CommandLine* command_line) { On 2017/07/07 16:55:06, Alexei Svitkine (slow) wrote: > 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... 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
Thanks for your detailed investigation and sorry for the extra yak shaving I've caused! Replies inline below. https://codereview.chromium.org/2946113002/diff/200001/content/DEPS File content/DEPS (right): https://codereview.chromium.org/2946113002/diff/200001/content/DEPS#newcode28 content/DEPS:28: "+components/variations", On 2017/07/07 20:41:42, Łukasz A. wrote: > On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > > I do wonder if we shouldn't perhaps restrict this more - i.e. only allow > > specific parts of the component to be includable? I suspect it should only be > a > > handful of files, so maybe we could just list them here with a comment > > mentioning that only that functionality is supported to be used from content. > > Good point. I've made changes that I think should address this concern. I've > included a long set of notes below, but after making the changes I've made, I am > not sure if they are that interesting. > > These are the includes I see after the CL: > > $ git grep include.*components.variations -- content/ > content/browser/memory/memory_condition_observer.cc:#include > "components/variations/variations_associated_data.h" > content/browser/renderer_host/render_view_host_impl.cc:#include > "components/variations/variations_associated_data.h" > content/ppapi_plugin/ppapi_thread.h:#include > "components/variations/child_process_field_trial_syncer.h" > content/public/test/test_utils.cc:#include > "components/variations/variations_switches.h" > content/renderer/render_thread_impl.h:#include > "components/variations/child_process_field_trial_syncer.h" > > I see the following groups of related functionality: > > - components/variations/child_process_field_trial_syncer.h > (variations::ChildProcessFieldTrialSyncer) > > - For now I moved it back to content/renderer/DEPS and > content/ppapi_plugin/DEPS > and made it specific to this particular header. That SGTM for now. Maybe we can file a crbug for the suggestions you have below? > > - This is a field in both content::RenderThreadImpl and content::PpapiThread. > > - Maybe this field should be moved to content::ChildThreadImpl? > > - Such move would help hide/encapsulate the dependency on > components/variations/child_process_field_trial_syncer.h > > - This might require moving field_trial_syncer_.InitFieldTrialObserving(...) > call > from RenderThreadImpl::Init to the constructor of ChildThreadImpl. > > - This might require having ChildThreadImpl's constructor pass > base::CommandLine::ForCurrentProcess as an argument for > field_trial_syncer_.InitFieldTrialObserving > (like RenderThreadImpl::Init does today; a little bit unlike > PpapiThread does today [although ultimately the value of > MainFunctionParams::command_line also comes from > base::CommandLine::ForCurrentProcess - called in > ContentMainRunnerImpl::Run] > > - I am not sure what to do about RenderThreadImpl::SetFieldTrialGroup. > Maybe this can also move to ChildThreadImpl. > > - I wonder if not having SetFieldTrialGroup on PpapiThread might indicate > a bug in propagation of field trial parameters to the ppapi process. > > > - components/variations/variations_associated_data.h (GetVariations... > functions) > > - This seems unused in memory_condition_observer.cc > I've removed the include here. > > - This seems removable in render_view_host_impl.cc. > OTOH, https://crbug.com/681160 is fixed and there is no active bug to track > the removal... > FWIW, I opened a new bug and moved this to "specific_include_rules" > in content/browser/renderer_host/DEPS. That SGTM. My mental model is we can add that include for now and it can be removed over time. > > - components/variations/variations_switches.h > > - This is the addition in this CL needed for testing. > > - As you suggested I am trying to use VariationsParamManager instead. > > - I've added a narrow dependency from //content's browser tests and test > utils. > https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... content/public/test/test_utils.cc:213: void EnableTopDocumentIsolationForTesting(base::CommandLine* command_line) { On 2017/07/07 20:41:42, Łukasz A. wrote: > On 2017/07/07 16:55:06, Alexei Svitkine (slow) wrote: > > 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... > > 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 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.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
asvitkine@, can you PTAL? https://codereview.chromium.org/2946113002/diff/200001/content/DEPS File content/DEPS (right): https://codereview.chromium.org/2946113002/diff/200001/content/DEPS#newcode28 content/DEPS:28: "+components/variations", On 2017/07/10 18:46:24, Alexei Svitkine (slow) wrote: > On 2017/07/07 20:41:42, Łukasz A. wrote: > > On 2017/07/07 16:55:05, Alexei Svitkine (slow) wrote: > > > I do wonder if we shouldn't perhaps restrict this more - i.e. only allow > > > specific parts of the component to be includable? I suspect it should only > be > > a > > > handful of files, so maybe we could just list them here with a comment > > > mentioning that only that functionality is supported to be used from > content. > > > > Good point. I've made changes that I think should address this concern. I've > > included a long set of notes below, but after making the changes I've made, I > am > > not sure if they are that interesting. > > > > These are the includes I see after the CL: > > > > $ git grep include.*components.variations -- content/ > > content/browser/memory/memory_condition_observer.cc:#include > > "components/variations/variations_associated_data.h" > > content/browser/renderer_host/render_view_host_impl.cc:#include > > "components/variations/variations_associated_data.h" > > content/ppapi_plugin/ppapi_thread.h:#include > > "components/variations/child_process_field_trial_syncer.h" > > content/public/test/test_utils.cc:#include > > "components/variations/variations_switches.h" > > content/renderer/render_thread_impl.h:#include > > "components/variations/child_process_field_trial_syncer.h" > > > > I see the following groups of related functionality: > > > > - components/variations/child_process_field_trial_syncer.h > > (variations::ChildProcessFieldTrialSyncer) > > > > - For now I moved it back to content/renderer/DEPS and > > content/ppapi_plugin/DEPS > > and made it specific to this particular header. > > That SGTM for now. Maybe we can file a crbug for the suggestions you have below? Done - https://crbug.com/740726. > > > > > - This is a field in both content::RenderThreadImpl and > content::PpapiThread. > > > > - Maybe this field should be moved to content::ChildThreadImpl? > > > > - Such move would help hide/encapsulate the dependency on > > components/variations/child_process_field_trial_syncer.h > > > > - This might require moving > field_trial_syncer_.InitFieldTrialObserving(...) > > call > > from RenderThreadImpl::Init to the constructor of ChildThreadImpl. > > > > - This might require having ChildThreadImpl's constructor pass > > base::CommandLine::ForCurrentProcess as an argument for > > field_trial_syncer_.InitFieldTrialObserving > > (like RenderThreadImpl::Init does today; a little bit unlike > > PpapiThread does today [although ultimately the value of > > MainFunctionParams::command_line also comes from > > base::CommandLine::ForCurrentProcess - called in > > ContentMainRunnerImpl::Run] > > > > - I am not sure what to do about RenderThreadImpl::SetFieldTrialGroup. > > Maybe this can also move to ChildThreadImpl. > > > > - I wonder if not having SetFieldTrialGroup on PpapiThread might indicate > > a bug in propagation of field trial parameters to the ppapi process. > > > > > > - components/variations/variations_associated_data.h (GetVariations... > > functions) > > > > - This seems unused in memory_condition_observer.cc > > I've removed the include here. > > > > - This seems removable in render_view_host_impl.cc. > > OTOH, https://crbug.com/681160 is fixed and there is no active bug to > track > > the removal... > > FWIW, I opened a new bug and moved this to "specific_include_rules" > > in content/browser/renderer_host/DEPS. > > That SGTM. My mental model is we can add that include for now and it can be > removed over time. Actually, I was able to already do it after rebasing - servolk@ has already kindly fixed https://crbug.com/740174 :-) > > > > > - components/variations/variations_switches.h > > > > - This is the addition in this CL needed for testing. > > > > - As you suggested I am trying to use VariationsParamManager instead. > > > > - I've added a narrow dependency from //content's browser tests and test > > utils. > > > https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/200001/content/public/test/te... content/public/test/test_utils.cc:213: void EnableTopDocumentIsolationForTesting(base::CommandLine* command_line) { On 2017/07/10 18:46:24, Alexei Svitkine (slow) wrote: > On 2017/07/07 20:41:42, Łukasz A. wrote: > > On 2017/07/07 16:55:06, Alexei Svitkine (slow) wrote: > > > 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... > > > > 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 > > 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? 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). > 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. 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
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lukasza@chromium.org changed reviewers: + jkarlin@chromium.org
jkarlin@, could you PTAL? I'd primarily like to get a review from you for the changes under chrome/browser/page_load_metrics/observer, but feel free to take a look at the whole CL (so that you are aware how the FrameIsAd function is used in TopDocumentIsolation mode). PS. I haven't yet finished addressing CR feedback from asvitkine@, but this should be addressed after rebasing on top of https://chromium-review.googlesource.com/c/565224 (and so I think starting parallel reviews in the meantime is okay).
On 2017/07/12 17:08:03, Łukasz A. wrote: > jkarlin@, could you PTAL? I'd primarily like to get a review from you for the > changes under chrome/browser/page_load_metrics/observer, but feel free to take a > look at the whole CL (so that you are aware how the FrameIsAd function is used > in TopDocumentIsolation mode). At what point do you need to make the TDI decision? I'm guessing very early? The Google AdFrame detection relies on the name attribute of the frame being updated, which doesn't happen immediately, but does happen before commit. I don't know exactly when though. Another concern I have is that I'm adding more ad detectors (e.g., see https://chromium-review.googlesource.com/c/567324/) and this API doesn't really fit for them. A public API would look more like FrameIsAd(WebContents* web_contents, int frame_tree_node_id) to be called after a frame commits. > > PS. I haven't yet finished addressing CR feedback from asvitkine@, but this > should be addressed after rebasing on top of > https://chromium-review.googlesource.com/c/565224 (and so I think starting > parallel reviews in the meantime is okay).
creis@ - could you PTAL below at one of my replies to jkarlin@'s questions? On 2017/07/12 18:01:40, jkarlin wrote: > On 2017/07/12 17:08:03, Łukasz A. wrote: > > jkarlin@, could you PTAL? I'd primarily like to get a review from you for the > > changes under chrome/browser/page_load_metrics/observer, but feel free to take > a > > look at the whole CL (so that you are aware how the FrameIsAd function is used > > in TopDocumentIsolation mode). > > At what point do you need to make the TDI decision? I'm guessing very early? The > Google AdFrame detection relies on the name attribute of the frame being > updated, which doesn't happen immediately, but does happen before commit. I > don't know exactly when though. The TDI decision is made when RenderFrameHostManager::DetermineSiteInstanceForURL runs. AFAIK this happens right before committing the navigation (i.e. is trigerred by RenderFrameHostManager::OnCrossSiteResponse). OTOH, I am not 100% sure here - creis@, can you please correct me if necessary? FWIW, this CL seems to put some frames into the TDI process on the test pages that you have kindly pointed me at (and the URL of which I've copied into the CL description). > Another concern I have is that I'm adding more ad detectors (e.g., see > https://chromium-review.googlesource.com/c/567324/) and this API doesn't really > fit for them. A public API would look more like FrameIsAd(WebContents* > web_contents, int frame_tree_node_id) to be called after a frame commits. Process allocation decision for the web frame is made when at the web frame's navigation commit time. :-/ I was hoping that the final frame URL and the current frame name would be sufficient for ad detection. :-(
On 2017/07/12 18:01:40, jkarlin wrote: > On 2017/07/12 17:08:03, Łukasz A. wrote: > > jkarlin@, could you PTAL? I'd primarily like to get a review from you for the > > changes under chrome/browser/page_load_metrics/observer, but feel free to take > a > > look at the whole CL (so that you are aware how the FrameIsAd function is used > > in TopDocumentIsolation mode). > > At what point do you need to make the TDI decision? I'm guessing very early? The > Google AdFrame detection relies on the name attribute of the frame being > updated, which doesn't happen immediately, but does happen before commit. I > don't know exactly when though. > > Another concern I have is that I'm adding more ad detectors (e.g., see > https://chromium-review.googlesource.com/c/567324/) and this API doesn't really > fit for them. A public API would look more like FrameIsAd(WebContents* > web_contents, int frame_tree_node_id) to be called after a frame commits. I've tried to look at your https://chromium-review.googlesource.com/c/567324, but it is not immediately obvious to me which heuristics are run after a frame commits. It seems to me that all the heuristics deal with frame navigations (not with subresources, despite the "subresource" in SubresourceFilterObserver class name). It seems to me that all the heuristics can run at commit time. Could you please help me understand which post-commit data is required as input for the heuristics?
Description was changed from ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Convenient website for testing that TDI consults the heuristics: http://pubtags-test.appspot.com/static/gpt-release/116/tests/index.html Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are introduced by this CL): 1. Default 2. Enabled 3. Enabled (CrossSite - isolate all frames from sites other than the top-level frame) 4. Enabled (Ads - isolate only cross-site ads detected by heuristics) 5. Disabled Today variations 1 and 5 turn off TDI mode (because the default for kTopDocumentIsolation base::Feature is to be disabled). Variation 3 enables TDI and isolates all cross-site frames (this is what browser tests use when enabling TDI via EnableTopDocumentIsolationForTesting). Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a better name for variation 2 would be "Enabled (unspecified isolation mode)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - When no extra cmdline switches are present, then TDI is controlled via chrome://flags and/or Finch experiments. - Cmdline flags to disable TDI: --disable-features=top-document-isolation - Cmdline flags to enable TDI using unspecified / default isolation mode: --enable-features=top-document-isolation - Cmdline flags to enable TDI using specific isolation mode (1 for cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode. FrameIsAd heuristics ==================== After this CL, TopDocumentIsolation will by default only isolate cross-site frames that also match FrameIsAd heuristics. This behavior is controllable via Finch, via chrome://flags and via command line (see below for details). Impact on browser tests ======================= Browser tests should not depend on the FrameIsAd heuristics. This is taken care of by EnableTopDocumentIsolationForTesting, which enables the mode that isolates all frames that are cross-site from the main frame. chrome://flags changes ====================== This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the following variations are present (3. and 4. are introduced by this CL): 1. Default 2. Enabled 3. Enabled (CrossSite - isolate all frames from sites other than the top-level frame) 4. Enabled (Ads - isolate only cross-site ads detected by heuristics) 5. Disabled Today variations 1 and 5 turn off TDI mode (because the default for kTopDocumentIsolation base::Feature is to be disabled). Variation 3 enables TDI and isolates all cross-site frames (this is what browser tests use when enabling TDI via EnableTopDocumentIsolationForTesting). Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a better name for variation 2 would be "Enabled (unspecified isolation mode)", but the "Enabled" string is hardcoded in the chrome://flags code. I've manually tested that if a user enabled TDI in chrome://flags before this CL, then it will stay enabled after this CL (variation 2). Command line ============ After this CL, the user can control TDI with the following command line switches: - When no extra cmdline switches are present, then TDI is controlled via chrome://flags and/or Finch experiments. - Cmdline flags to disable TDI: --disable-features=top-document-isolation - Cmdline flags to enable TDI using unspecified / default isolation mode: --enable-features=top-document-isolation - Cmdline flags to enable TDI using specific isolation mode (1 for cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all possible values): --enable-features="top-document-isolation<TopDocumentIsolation" \ --force-fieldtrials=TopDocumentIsolation/Cmdline \ --force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1 BUG=733303 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/07/12 18:19:42, Łukasz A. wrote: > creis@ - could you PTAL below at one of my replies to jkarlin@'s questions? > > On 2017/07/12 18:01:40, jkarlin wrote: > > On 2017/07/12 17:08:03, Łukasz A. wrote: > > > jkarlin@, could you PTAL? I'd primarily like to get a review from you for > the > > > changes under chrome/browser/page_load_metrics/observer, but feel free to > take > > a > > > look at the whole CL (so that you are aware how the FrameIsAd function is > used > > > in TopDocumentIsolation mode). > > > > At what point do you need to make the TDI decision? I'm guessing very early? > The > > Google AdFrame detection relies on the name attribute of the frame being > > updated, which doesn't happen immediately, but does happen before commit. I > > don't know exactly when though. > > The TDI decision is made when > RenderFrameHostManager::DetermineSiteInstanceForURL runs. AFAIK this happens > right before committing the navigation (i.e. is trigerred by > RenderFrameHostManager::OnCrossSiteResponse). OTOH, I am not 100% sure here - > creis@, can you please correct me if necessary? Correct-- we make the process model decision just before the page is ready to commit, since we can't move it after the commit. > FWIW, this CL seems to put some frames into the TDI process on the test pages > that you have kindly pointed me at (and the URL of which I've copied into the CL > description). > > > Another concern I have is that I'm adding more ad detectors (e.g., see > > https://chromium-review.googlesource.com/c/567324/) and this API doesn't > really > > fit for them. A public API would look more like FrameIsAd(WebContents* > > web_contents, int frame_tree_node_id) to be called after a frame commits. > > Process allocation decision for the web frame is made when at the web frame's > navigation commit time. :-/ I was hoping that the final frame URL and the > current frame name would be sufficient for ad detection. :-( On 2017/07/12 18:31:32, Łukasz A. wrote: > I've tried to look at your https://chromium-review.googlesource.com/c/567324, > but it is not immediately obvious to me which heuristics are run after a frame > commits. It seems to me that all the heuristics deal with frame navigations > (not with subresources, despite the "subresource" in SubresourceFilterObserver > class name). It seems to me that all the heuristics can run at commit time. > > Could you please help me understand which post-commit data is required as input > for the heuristics? Yes, more info from Josh here would be helpful. Any process model decision for TDI must be finished before the page commits in the frame, so it's possible that we won't be able to reuse the same heuristics if some of them look at the page after it has committed. Maybe there's a way to split the heuristic into parts that can be run before and after commit, and TDI will just use the first one (and be somewhat less accurate)?
I think I understand the concern from Josh a little bit better now. I think the concern is that after his https://chromium-review.googlesource.com/c/567324, ad frame detection 1) happens before commit, but is 2) asynchronous (which means that my IsAdFrame API doesn't fit in the future). Josh - is this a fair summary of the concern? I wonder if Josh's CL could somehow associate an is-ad-frame bit with a NavigationHandle? 1. This associating can happen at any time before the commit - I think this is compatible with what is happening in Josh's CL (although I admit I haven't yet fully grokked the CL). 2. OTOH, NavigationHandle doesn't currently support UserData (but maybe we can have a map from NavigationHandle to the is-ad-frame bit somewhere? not sure how to clean/maintain this map when a NavigationHandle is destroyed...). 3. I assume that RenderFrameHandleManager can easily access and pass a NavigationHandle to a (synchronous) IsAdFrame. This is probably better than just passing the URL and frame name (I think one can recover RenderFrameHost [and so also the frame name] from a NavigationHandle).
On 2017/07/12 19:30:25, Łukasz A. (OOO on Jul13-14) wrote: > I think I understand the concern from Josh a little bit better now. I think the > concern is that after his https://chromium-review.googlesource.com/c/567324, ad > frame detection 1) happens before commit, but is 2) asynchronous (which means > that my IsAdFrame API doesn't fit in the future). Josh - is this a fair summary > of the concern? > > I wonder if Josh's CL could somehow associate an is-ad-frame bit with a > NavigationHandle? > > 1. This associating can happen at any time before the commit - I think this is > compatible with what is happening in Josh's CL (although I admit I haven't yet > fully grokked the CL). > > 2. OTOH, NavigationHandle doesn't currently support UserData (but maybe we can > have a map from NavigationHandle to the is-ad-frame bit somewhere? not sure how > to clean/maintain this map when a NavigationHandle is destroyed...). > > 3. I assume that RenderFrameHandleManager can easily access and pass a > NavigationHandle to a (synchronous) IsAdFrame. This is probably better than > just passing the URL and frame name (I think one can recover RenderFrameHost > [and so also the frame name] from a NavigationHandle). I believe we're in agreement. I think the best way to move forward is to add an IsAd() method to NavigationHandle. I've put together a quick design doc here: https://docs.google.com/document/d/1fZj6g5Xjhk9ddu3ljyxCCitiNxsVd8E8H3vN7sFtY... I'm not 100% positive the design will work, I'll prototype and see. We also need to get content/ OWNERs to review the idea, but I don't think they're around until Monday.
On 2017/07/13 16:09:16, jkarlin wrote: > On 2017/07/12 19:30:25, Łukasz A. (OOO on Jul13-14) wrote: > > I think I understand the concern from Josh a little bit better now. I think > the > > concern is that after his https://chromium-review.googlesource.com/c/567324, > ad > > frame detection 1) happens before commit, but is 2) asynchronous (which means > > that my IsAdFrame API doesn't fit in the future). Josh - is this a fair > summary > > of the concern? > > > > I wonder if Josh's CL could somehow associate an is-ad-frame bit with a > > NavigationHandle? > > > > 1. This associating can happen at any time before the commit - I think this is > > compatible with what is happening in Josh's CL (although I admit I haven't yet > > fully grokked the CL). > > > > 2. OTOH, NavigationHandle doesn't currently support UserData (but maybe we can > > have a map from NavigationHandle to the is-ad-frame bit somewhere? not sure > how > > to clean/maintain this map when a NavigationHandle is destroyed...). > > > > 3. I assume that RenderFrameHandleManager can easily access and pass a > > NavigationHandle to a (synchronous) IsAdFrame. This is probably better than > > just passing the URL and frame name (I think one can recover RenderFrameHost > > [and so also the frame name] from a NavigationHandle). > > I believe we're in agreement. I think the best way to move forward is to add an > IsAd() method to NavigationHandle. I've put together a quick design doc here: > https://docs.google.com/document/d/1fZj6g5Xjhk9ddu3ljyxCCitiNxsVd8E8H3vN7sFtY... > > I'm not 100% positive the design will work, I'll prototype and see. We also need > to get content/ OWNERs to review the idea, but I don't think they're around > until Monday. Created crbug.com/742397 as well. Creis, wdyt?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asvitkine@, can you PTAL? AFAIK this CL should have all of your comments addressed now. PS. I've started a separate email thread to figure out if we can avoid tests trampling on cmdline switches passed via bot config. I hope that this email thread can progress in parallel, without blocking this CL (I think changes in the current CL stand on their own / are a step in the right direction regardless of the conclusions of the separate email thread). https://codereview.chromium.org/2946113002/diff/340001/content/public/test/te... File content/public/test/test_utils.cc (right): https://codereview.chromium.org/2946113002/diff/340001/content/public/test/te... content/public/test/test_utils.cc:242: command_line); This is now rebased on top of r486814.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jkarlin@chromium.org changed reviewers: + csharrison@chromium.org
+r csharrison for discussion about where to place the utility file. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.cc:88: if (IsGoogleAd(navigation_handle)) Perhaps: AdTypes GetAdDetectionHeuristics(content::NavigationHandle* navigation_handle) { const NavigationHandleAdsData* ads_data = NavigationHandleAdsData::From(navigation_handle); if (!ads_data->ad_types().test(AD_TYPE_GOOGLE) && IsGoogleAd(navigation_handle)) { ads_data->SetDetectedAdType(AD_TYPE_GOOGLE); } return ads_data->ad_types(); } That way we save the Google state once it's discovered and all of the detected types are stored in the NavigationHandleAdsData structure. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. I'm not sure that this utility file belongs in this directory. It may be best to just leave it in the observer file and include that. csharrison@ may have an alternative suggestion. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:30: // navigated). Discussion about the SetNavigationAdHeuristics stuff seems like implementation detail that doesn't belong in a public comment. How about: Uses heuristics to determine if |navigation_handle| is a navigation for an ad. Must be called no earlier than NavigationHandleImpl::ReadyToCommitNavigation. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:31: AdTypes GetAdDetectionHeuristics(content::NavigationHandle* navigation_handle); GetAdDetectionHeuristics sounds like it's returning heuristics, and not types. I strongly prefer GetDetectedAdTypes. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:34: void SetAdDetectionHeuristics(content::NavigationHandle* navigation_handle, Likewise, I much prefer SetDetectedAdType.
https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/18 17:34:44, jkarlin wrote: > I'm not sure that this utility file belongs in this directory. It may be best to > just leave it in the observer file and include that. > > csharrison@ may have an alternative suggestion. I would slightly prefer it to be in chrome/browser/page_load_metrics or even chrome/common/page_load_metrics if we think using it in the renderer ever makes sense. Honestly, page_load_metrics is a pretty weird place for it, so I was thinking it could maybe live somewhere in //content, but I don't really mind.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jkarlin@ / csharrison@ - can you take another look please? https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.cc:88: if (IsGoogleAd(navigation_handle)) On 2017/07/18 17:34:44, jkarlin wrote: > Perhaps: > > AdTypes GetAdDetectionHeuristics(content::NavigationHandle* navigation_handle) { > const NavigationHandleAdsData* ads_data = > NavigationHandleAdsData::From(navigation_handle); > > if (!ads_data->ad_types().test(AD_TYPE_GOOGLE) && > IsGoogleAd(navigation_handle)) { > ads_data->SetDetectedAdType(AD_TYPE_GOOGLE); > } > > return ads_data->ad_types(); > } > > That way we save the Google state once it's discovered and all of the detected > types are stored in the NavigationHandleAdsData structure. Good point. This probably means that NavigationHandleAdsData::From should create NavigationHandleAdsData if it doesn't already exist. This does seem cleaner / avoiding this allocation doesn't seem desirable (especially given that subresource filter will in most cases create NavigationHandleAdsData instance anyway). FWIW, I am checking IsGoogleAd inside NavigationHandleAdsData::From, in case when it needs to create a new NavigationHandleAdsData instance from scratch. The suggested change also allows returning |const AdTypes&| rather than |AdTypes| (i.e. avoids constructing a new AdTypes and possibly copying [although maybe RVO / copy elision is expected here]). Done. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/18 18:11:30, Charlie Harrison wrote: > On 2017/07/18 17:34:44, jkarlin wrote: > > I'm not sure that this utility file belongs in this directory. It may be best > to > > just leave it in the observer file and include that. > > > > csharrison@ may have an alternative suggestion. > > I would slightly prefer it to be in chrome/browser/page_load_metrics sgtm - I've moved ads_detection.h/.cc to chrome/browser/page_load_metrics. > or even > chrome/common/page_load_metrics if we think using it in the renderer ever makes > sense. NavigationHandle is a browser-side-only concept. > Honestly, page_load_metrics is a pretty weird place for it, so I was thinking it > could maybe live somewhere in //content, but I don't really mind. I've talked with nasko@ and I think creis@ also thinks that we should keep ads detection out of //content. Two reasons: 1. Different //content embedders (e.g. Chrome, Opera, Brave, ...) should be free to use different ad-detection heuristics. 2. Some //content embedders (e.g. Chromecast) might not be interested in ad-detection at all. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:30: // navigated). On 2017/07/18 17:34:44, jkarlin wrote: > Discussion about the SetNavigationAdHeuristics stuff seems like implementation > detail that doesn't belong in a public comment. > > How about: > Uses heuristics to determine if |navigation_handle| is a navigation for an ad. Done. > Must be called no earlier than NavigationHandleImpl::ReadyToCommitNavigation. Also done. Sadly, I don't see a way to DCHECK that this is properly used / that this is not used before ReadyToCommitNavigation. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:31: AdTypes GetAdDetectionHeuristics(content::NavigationHandle* navigation_handle); On 2017/07/18 17:34:44, jkarlin wrote: > GetAdDetectionHeuristics sounds like it's returning heuristics, and not types. I > strongly prefer GetDetectedAdTypes. Done. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:34: void SetAdDetectionHeuristics(content::NavigationHandle* navigation_handle, On 2017/07/18 17:34:44, jkarlin wrote: > Likewise, I much prefer SetDetectedAdType. Done.
Generally LGTM but let's wait for Josh's final OK since he has most context here. https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_detection.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/18 19:06:11, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/18 18:11:30, Charlie Harrison wrote: > > On 2017/07/18 17:34:44, jkarlin wrote: > > > I'm not sure that this utility file belongs in this directory. It may be > best > > to > > > just leave it in the observer file and include that. > > > > > > csharrison@ may have an alternative suggestion. > > > > I would slightly prefer it to be in chrome/browser/page_load_metrics > > sgtm - I've moved ads_detection.h/.cc to chrome/browser/page_load_metrics. > > > or even > > chrome/common/page_load_metrics if we think using it in the renderer ever > makes > > sense. > > NavigationHandle is a browser-side-only concept. Yeah, it would take some maneuvering to put it in //common (i.e. make it not depend on NavigationHandle/RFH), so I don't think we should do it yet. However, I could totally see places in the renderer wanting to know this kind of information, so it's worth keeping in mind. > > > Honestly, page_load_metrics is a pretty weird place for it, so I was thinking > it > > could maybe live somewhere in //content, but I don't really mind. > > I've talked with nasko@ and I think creis@ also thinks that we should keep ads > detection out of //content. Two reasons: > 1. Different //content embedders (e.g. Chrome, Opera, Brave, ...) should be free > to use different ad-detection heuristics. > 2. Some //content embedders (e.g. Chromecast) might not be interested in > ad-detection at all. Makes sense to me. https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:63: NavigationHandleAdsData* ads_data = static_cast<NavigationHandleAdsData*>( Optional: I would much prefer the following, to avoid raw new: if (NavigationHandleAdsData* existing_ads_data = ...) { return existing_ads_data; } auto ads_data = base::MakeUnique<NavigationHandleAdsData>(); auto* raw_ads_data = ads_data.get(); if (IsGoogleAd(navigation_handle)) ads_data->ad_types().set(AD_TYPE_GOOGLE); navigation_handle->SetUserData(kUserDataKey, std::move(ads_data)); // Comment about lifetime of the raw pointer. return raw_ads_data.get(); https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:92: }; DISALLOW_COPY_AND_ASSIGN?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/18 19:24:42, Charlie Harrison wrote: > Generally LGTM but let's wait for Josh's final OK since he has most context > here. Right - this CL still needs a green light from 1) jkarlin@ and 2) creis@ (mostly to approve making content::NavigationHandle inherit from base::SupportsUserData which is a change that was done *after* his l-g-t-m). https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:63: NavigationHandleAdsData* ads_data = static_cast<NavigationHandleAdsData*>( On 2017/07/18 19:24:41, Charlie Harrison wrote: > Optional: I would much prefer the following, to avoid raw new: > > if (NavigationHandleAdsData* existing_ads_data = ...) { > return existing_ads_data; > } > > auto ads_data = base::MakeUnique<NavigationHandleAdsData>(); > auto* raw_ads_data = ads_data.get(); > if (IsGoogleAd(navigation_handle)) > ads_data->ad_types().set(AD_TYPE_GOOGLE); > navigation_handle->SetUserData(kUserDataKey, std::move(ads_data)); > > // Comment about lifetime of the raw pointer. > return raw_ads_data.get(); Done (= I switched to base::MakeUnique<...>; OTOH, I did this in a slightly different way than suggested - for example we shouldn't do |raw_ads_data.get()| after already |std::move|-ing the unique pointer into SetUserData). https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:92: }; On 2017/07/18 19:24:41, Charlie Harrison wrote: > DISALLOW_COPY_AND_ASSIGN? Ooops. Yes - DISALLOW_COPY_AND_ASSIGN!
slgtm! https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:63: NavigationHandleAdsData* ads_data = static_cast<NavigationHandleAdsData*>( On 2017/07/18 20:04:29, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/18 19:24:41, Charlie Harrison wrote: > > Optional: I would much prefer the following, to avoid raw new: > > > > if (NavigationHandleAdsData* existing_ads_data = ...) { > > return existing_ads_data; > > } > > > > auto ads_data = base::MakeUnique<NavigationHandleAdsData>(); > > auto* raw_ads_data = ads_data.get(); > > if (IsGoogleAd(navigation_handle)) > > ads_data->ad_types().set(AD_TYPE_GOOGLE); > > navigation_handle->SetUserData(kUserDataKey, std::move(ads_data)); > > > > // Comment about lifetime of the raw pointer. > > return raw_ads_data.get(); > > Done (= I switched to base::MakeUnique<...>; OTOH, I did this in a slightly > different way than suggested - for example we shouldn't do |raw_ads_data.get()| > after already |std::move|-ing the unique pointer into SetUserData). Right, my typo, you can just return raw_ads_data directly.
BTW I am very happy to see NavigationHandle supporting user data. I strongly support that change to the API, as a non-owner. In fact, I was thinking of doing it myself sometime in the near future :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments from chrome/browser/page_load_metrics/ https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:20: page_load_metrics namespace https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:71: if (IsGoogleAd(navigation_handle)) The NavigationHandleAdsData might be created earlier (e.g., for subresource filter) than IsGoogleAd requires. I propose we run it as part of GetDetectedAdTypes on the first run of GetDetectedAdTypes. https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); An indirect (but better than nothing) way to verify that GetDetectedAdTypes is called late enough in the navigation process is to add: DCHECK(navigation_handle->GetRenderFrameHost()); https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.h:12: } // namespace content This file should be in the page_load_metrics namespace https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.h:29: void SetDetectedAdTypes(content::NavigationHandle* navigation_handle, s/SetDetectedAdTypes/SetDetectedAdType/ https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:182: SetDetectedAdTypes(navigation_handle, AD_TYPE_SUBRESOURCE_FILTER); Yay for this change. This is much nicer than unfinished_soubresource_ad_frames_.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Josh. Can you take another look please? https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:20: On 2017/07/19 13:32:36, jkarlin wrote: > page_load_metrics namespace Done. https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:71: if (IsGoogleAd(navigation_handle)) On 2017/07/19 13:32:36, jkarlin wrote: > The NavigationHandleAdsData might be created earlier (e.g., for subresource > filter) than IsGoogleAd requires. > > I propose we run it as part of GetDetectedAdTypes on the first run of > GetDetectedAdTypes. Good point (and sort of "done"), but: 1. Repeated calls to GetDetectedAdTypes will recalculate IsGoogleAd every time. To avoid recalculating, I would have to add a field to NavigationHandleAdsData to track whether ready-to-commit-time heuristics have been already calculated. Is this something you think is desirable? 2. Just to make sure I understand why the timing is important - it is important for A) making sure that subresource filter has time to do its thing and B) it is important for looking at the final (in case of redirects) NavigationHandle::GetURL. Does that sound right? FWIW, because of item 2 above, I've added the DCHECK to GetDetectedAdTypes (rather than just to IsGoogleAd). https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); On 2017/07/19 13:32:35, jkarlin wrote: > An indirect (but better than nothing) way to verify that GetDetectedAdTypes is > called late enough in the navigation process is to add: > > DCHECK(navigation_handle->GetRenderFrameHost()); > > Done, although this is somewhat tricky in case the navigation aborts earlier. I tried to work around the trickyness by looking at NavigationHandle::GetNetErrorCode in my new DCHECK - this avoided hitting the CHECK below in some of the tests: 38052:38052:0719/093355.488642:5009252265848:FATAL:navigation_handle_impl.cc(306)] Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should only be called after a response has been delivered for processing. #0 0x7f287f671967 base::debug::StackTrace::StackTrace() #1 0x7f287f698481 logging::LogMessage::~LogMessage() #2 0x7f287da694a6 content::NavigationHandleImpl::GetRenderFrameHost() #3 0x0000022e10eb page_load_metrics::GetDetectedAdTypes() #4 0x0000022e6223 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() #5 0x00000230ec7c page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() #6 0x0000022e31b0 page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() #7 0x7f287dd90037 content::WebContentsImpl::DidFinishNavigation() #8 0x7f287da68892 content::NavigationHandleImpl::~NavigationHandleImpl() #9 0x7f287da68f59 content::NavigationHandleImpl::~NavigationHandleImpl() #10 0x7f287da84561 content::RenderFrameHostImpl::OnDidStopLoading() ... #12 0x7f287da7a0c4 content::RenderFrameHostImpl::OnMessageReceived() #13 0x00000204cf2d content::NavigationSimulator::Fail() #14 0x000000a1d5cd AdsPageLoadMetricsObserverTest_TwoResourceLoadsBeforeCommit_Test::TestBody() I've discussed usage of GetNetErrorCode with nasko@ and it seems to be valid here (in particular it should cover all conditions that can lead to stopping a navigation - aborting, cancellation, etc.) https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.h (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.h:12: } // namespace content On 2017/07/19 13:32:36, jkarlin wrote: > This file should be in the page_load_metrics namespace Done. https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.h:29: void SetDetectedAdTypes(content::NavigationHandle* navigation_handle, On 2017/07/19 13:32:36, jkarlin wrote: > s/SetDetectedAdTypes/SetDetectedAdType/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); On 2017/07/19 18:11:29, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/19 13:32:35, jkarlin wrote: > > An indirect (but better than nothing) way to verify that GetDetectedAdTypes is > > called late enough in the navigation process is to add: > > > > DCHECK(navigation_handle->GetRenderFrameHost()); > > > > > > Done, although this is somewhat tricky in case the navigation aborts earlier. I > tried to work around the trickyness by looking at > NavigationHandle::GetNetErrorCode in my new DCHECK - this avoided hitting the > CHECK below in some of the tests: > > 38052:38052:0719/093355.488642:5009252265848:FATAL:navigation_handle_impl.cc(306)] > Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should only > be called after a response has been delivered for processing. > #0 0x7f287f671967 base::debug::StackTrace::StackTrace() > #1 0x7f287f698481 logging::LogMessage::~LogMessage() > #2 0x7f287da694a6 content::NavigationHandleImpl::GetRenderFrameHost() > #3 0x0000022e10eb page_load_metrics::GetDetectedAdTypes() > #4 0x0000022e6223 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() > #5 0x00000230ec7c > page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() > #6 0x0000022e31b0 > page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() > #7 0x7f287dd90037 content::WebContentsImpl::DidFinishNavigation() > #8 0x7f287da68892 content::NavigationHandleImpl::~NavigationHandleImpl() > #9 0x7f287da68f59 content::NavigationHandleImpl::~NavigationHandleImpl() > #10 0x7f287da84561 content::RenderFrameHostImpl::OnDidStopLoading() > ... > #12 0x7f287da7a0c4 content::RenderFrameHostImpl::OnMessageReceived() > #13 0x00000204cf2d content::NavigationSimulator::Fail() > #14 0x000000a1d5cd > AdsPageLoadMetricsObserverTest_TwoResourceLoadsBeforeCommit_Test::TestBody() > > I've discussed usage of GetNetErrorCode with nasko@ and it seems to be valid > here (in particular it should cover all conditions that can lead to stopping a > navigation - aborting, cancellation, etc.) Actually this didn't quite work with PlzNavigate: $ DISPLAY=:20 out/gn/browser_tests \ --gtest_filter=*SafeBrowsingBlockingPageBrowserTest.SecurityStateGoBackOnSubresource* \ --enable-browser-side-navigation ... [22546:22546:0719/133042.337131:FATAL:navigation_handle_impl.cc(306)] Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should onl y be called after a response has been delivered for processing. #0 0x7f1c2ba87b87 base::debug::StackTrace::StackTrace() #1 0x7f1c2baae6a1 logging::LogMessage::~LogMessage() #2 0x7f1c27f290b6 content::NavigationHandleImpl::GetRenderFrameHost() #3 0x0000018a2201 page_load_metrics::GetDetectedAdTypes() #4 0x000002f32bd3 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() #5 0x0000018b806c page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() #6 0x0000018a41e0 page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() #7 0x7f1c2824de87 content::WebContentsImpl::DidFinishNavigation() #8 0x7f1c27f284a2 content::NavigationHandleImpl::~NavigationHandleImpl() #9 0x7f1c27f28b69 content::NavigationHandleImpl::~NavigationHandleImpl() #10 0x7f1c27f2ee23 content::NavigationRequest::~NavigationRequest() #11 0x7f1c27f2ef89 content::NavigationRequest::~NavigationRequest() #12 0x7f1c27f12c95 content::FrameTreeNode::~FrameTreeNode() #13 0x7f1c27f1324e content::FrameTreeNode::RemoveChild() #14 0x7f1c27f10e3a content::FrameTree::RemoveFrame() ... #16 0x7f1c27f399d4 content::RenderFrameHostImpl::OnMessageReceived() ... #26 0x000001db866b content::MessageLoopRunner::Run() #27 0x000001d84ae0 content::RunTaskAndWaitForInterstitialDetach() #28 0x000001d849ef content::WaitForInterstitialDetach() #29 0x000001231826 safe_browsing::SafeBrowsingBlockingPageBrowserTest_SecurityStateGoBackOnSubresourceInterstitial_Test::RunTestOnMainThread() So - I've removed the DCHECK and opened https://crbug.com/746638 to track the problem.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/browser/page_load_metrics/ lgtm, thanks! https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:71: if (IsGoogleAd(navigation_handle)) On 2017/07/19 18:11:29, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/19 13:32:36, jkarlin wrote: > > The NavigationHandleAdsData might be created earlier (e.g., for subresource > > filter) than IsGoogleAd requires. > > > > I propose we run it as part of GetDetectedAdTypes on the first run of > > GetDetectedAdTypes. > > Good point (and sort of "done"), but: > > 1. Repeated calls to GetDetectedAdTypes will recalculate IsGoogleAd every time. > To avoid recalculating, I would have to add a field to NavigationHandleAdsData > to track whether ready-to-commit-time heuristics have been already calculated. > Is this something you think is desirable? The calculation cost isn't terrible, but I'd prefer if we stored a has_calculated_google_ads_ bit in the AdsData object. > 2. Just to make sure I understand why the timing is important - it is important > for A) making sure that subresource filter has time to do its thing and B) it is > important for looking at the final (in case of redirects) > NavigationHandle::GetURL. Does that sound right? And it's important to wait for the name attribute to propagate to the browser process. > > FWIW, because of item 2 above, I've added the DCHECK to GetDetectedAdTypes > (rather than just to IsGoogleAd). https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); On 2017/07/19 22:44:26, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/19 18:11:29, Łukasz A. (OOO Jul21-Aug17) wrote: > > On 2017/07/19 13:32:35, jkarlin wrote: > > > An indirect (but better than nothing) way to verify that GetDetectedAdTypes > is > > > called late enough in the navigation process is to add: > > > > > > DCHECK(navigation_handle->GetRenderFrameHost()); > > > > > > > > > > Done, although this is somewhat tricky in case the navigation aborts earlier. > I > > tried to work around the trickyness by looking at > > NavigationHandle::GetNetErrorCode in my new DCHECK - this avoided hitting the > > CHECK below in some of the tests: > > > > > 38052:38052:0719/093355.488642:5009252265848:FATAL:navigation_handle_impl.cc(306)] > > Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should > only > > be called after a response has been delivered for processing. > > #0 0x7f287f671967 base::debug::StackTrace::StackTrace() > > #1 0x7f287f698481 logging::LogMessage::~LogMessage() > > #2 0x7f287da694a6 content::NavigationHandleImpl::GetRenderFrameHost() > > #3 0x0000022e10eb page_load_metrics::GetDetectedAdTypes() > > #4 0x0000022e6223 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() > > #5 0x00000230ec7c > > page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() > > #6 0x0000022e31b0 > > page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() > > #7 0x7f287dd90037 content::WebContentsImpl::DidFinishNavigation() > > #8 0x7f287da68892 content::NavigationHandleImpl::~NavigationHandleImpl() > > #9 0x7f287da68f59 content::NavigationHandleImpl::~NavigationHandleImpl() > > #10 0x7f287da84561 content::RenderFrameHostImpl::OnDidStopLoading() > > ... > > #12 0x7f287da7a0c4 content::RenderFrameHostImpl::OnMessageReceived() > > #13 0x00000204cf2d content::NavigationSimulator::Fail() > > #14 0x000000a1d5cd > > AdsPageLoadMetricsObserverTest_TwoResourceLoadsBeforeCommit_Test::TestBody() > > > > I've discussed usage of GetNetErrorCode with nasko@ and it seems to be valid > > here (in particular it should cover all conditions that can lead to stopping a > > navigation - aborting, cancellation, etc.) > > Actually this didn't quite work with PlzNavigate: > > $ DISPLAY=:20 out/gn/browser_tests \ > > --gtest_filter=*SafeBrowsingBlockingPageBrowserTest.SecurityStateGoBackOnSubresource* > \ > --enable-browser-side-navigation > ... > [22546:22546:0719/133042.337131:FATAL:navigation_handle_impl.cc(306)] Check > failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should onl > y be called after a response has been delivered for processing. > #0 0x7f1c2ba87b87 base::debug::StackTrace::StackTrace() > #1 0x7f1c2baae6a1 logging::LogMessage::~LogMessage() > #2 0x7f1c27f290b6 content::NavigationHandleImpl::GetRenderFrameHost() > #3 0x0000018a2201 page_load_metrics::GetDetectedAdTypes() > #4 0x000002f32bd3 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() > #5 0x0000018b806c > page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() > #6 0x0000018a41e0 > page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() > #7 0x7f1c2824de87 content::WebContentsImpl::DidFinishNavigation() > #8 0x7f1c27f284a2 content::NavigationHandleImpl::~NavigationHandleImpl() > #9 0x7f1c27f28b69 content::NavigationHandleImpl::~NavigationHandleImpl() > #10 0x7f1c27f2ee23 content::NavigationRequest::~NavigationRequest() > #11 0x7f1c27f2ef89 content::NavigationRequest::~NavigationRequest() > #12 0x7f1c27f12c95 content::FrameTreeNode::~FrameTreeNode() > #13 0x7f1c27f1324e content::FrameTreeNode::RemoveChild() > #14 0x7f1c27f10e3a content::FrameTree::RemoveFrame() > ... > #16 0x7f1c27f399d4 content::RenderFrameHostImpl::OnMessageReceived() > ... > #26 0x000001db866b content::MessageLoopRunner::Run() > #27 0x000001d84ae0 content::RunTaskAndWaitForInterstitialDetach() > #28 0x000001d849ef content::WaitForInterstitialDetach() > #29 0x000001231826 > safe_browsing::SafeBrowsingBlockingPageBrowserTest_SecurityStateGoBackOnSubresourceInterstitial_Test::RunTestOnMainThread() > > So - I've removed the DCHECK and opened https://crbug.com/746638 to track the > problem. Sorry, I should have realized that it wouldn't work with PlzNavigate. Though if the bug gets resolved and you can check only when there is no error that would be great. https://codereview.chromium.org/2946113002/diff/520001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/520001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); Please add a TODO to add a DCHECK that the navigation is far enough along in the process and reference the bug you made.
Thanks! LGTM with nits. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:36: // once a frame has an ad we always consider it to have an ad. Are we ok with this for the process model decision (i.e., once an ad frame, always an ad frame)? I guess that's probably ok. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:39: // bytes and not granting security priveleges). nit: privileges https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:63: class NavigationHandleAdsData : public base::SupportsUserData::Data { nit: Please add comment, along the lines of "Associates the detected AdTypes for a navigation with its NavigationHandle." https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:100: const char NavigationHandleAdsData::kUserDataKey[] = "AdsData"; nit: Move above the NavigationHandleAdsData class.
https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:36: // once a frame has an ad we always consider it to have an ad. On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > Are we ok with this for the process model decision (i.e., once an ad frame, > always an ad frame)? I guess that's probably ok. Actually, that comment (the once an ad always an ad comment) is specific to PageLoadMetrics and doesn't belong here. Let's use GetRenderFrameHost and return false if it's nullptr.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks - please see my replies below. https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:71: if (IsGoogleAd(navigation_handle)) On 2017/07/20 16:51:51, jkarlin wrote: > On 2017/07/19 18:11:29, Łukasz A. (OOO Jul21-Aug17) wrote: > > On 2017/07/19 13:32:36, jkarlin wrote: > > > The NavigationHandleAdsData might be created earlier (e.g., for subresource > > > filter) than IsGoogleAd requires. > > > > > > I propose we run it as part of GetDetectedAdTypes on the first run of > > > GetDetectedAdTypes. > > > > Good point (and sort of "done"), but: > > > > 1. Repeated calls to GetDetectedAdTypes will recalculate IsGoogleAd every > time. > > To avoid recalculating, I would have to add a field to NavigationHandleAdsData > > to track whether ready-to-commit-time heuristics have been already calculated. > > > Is this something you think is desirable? > > The calculation cost isn't terrible, but I'd prefer if we stored a > has_calculated_google_ads_ bit in the AdsData object. > Done. > > > 2. Just to make sure I understand why the timing is important - it is > important > > for A) making sure that subresource filter has time to do its thing and B) it > is > > important for looking at the final (in case of redirects) > > NavigationHandle::GetURL. Does that sound right? > > And it's important to wait for the name attribute to propagate to the browser > process. I think that the javascript initiating the navigation doesn't have control over the navigation timing (especially if navigating to a cross-site URL / when the target frame might end-up being hosted in a separate renderer process). Therefore, I think that the javascript code has to set the frame name first and *then* initiate the navigation (i.e. I think the new frame name will always propagate to the browser first). > > > > > FWIW, because of item 2 above, I've added the DCHECK to GetDetectedAdTypes > > (rather than just to IsGoogleAd). https://codereview.chromium.org/2946113002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); On 2017/07/20 16:51:51, jkarlin wrote: > On 2017/07/19 22:44:26, Łukasz A. (OOO Jul21-Aug17) wrote: > > On 2017/07/19 18:11:29, Łukasz A. (OOO Jul21-Aug17) wrote: > > > On 2017/07/19 13:32:35, jkarlin wrote: > > > > An indirect (but better than nothing) way to verify that > GetDetectedAdTypes > > is > > > > called late enough in the navigation process is to add: > > > > > > > > DCHECK(navigation_handle->GetRenderFrameHost()); > > > > > > > > > > > > > > Done, although this is somewhat tricky in case the navigation aborts > earlier. > > I > > > tried to work around the trickyness by looking at > > > NavigationHandle::GetNetErrorCode in my new DCHECK - this avoided hitting > the > > > CHECK below in some of the tests: > > > > > > > > > 38052:38052:0719/093355.488642:5009252265848:FATAL:navigation_handle_impl.cc(306)] > > > Check failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should > > only > > > be called after a response has been delivered for processing. > > > #0 0x7f287f671967 base::debug::StackTrace::StackTrace() > > > #1 0x7f287f698481 logging::LogMessage::~LogMessage() > > > #2 0x7f287da694a6 content::NavigationHandleImpl::GetRenderFrameHost() > > > #3 0x0000022e10eb page_load_metrics::GetDetectedAdTypes() > > > #4 0x0000022e6223 > AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() > > > #5 0x00000230ec7c > > > page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() > > > #6 0x0000022e31b0 > > > page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() > > > #7 0x7f287dd90037 content::WebContentsImpl::DidFinishNavigation() > > > #8 0x7f287da68892 content::NavigationHandleImpl::~NavigationHandleImpl() > > > #9 0x7f287da68f59 content::NavigationHandleImpl::~NavigationHandleImpl() > > > #10 0x7f287da84561 content::RenderFrameHostImpl::OnDidStopLoading() > > > ... > > > #12 0x7f287da7a0c4 content::RenderFrameHostImpl::OnMessageReceived() > > > #13 0x00000204cf2d content::NavigationSimulator::Fail() > > > #14 0x000000a1d5cd > > > AdsPageLoadMetricsObserverTest_TwoResourceLoadsBeforeCommit_Test::TestBody() > > > > > > I've discussed usage of GetNetErrorCode with nasko@ and it seems to be valid > > > here (in particular it should cover all conditions that can lead to stopping > a > > > navigation - aborting, cancellation, etc.) > > > > Actually this didn't quite work with PlzNavigate: > > > > $ DISPLAY=:20 out/gn/browser_tests \ > > > > > --gtest_filter=*SafeBrowsingBlockingPageBrowserTest.SecurityStateGoBackOnSubresource* > > \ > > --enable-browser-side-navigation > > ... > > [22546:22546:0719/133042.337131:FATAL:navigation_handle_impl.cc(306)] Check > > failed: state_ >= WILL_PROCESS_RESPONSE (1 vs. 6)This accessor should onl > > y be called after a response has been delivered for processing. > > #0 0x7f1c2ba87b87 base::debug::StackTrace::StackTrace() > > #1 0x7f1c2baae6a1 logging::LogMessage::~LogMessage() > > #2 0x7f1c27f290b6 content::NavigationHandleImpl::GetRenderFrameHost() > > #3 0x0000018a2201 page_load_metrics::GetDetectedAdTypes() > > #4 0x000002f32bd3 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation() > > #5 0x0000018b806c > > page_load_metrics::PageLoadTracker::DidFinishSubFrameNavigation() > > #6 0x0000018a41e0 > > page_load_metrics::MetricsWebContentsObserver::DidFinishNavigation() > > #7 0x7f1c2824de87 content::WebContentsImpl::DidFinishNavigation() > > #8 0x7f1c27f284a2 content::NavigationHandleImpl::~NavigationHandleImpl() > > #9 0x7f1c27f28b69 content::NavigationHandleImpl::~NavigationHandleImpl() > > #10 0x7f1c27f2ee23 content::NavigationRequest::~NavigationRequest() > > #11 0x7f1c27f2ef89 content::NavigationRequest::~NavigationRequest() > > #12 0x7f1c27f12c95 content::FrameTreeNode::~FrameTreeNode() > > #13 0x7f1c27f1324e content::FrameTreeNode::RemoveChild() > > #14 0x7f1c27f10e3a content::FrameTree::RemoveFrame() > > ... > > #16 0x7f1c27f399d4 content::RenderFrameHostImpl::OnMessageReceived() > > ... > > #26 0x000001db866b content::MessageLoopRunner::Run() > > #27 0x000001d84ae0 content::RunTaskAndWaitForInterstitialDetach() > > #28 0x000001d849ef content::WaitForInterstitialDetach() > > #29 0x000001231826 > > > safe_browsing::SafeBrowsingBlockingPageBrowserTest_SecurityStateGoBackOnSubresourceInterstitial_Test::RunTestOnMainThread() > > > > So - I've removed the DCHECK and opened https://crbug.com/746638 to track the > > problem. > > Sorry, I should have realized that it wouldn't work with PlzNavigate. Though if > the bug gets resolved and you can check only when there is no error that would > be great. I've added a TODO here. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:36: // once a frame has an ad we always consider it to have an ad. On 2017/07/20 17:19:43, jkarlin wrote: > On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > > Are we ok with this for the process model decision (i.e., once an ad frame, > > always an ad frame)? I guess that's probably ok. I think this is okay for now. > > Actually, that comment (the once an ad always an ad comment) is specific to > PageLoadMetrics and doesn't belong here. > > Let's use GetRenderFrameHost and return false if it's nullptr. I agree that we should use |navigation_handle->GetRenderFrameHost()| here (and delete the comment above), but this is problematic because of https://crbug.com/746638 (which makes it hard to distinguish between navigation-finished was called because 1) navigation has committed or 2) navigation was aborted or cancelled or has failed. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:39: // bytes and not granting security priveleges). On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > nit: privileges Done. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:63: class NavigationHandleAdsData : public base::SupportsUserData::Data { On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > nit: Please add comment, along the lines of "Associates the detected AdTypes for > a navigation with its NavigationHandle." Done. https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:100: const char NavigationHandleAdsData::kUserDataKey[] = "AdsData"; On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > nit: Move above the NavigationHandleAdsData class. This wouldn't compile (error: use of undeclared identifier 'NavigationHandleAdsData'). I could make this compile by making kUserDataKey a namespace-level variable, but I prefer to keep kUserDataKey a static, *private* member of NavigationHandleAdsData class. I tried to get rid of the separate definition, by moving the string literal into the class declaration, but I couldn't make this compile (constexpr is needed, but then things fail to link). https://codereview.chromium.org/2946113002/diff/520001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/520001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:106: DCHECK(navigation_handle); On 2017/07/20 16:51:51, jkarlin wrote: > Please add a TODO to add a DCHECK that the navigation is far enough along in the > process and reference the bug you made. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, asvitkine@chromium.org, csharrison@chromium.org, creis@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2946113002/#ps540001 (title: "Addressing CR feedback from jkarlin@ and creis@.")
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": 540001, "attempt_start_ts": 1500580734649540,
"parent_rev": "94bed167487c90db4d6c445ada0170d45397a4e0", "commit_rev":
"c1dd61f01ced0c94ccc2efe373a93c696bb98c2f"}
Message was sent while issue was closed.
Description was changed from
==========
Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.
FrameIsAd heuristics
====================
After this CL, TopDocumentIsolation will by default only isolate
cross-site frames that also match FrameIsAd heuristics. This behavior
is controllable via Finch, via chrome://flags and via command line
(see below for details).
Impact on browser tests
=======================
Browser tests should not depend on the FrameIsAd heuristics. This is
taken care of by EnableTopDocumentIsolationForTesting, which enables
the mode that isolates all frames that are cross-site from the main
frame.
chrome://flags changes
======================
This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the
following variations are present (3. and 4. are introduced by this CL):
1. Default
2. Enabled
3. Enabled (CrossSite - isolate all frames from sites other than
the top-level frame)
4. Enabled (Ads - isolate only cross-site ads detected by heuristics)
5. Disabled
Today variations 1 and 5 turn off TDI mode (because the default for
kTopDocumentIsolation base::Feature is to be disabled). Variation 3
enables TDI and isolates all cross-site frames (this is what browser
tests use when enabling TDI via EnableTopDocumentIsolationForTesting).
Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a
better name for variation 2 would be "Enabled (unspecified isolation
mode)", but the "Enabled" string is hardcoded in the chrome://flags
code.
I've manually tested that if a user enabled TDI in chrome://flags before
this CL, then it will stay enabled after this CL (variation 2).
Command line
============
After this CL, the user can control TDI with the following command line
switches:
- When no extra cmdline switches are present, then TDI is controlled via
chrome://flags and/or Finch experiments.
- Cmdline flags to disable TDI:
--disable-features=top-document-isolation
- Cmdline flags to enable TDI using unspecified / default isolation mode:
--enable-features=top-document-isolation
- Cmdline flags to enable TDI using specific isolation mode (1 for
cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all
possible values):
--enable-features="top-document-isolation<TopDocumentIsolation" \
--force-fieldtrials=TopDocumentIsolation/Cmdline \
--force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1
BUG=733303
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.
FrameIsAd heuristics
====================
After this CL, TopDocumentIsolation will by default only isolate
cross-site frames that also match FrameIsAd heuristics. This behavior
is controllable via Finch, via chrome://flags and via command line
(see below for details).
Impact on browser tests
=======================
Browser tests should not depend on the FrameIsAd heuristics. This is
taken care of by EnableTopDocumentIsolationForTesting, which enables
the mode that isolates all frames that are cross-site from the main
frame.
chrome://flags changes
======================
This CL tweaks TopDocumentIsolation's entry in chrome://flags, so that the
following variations are present (3. and 4. are introduced by this CL):
1. Default
2. Enabled
3. Enabled (CrossSite - isolate all frames from sites other than
the top-level frame)
4. Enabled (Ads - isolate only cross-site ads detected by heuristics)
5. Disabled
Today variations 1 and 5 turn off TDI mode (because the default for
kTopDocumentIsolation base::Feature is to be disabled). Variation 3
enables TDI and isolates all cross-site frames (this is what browser
tests use when enabling TDI via EnableTopDocumentIsolationForTesting).
Variations 2 and 4 enable TDI and use FrameIsAd heuristics. Probably a
better name for variation 2 would be "Enabled (unspecified isolation
mode)", but the "Enabled" string is hardcoded in the chrome://flags
code.
I've manually tested that if a user enabled TDI in chrome://flags before
this CL, then it will stay enabled after this CL (variation 2).
Command line
============
After this CL, the user can control TDI with the following command line
switches:
- When no extra cmdline switches are present, then TDI is controlled via
chrome://flags and/or Finch experiments.
- Cmdline flags to disable TDI:
--disable-features=top-document-isolation
- Cmdline flags to enable TDI using unspecified / default isolation mode:
--enable-features=top-document-isolation
- Cmdline flags to enable TDI using specific isolation mode (1 for
cross-site, 2 for ads; see the TopDocumentIsolationMode enum for all
possible values):
--enable-features="top-document-isolation<TopDocumentIsolation" \
--force-fieldtrials=TopDocumentIsolation/Cmdline \
--force-fieldtrial-params=TopDocumentIsolation.Cmdline:mode/1
BUG=733303
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2946113002
Cr-Commit-Position: refs/heads/master@{#488372}
Committed:
https://chromium.googlesource.com/chromium/src/+/c1dd61f01ced0c94ccc2efe373a9...
==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001) as https://chromium.googlesource.com/chromium/src/+/c1dd61f01ced0c94ccc2efe373a9...
Message was sent while issue was closed.
https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/ads_detection.cc (right): https://codereview.chromium.org/2946113002/diff/500001/chrome/browser/page_lo... chrome/browser/page_load_metrics/ads_detection.cc:100: const char NavigationHandleAdsData::kUserDataKey[] = "AdsData"; On 2017/07/20 17:33:51, Łukasz A. (OOO Jul21-Aug17) wrote: > On 2017/07/20 17:06:16, Charlie Reis (slow) wrote: > > nit: Move above the NavigationHandleAdsData class. > > This wouldn't compile (error: use of undeclared identifier > 'NavigationHandleAdsData'). > > I could make this compile by making kUserDataKey a namespace-level variable, but > I prefer to keep kUserDataKey a static, *private* member of > NavigationHandleAdsData class. > > I tried to get rid of the separate definition, by moving the string literal into > the class declaration, but I couldn't make this compile (constexpr is needed, > but then things fail to link). Ah, sorry, I didn't see it was a member, just that it was used before this line. No need to move.
Message was sent while issue was closed.
A revert of this CL (patchset #28 id:540001) has been created in https://codereview.chromium.org/2987563004/ by lukasza@chromium.org. The reason for reverting is: This CL has caused crashes reported in https://crbug.com/747403. |
