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

chrome/browser/page_load_metrics/ads_detection.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/page_load_metrics/ads_detection.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+#include "base/supports_user_data.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "url/gurl.h"
+

[jkarlin, 2017/07/19 13:32:36]

page_load_metrics namespace

[Łukasz Anforowicz, 2017/07/19 18:11:29]

Done.

+namespace {
+
+bool IsGoogleAd(content::NavigationHandle* navigation_handle) {
+  // Because sub-resource filtering isn't always enabled, and doesn't work
+  // well in monitoring mode (no CSS enforcement), it's difficult to identify
+  // ads. Google ads are prevalent and easy to track, so we'll start by
+  // tracking those. Note that the frame name can be very large, so be careful
+  // to avoid full string searches if possible.
+  // TODO(jkarlin): Track other ad networks that are easy to identify.
+
+  // In case the navigation aborted, look up the RFH by the Frame Tree Node
+  // ID. It returns the committed frame host or the initial frame host for the
+  // frame if no committed host exists. Using a previous host is fine because
+  // once a frame has an ad we always consider it to have an ad.
+  // We use the unsafe method of FindFrameByFrameTreeNodeId because we're not
+  // concerned with which process the frame lives on (we're just measuring
+  // bytes and not granting security priveleges).
+  content::RenderFrameHost* current_frame_host =
+      navigation_handle->GetWebContents()->UnsafeFindFrameByFrameTreeNodeId(
+          navigation_handle->GetFrameTreeNodeId());
+  if (current_frame_host) {
+    const std::string& frame_name = current_frame_host->GetFrameName();
+    if (base::StartsWith(frame_name, "google_ads_iframe",
+                         base::CompareCase::SENSITIVE) ||
+        base::StartsWith(frame_name, "google_ads_frame",
+                         base::CompareCase::SENSITIVE)) {
+      return true;
+    }
+  }
+
+  const GURL& frame_url = navigation_handle->GetURL();
+  if (frame_url.host_piece() == "tpc.googlesyndication.com" &&
+      base::StartsWith(frame_url.path_piece(), "/safeframe",
+                       base::CompareCase::SENSITIVE)) {
+    return true;
+  }
+
+  return false;
+}
+
+class NavigationHandleAdsData : public base::SupportsUserData::Data {
+ public:
+  static NavigationHandleAdsData* GetOrCreate(
+      content::NavigationHandle* navigation_handle) {
+    DCHECK(navigation_handle);
+    NavigationHandleAdsData* ads_data = static_cast<NavigationHandleAdsData*>(
+        navigation_handle->GetUserData(kUserDataKey));
+    if (!ads_data) {
+      std::unique_ptr<NavigationHandleAdsData> new_ads_data =
+          base::MakeUnique<NavigationHandleAdsData>();
+      if (IsGoogleAd(navigation_handle))

[jkarlin, 2017/07/19 13:32:36]

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.

[Łukasz Anforowicz, 2017/07/19 18:11:29]

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).

[jkarlin, 2017/07/20 16:51:51]

The calculation cost isn't terrible, but I'd prefer if we stored a
has_calculated_google_ads_ bit in the AdsData object.
And it's important to wait for the name attribute to propagate to the browser
process.

[Łukasz Anforowicz, 2017/07/20 17:33:51]

Done.
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).

+        new_ads_data->ad_types().set(AD_TYPE_GOOGLE);
+
+      // It is safe to retain |ads_data| raw pointer, despite passing an
+      // ownership of |new_ads_data| to SetUserData, because |navigation_handle|
+      // will keep the NavigationHandleAdsData instance alive until the
+      // |navigation_handle| is destroyed.
+      ads_data = new_ads_data.get();
+
+      navigation_handle->SetUserData(kUserDataKey, std::move(new_ads_data));
+    }
+
+    return ads_data;
+  }
+
+  NavigationHandleAdsData() = default;
+  ~NavigationHandleAdsData() override {}
+
+  AdTypes& ad_types() { return ad_types_; }
+  const AdTypes& ad_types() const { return ad_types_; }
+
+ private:
+  AdTypes ad_types_;
+
+  static const char kUserDataKey[];
+
+  DISALLOW_COPY_AND_ASSIGN(NavigationHandleAdsData);
+};
+
+const char NavigationHandleAdsData::kUserDataKey[] = "AdsData";
+
+}  // namespace
+
+const AdTypes& GetDetectedAdTypes(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(navigation_handle);

[jkarlin, 2017/07/19 13:32:35]

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());

[Łukasz Anforowicz, 2017/07/19 18:11:29]

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.)

[Łukasz Anforowicz, 2017/07/19 22:44:26]

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.

[jkarlin, 2017/07/20 16:51:51]

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.

[Łukasz Anforowicz, 2017/07/20 17:33:51]

I've added a TODO here.

+  return NavigationHandleAdsData::GetOrCreate(navigation_handle)->ad_types();
+}
+
+void SetDetectedAdTypes(content::NavigationHandle* navigation_handle,
+                        AdType type) {
+  DCHECK(navigation_handle);
+  NavigationHandleAdsData::GetOrCreate(navigation_handle)->ad_types().set(type);
+}