Adapt webview tests for SB CI and fix WebviewClient callbacks

This CL adapts webview tests to run with committed interstitials
enabled, and fixes WebViewClient callback invocation issues with CI.

All SafeBrowsing CTS tests pass with CI as of this CL, except for
android.webkit.cts.WebViewClientTest#testOnSafeBrowsingHit which is
fixed in the follow-up CL.

Tests will be enabled for committed interstitials using
fieldtrial_testing_config.json once the blocking bugs are fixed.

Bug: 1031315, 1022477

Change-Id: Iadf1848cd91c29fd3b841429f74fb4e3464a055f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1909276
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735185}

android_webview/browser/aw_contents.cc

@@ -47,6 +47,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/i18n/rtl.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
@@ -65,6 +66,8 @@
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
 #include "components/navigation_interception/intercept_navigation_delegate.h"
+#include "components/safe_browsing/core/features.h"
+#include "components/security_interstitials/content/security_interstitial_tab_helper.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "content/public/browser/android/child_process_importance.h"
 #include "content/public/browser/android/synchronous_compositor.h"
@@ -1411,6 +1414,22 @@ void AwContents::DidFinishNavigation(
       error_code != net::ERR_ABORTED) {
     return;
   }
+
+  // We do not call OnReceivedError for requests that were blocked due to an
+  // interstitial showing. OnReceivedError is handled directly by the blocking
+  // page for interstitials.
+  if (base::FeatureList::IsEnabled(safe_browsing::kCommittedSBInterstitials)) {
+    security_interstitials::SecurityInterstitialTabHelper*
+        security_interstitial_tab_helper = security_interstitials::
+            SecurityInterstitialTabHelper::FromWebContents(web_contents_.get());
+    if (security_interstitial_tab_helper &&
+        (security_interstitial_tab_helper->IsInterstitialPendingForNavigation(
+             navigation_handle->GetNavigationId()) ||
+         security_interstitial_tab_helper->IsDisplayingInterstitial())) {
+      return;
+    }
+  }
+
   AwContentsClientBridge* client =
       AwContentsClientBridge::FromWebContents(web_contents_.get());
   if (!client)
@@ -1422,8 +1441,7 @@ void AwContents::DidFinishNavigation(
                                navigation_handle->HasUserGesture(),
                                net::HttpRequestHeaders());
   request.is_renderer_initiated = navigation_handle->IsRendererInitiated();
-
-  client->OnReceivedError(request, error_code, false);
+  client->OnReceivedError(request, error_code, false, false);
 }
 
 void AwContents::DidAttachInterstitialPage() {
@@ -1455,19 +1473,28 @@ int AwContents::GetErrorUiType() {
   return Java_AwContents_getErrorUiType(env, obj);
 }
 
+// TODO(carlosil): Once committed interstitials are the only codepath supported
+// this will have nothing that's interstitial specific so this function should
+// be cleaned up.
 void AwContents::EvaluateJavaScriptOnInterstitialForTesting(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
     const base::android::JavaParamRef<jstring>& script,
     const base::android::JavaParamRef<jobject>& callback) {
-  content::InterstitialPage* interstitial =
-      web_contents_->GetInterstitialPage();
-  DCHECK(interstitial);
+  content::RenderFrameHost* main_frame;
+  if (base::FeatureList::IsEnabled(safe_browsing::kCommittedSBInterstitials)) {
+    main_frame = web_contents_->GetMainFrame();
+  } else {
+    content::InterstitialPage* interstitial =
+        web_contents_->GetInterstitialPage();
+    DCHECK(interstitial);
+    main_frame = interstitial->GetMainFrame();
+  }
 
   if (!callback) {
     // No callback requested.
-    interstitial->GetMainFrame()->ExecuteJavaScriptForTests(
-        ConvertJavaStringToUTF16(env, script), base::NullCallback());
+    main_frame->ExecuteJavaScriptForTests(ConvertJavaStringToUTF16(env, script),
+                                          base::NullCallback());
     return;
   }
 
@@ -1478,8 +1505,8 @@ void AwContents::EvaluateJavaScriptOnInterstitialForTesting(
   RenderFrameHost::JavaScriptResultCallback js_callback =
       base::BindOnce(&JavaScriptResultCallbackForTesting, j_callback);
 
-  interstitial->GetMainFrame()->ExecuteJavaScriptForTests(
-      ConvertJavaStringToUTF16(env, script), std::move(js_callback));
+  main_frame->ExecuteJavaScriptForTests(ConvertJavaStringToUTF16(env, script),
+                                        std::move(js_callback));
 }
 
 void AwContents::RendererUnresponsive(

android_webview/browser/aw_contents_client_bridge.cc

@@ -435,7 +435,8 @@ void AwContentsClientBridge::NewLoginRequest(const std::string& realm,
 void AwContentsClientBridge::OnReceivedError(
     const AwWebResourceRequest& request,
     int error_code,
-    bool safebrowsing_hit) {
+    bool safebrowsing_hit,
+    bool should_omit_notifications_for_safebrowsing_hit) {
   DCHECK(request.is_renderer_initiated.has_value());
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   JNIEnv* env = AttachCurrentThread();
@@ -454,7 +455,7 @@ void AwContentsClientBridge::OnReceivedError(
       java_web_resource_request.jmethod,
       java_web_resource_request.jheader_names,
       java_web_resource_request.jheader_values, error_code, jstring_description,
-      safebrowsing_hit);
+      safebrowsing_hit, should_omit_notifications_for_safebrowsing_hit);
 }
 
 void AwContentsClientBridge::OnSafeBrowsingHit(

android_webview/browser/aw_contents_client_bridge.h

@@ -116,7 +116,8 @@ class AwContentsClientBridge {
   // host name lookup failure etc.)
   void OnReceivedError(const AwWebResourceRequest& request,
                        int error_code,
-                       bool safebrowsing_hit);
+                       bool safebrowsing_hit,
+                       bool should_omit_notifications_for_safebrowsing_hit);
 
   void OnSafeBrowsingHit(const AwWebResourceRequest& request,
                          const safe_browsing::SBThreatType& threat_type,

android_webview/browser/aw_feature_list.cc

@@ -10,6 +10,7 @@
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
+#include "components/safe_browsing/core/features.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::JavaParamRef;
@@ -23,6 +24,7 @@ namespace {
 // in other locations in the code base (e.g. content/, components/, etc).
 const base::Feature* kFeaturesExposedToJava[] = {
     &features::kWebViewConnectionlessSafeBrowsing,
+    &safe_browsing::kCommittedSBInterstitials,
 };
 
 const base::Feature* FindFeatureExposedToJava(const std::string& feature_name) {

android_webview/browser/network_service/aw_proxying_url_loader_factory.cc

@@ -480,7 +480,7 @@ void OnReceivedErrorOnUiThread(int process_id,
                   << request.url;
     return;
   }
-  client->OnReceivedError(request, error_code, safebrowsing_hit);
+  client->OnReceivedError(request, error_code, safebrowsing_hit, true);
 }
 
 void OnNewLoginRequestOnUiThread(int process_id,

android_webview/browser/network_service/aw_web_resource_request.cc

@@ -61,6 +61,8 @@ AwWebResourceRequest::AwWebResourceRequest(
   ConvertRequestHeadersToVectors(in_headers, &header_names, &header_values);
 }
 
+AwWebResourceRequest::AwWebResourceRequest(const AwWebResourceRequest& other) =
+    default;
 AwWebResourceRequest::AwWebResourceRequest(AwWebResourceRequest&& other) =
     default;
 AwWebResourceRequest& AwWebResourceRequest::operator=(

android_webview/browser/network_service/aw_web_resource_request.h

@@ -35,6 +35,7 @@ struct AwWebResourceRequest final {
 
   // Add default copy/move/assign operators. Adding explicit destructor
   // prevents generating move operator.
+  AwWebResourceRequest(const AwWebResourceRequest& other);
   AwWebResourceRequest(AwWebResourceRequest&& other);
   AwWebResourceRequest& operator=(AwWebResourceRequest&& other);
   ~AwWebResourceRequest();

android_webview/browser/safe_browsing/aw_safe_browsing_blocking_page.cc

@@ -9,11 +9,14 @@
 
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_browser_process.h"
+#include "android_webview/browser/aw_contents_client_bridge.h"
+#include "android_webview/browser/network_service/aw_web_resource_request.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_ui_manager.h"
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/content/browser/threat_details.h"
+#include "components/safe_browsing/core/common/safebrowsing_constants.h"
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/triggers/trigger_manager.h"
 #include "components/security_interstitials/content/security_interstitial_controller_client.h"
@@ -25,6 +28,8 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
+#include "net/base/net_errors.h"
+
 using content::InterstitialPage;
 using content::WebContents;
 using security_interstitials::BaseSafeBrowsingErrorUI;
@@ -40,14 +45,16 @@ AwSafeBrowsingBlockingPage::AwSafeBrowsingBlockingPage(
     const UnsafeResourceList& unsafe_resources,
     std::unique_ptr<SecurityInterstitialControllerClient> controller_client,
     const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options,
-    ErrorUiType errorUiType)
+    ErrorUiType errorUiType,
+    std::unique_ptr<AwWebResourceRequest> resource_request)
     : BaseBlockingPage(ui_manager,
                        web_contents,
                        main_frame_url,
                        unsafe_resources,
                        std::move(controller_client),
                        display_options),
-      threat_details_in_progress_(false) {
+      threat_details_in_progress_(false),
+      resource_request_(std::move(resource_request)) {
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.Interstitial.Type", errorUiType,
                             ErrorUiType::COUNT);
   if (errorUiType == ErrorUiType::QUIET_SMALL ||
@@ -98,7 +105,7 @@ void AwSafeBrowsingBlockingPage::ShowBlockingPage(
     // the new one will automatically hide the old one.
     AwSafeBrowsingBlockingPage* blocking_page = CreateBlockingPage(
         ui_manager, unsafe_resource.web_contents_getter.Run(), GURL(),
-        unsafe_resource);
+        unsafe_resource, nullptr);
     blocking_page->Show();
   }
 }
@@ -107,7 +114,8 @@ AwSafeBrowsingBlockingPage* AwSafeBrowsingBlockingPage::CreateBlockingPage(
     AwSafeBrowsingUIManager* ui_manager,
     content::WebContents* web_contents,
     const GURL& main_frame_url,
-    const UnsafeResource& unsafe_resource) {
+    const UnsafeResource& unsafe_resource,
+    std::unique_ptr<AwWebResourceRequest> resource_request) {
   const UnsafeResourceList unsafe_resources{unsafe_resource};
   AwBrowserContext* browser_context =
       AwBrowserContext::FromWebContents(web_contents);
@@ -139,9 +147,11 @@ AwSafeBrowsingBlockingPage* AwSafeBrowsingBlockingPage::CreateBlockingPage(
       ui_manager, web_contents, url, unsafe_resources,
       CreateControllerClient(web_contents, unsafe_resources, ui_manager,
                              pref_service),
-      display_options, errorType);
+      display_options, errorType, std::move(resource_request));
 }
 
+AwSafeBrowsingBlockingPage::~AwSafeBrowsingBlockingPage() {}
+
 void AwSafeBrowsingBlockingPage::FinishThreatDetails(
     const base::TimeDelta& delay,
     bool did_proceed,
@@ -165,4 +175,21 @@ void AwSafeBrowsingBlockingPage::FinishThreatDetails(
   }
 }
 
+void AwSafeBrowsingBlockingPage::OnInterstitialClosing() {
+  if (resource_request_ && !proceeded()) {
+    // resource_request_ should only be set for committed interstitials.
+    DCHECK(
+        base::FeatureList::IsEnabled(safe_browsing::kCommittedSBInterstitials));
+    AwContentsClientBridge* client =
+        AwContentsClientBridge::FromWebContents(web_contents());
+    // With committed interstitials, the navigation to the site is failed before
+    // showing the interstitial so we omit notifications to embedders at that
+    // time, and manually trigger them here.
+    client->OnReceivedError(*resource_request_,
+                            safe_browsing::GetNetErrorCodeForSafeBrowsing(),
+                            true, false);
+  }
+  safe_browsing::BaseBlockingPage::OnInterstitialClosing();
+}
+
 }  // namespace android_webview

android_webview/browser/safe_browsing/aw_safe_browsing_blocking_page.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "android_webview/browser/network_service/aw_web_resource_request.h"
 #include "components/safe_browsing/content/base_blocking_page.h"
 #include "components/security_interstitials/core/base_safe_browsing_error_ui.h"
 
@@ -33,7 +34,10 @@ class AwSafeBrowsingBlockingPage : public safe_browsing::BaseBlockingPage {
       AwSafeBrowsingUIManager* ui_manager,
       content::WebContents* web_contents,
       const GURL& main_frame_url,
-      const UnsafeResource& unsafe_resource);
+      const UnsafeResource& unsafe_resource,
+      std::unique_ptr<AwWebResourceRequest> resource_request);
+
+  ~AwSafeBrowsingBlockingPage() override;
 
  protected:
   // Used to specify which BaseSafeBrowsingErrorUI to instantiate, and
@@ -53,7 +57,8 @@ class AwSafeBrowsingBlockingPage : public safe_browsing::BaseBlockingPage {
           security_interstitials::SecurityInterstitialControllerClient>
           controller_client,
       const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options,
-      ErrorUiType errorUiType);
+      ErrorUiType errorUiType,
+      std::unique_ptr<AwWebResourceRequest> resource_request);
 
   // Called when the interstitial is going away. If there is a
   // pending threat details object, we look at the user's
@@ -63,9 +68,15 @@ class AwSafeBrowsingBlockingPage : public safe_browsing::BaseBlockingPage {
                            bool did_proceed,
                            int num_visits) override;
 
+  void OnInterstitialClosing() override;
+
   // Whether ThreatDetails collection is in progress as part of this
   // interstitial.
   bool threat_details_in_progress_;
+
+  // Holds a copy of the resource request that triggered this blocking page,
+  // only used with committed interstitials.
+  std::unique_ptr<AwWebResourceRequest> resource_request_;
 };
 
 }  // namespace android_webview

android_webview/browser/safe_browsing/aw_safe_browsing_navigation_throttle.cc

@@ -4,7 +4,10 @@
 
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_navigation_throttle.h"
 
+#include <memory>
+
 #include "android_webview/browser/aw_browser_process.h"
+#include "android_webview/browser/network_service/aw_web_resource_request.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_blocking_page.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_ui_manager.h"
 #include "base/memory/ptr_util.h"
@@ -30,9 +33,16 @@ AwSafeBrowsingNavigationThrottle::WillFailRequest() {
     security_interstitials::UnsafeResource resource;
     content::NavigationHandle* handle = navigation_handle();
     if (manager->PopUnsafeResourceForURL(handle->GetURL(), &resource)) {
+      std::unique_ptr<AwWebResourceRequest> request =
+          std::make_unique<AwWebResourceRequest>(
+              handle->GetURL().spec(), handle->IsPost() ? "POST" : "GET",
+              handle->IsInMainFrame(), handle->HasUserGesture(),
+              handle->GetRequestHeaders());
+      request->is_renderer_initiated = handle->IsRendererInitiated();
       AwSafeBrowsingBlockingPage* blocking_page =
           AwSafeBrowsingBlockingPage::CreateBlockingPage(
-              manager, handle->GetWebContents(), handle->GetURL(), resource);
+              manager, handle->GetWebContents(), handle->GetURL(), resource,
+              std::move(request));
       std::string error_page_content = blocking_page->GetHTMLContents();
       security_interstitials::SecurityInterstitialTabHelper::
           AssociateBlockingPage(handle->GetWebContents(),

android_webview/browser/safe_browsing/aw_safe_browsing_subresource_helper.cc

@@ -4,7 +4,10 @@
 
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_subresource_helper.h"
 
+#include <memory>
+
 #include "android_webview/browser/aw_browser_process.h"
+#include "android_webview/browser/network_service/aw_web_resource_request.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_blocking_page.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_ui_manager.h"
 #include "components/security_interstitials/content/security_interstitial_tab_helper.h"
@@ -27,10 +30,18 @@ void AwSafeBrowsingSubresourceHelper::ReadyToCommitNavigation(
     security_interstitials::UnsafeResource resource;
     if (manager->PopUnsafeResourceForURL(navigation_handle->GetURL(),
                                          &resource)) {
+      std::unique_ptr<AwWebResourceRequest> request =
+          std::make_unique<AwWebResourceRequest>(
+              navigation_handle->GetURL().spec(),
+              navigation_handle->IsPost() ? "POST" : "GET",
+              navigation_handle->IsInMainFrame(),
+              navigation_handle->HasUserGesture(),
+              navigation_handle->GetRequestHeaders());
+      request->is_renderer_initiated = navigation_handle->IsRendererInitiated();
       AwSafeBrowsingBlockingPage* blocking_page =
           AwSafeBrowsingBlockingPage::CreateBlockingPage(
               manager, navigation_handle->GetWebContents(),
-              navigation_handle->GetURL(), resource);
+              navigation_handle->GetURL(), resource, std::move(request));
       security_interstitials::SecurityInterstitialTabHelper::
           AssociateBlockingPage(navigation_handle->GetWebContents(),
                                 navigation_handle->GetNavigationId(),

android_webview/browser/safe_browsing/aw_safe_browsing_ui_manager.cc

@@ -143,7 +143,7 @@ AwSafeBrowsingUIManager::CreateBlockingPageForSubresource(
   AwSafeBrowsingSubresourceHelper::CreateForWebContents(contents);
   AwSafeBrowsingBlockingPage* blocking_page =
       AwSafeBrowsingBlockingPage::CreateBlockingPage(
-          this, contents, blocked_url, unsafe_resource);
+          this, contents, blocked_url, unsafe_resource, nullptr);
   return blocking_page;
 }
 

android_webview/browser/safe_browsing/aw_url_checker_delegate_impl.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "android_webview/browser/aw_browser_context.h"
+#include "android_webview/browser/aw_contents.h"
 #include "android_webview/browser/aw_contents_client_bridge.h"
 #include "android_webview/browser/aw_contents_io_thread_client.h"
 #include "android_webview/browser/network_service/aw_web_resource_request.h"
@@ -18,9 +19,11 @@
 #include "android_webview/browser_jni_headers/AwSafeBrowsingConfigHelper_jni.h"
 #include "base/android/jni_android.h"
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/task/post_task.h"
 #include "components/safe_browsing/core/db/database_manager.h"
 #include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
+#include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/web_ui/constants.h"
 #include "components/security_interstitials/content/unsafe_resource_util.h"
 #include "components/security_interstitials/core/unsafe_resource.h"
@@ -30,9 +33,29 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
+#include "ui/base/page_transition_types.h"
 
 namespace android_webview {
 
+namespace {
+void CallOnReceivedError(AwContentsClientBridge* client,
+                         AwWebResourceRequest request,
+                         content::NavigationEntry* entry) {
+  if (!client)
+    return;
+  // We have no way of telling if a navigation was renderer initiated if entry
+  // is null but OnReceivedError requires the value to be set, we default to
+  // false for that case.
+  request.is_renderer_initiated =
+      entry ? ui::PageTransitionIsWebTriggerable(entry->GetTransitionType())
+            : false;
+
+  // We use ERR_ABORTED here since this is only used in cases where no
+  // interstitial is shown.
+  client->OnReceivedError(request, net::ERR_ABORTED, true, false);
+}
+}  // namespace
+
 AwUrlCheckerDelegateImpl::AwUrlCheckerDelegateImpl(
     scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager,
     scoped_refptr<AwSafeBrowsingUIManager> ui_manager,
@@ -143,7 +166,7 @@ void AwUrlCheckerDelegateImpl::StartApplicationResponse(
   if (client) {
     base::OnceCallback<void(SafeBrowsingAction, bool)> callback =
         base::BindOnce(&AwUrlCheckerDelegateImpl::DoApplicationResponse,
-                       ui_manager, resource);
+                       ui_manager, resource, request);
 
     client->OnSafeBrowsingHit(request, resource.threat_type,
                               std::move(callback));
@@ -154,6 +177,7 @@ void AwUrlCheckerDelegateImpl::StartApplicationResponse(
 void AwUrlCheckerDelegateImpl::DoApplicationResponse(
     scoped_refptr<AwSafeBrowsingUIManager> ui_manager,
     const security_interstitials::UnsafeResource& resource,
+    const AwWebResourceRequest& request,
     SafeBrowsingAction action,
     bool reporting) {
   content::WebContents* web_contents = resource.web_contents_getter.Run();
@@ -164,6 +188,8 @@ void AwUrlCheckerDelegateImpl::DoApplicationResponse(
     browser_context->SetExtendedReportingAllowed(false);
   }
 
+  content::NavigationEntry* entry = GetNavigationEntryForResource(resource);
+
   // TODO(ntfschr): fully handle reporting once we add support (crbug/688629)
   bool proceed;
   switch (action) {
@@ -173,6 +199,19 @@ void AwUrlCheckerDelegateImpl::DoApplicationResponse(
           base::BindOnce(
               &AwUrlCheckerDelegateImpl::StartDisplayingDefaultBlockingPage,
               ui_manager, resource));
+      if (base::FeatureList::IsEnabled(
+              safe_browsing::kCommittedSBInterstitials)) {
+        AwContents* contents = AwContents::FromWebContents(web_contents);
+        if (!contents || !contents->CanShowInterstitial()) {
+          // With committed interstitials OnReceivedError for safe browsing
+          // blocked sites is generally called from the blocking page object.
+          // When CanShowInterstitial is false, no blocking page is created from
+          // StartDisplayingDefaultBlockingPage, so we handle the call here.
+          CallOnReceivedError(
+              AwContentsClientBridge::FromWebContents(web_contents), request,
+              entry);
+        }
+      }
       return;
     case SafeBrowsingAction::PROCEED:
       proceed = true;
@@ -184,8 +223,14 @@ void AwUrlCheckerDelegateImpl::DoApplicationResponse(
       NOTREACHED();
   }
 
-  content::NavigationEntry* entry = GetNavigationEntryForResource(resource);
-  GURL main_frame_url = entry ? entry->GetURL() : GURL();
+  if (!proceed &&
+      base::FeatureList::IsEnabled(safe_browsing::kCommittedSBInterstitials)) {
+    // With committed interstitials OnReceivedError for safe browsing blocked
+    // sites is generally called from the blocking page object. Since no
+    // blocking page is created in this case, we manually call it here.
+    CallOnReceivedError(AwContentsClientBridge::FromWebContents(web_contents),
+                        request, entry);
+  }
 
   // Navigate back for back-to-safety on subresources
   if (!proceed && resource.is_subframe) {
@@ -198,6 +243,7 @@ void AwUrlCheckerDelegateImpl::DoApplicationResponse(
     }
   }
 
+  GURL main_frame_url = entry ? entry->GetURL() : GURL();
   ui_manager->OnBlockingPageDone(
       std::vector<security_interstitials::UnsafeResource>{resource}, proceed,
       web_contents, main_frame_url);

android_webview/browser/safe_browsing/aw_url_checker_delegate_impl.h

@@ -70,6 +70,7 @@ class AwUrlCheckerDelegateImpl : public safe_browsing::UrlCheckerDelegate {
   static void DoApplicationResponse(
       scoped_refptr<AwSafeBrowsingUIManager> ui_manager,
       const security_interstitials::UnsafeResource& resource,
+      const AwWebResourceRequest& request,
       SafeBrowsingAction action,
       bool reporting);
 

android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java

@@ -305,7 +305,8 @@ public class AwContentsClientBridge {
             String url, boolean isMainFrame, boolean hasUserGesture, boolean isRendererInitiated,
             String method, String[] requestHeaderNames, String[] requestHeaderValues,
             // WebResourceError
-            @NetError int errorCode, String description, boolean safebrowsingHit) {
+            @NetError int errorCode, String description, boolean safebrowsingHit,
+            boolean shouldOmitNotificationsForSafeBrowsingHit) {
         AwContentsClient.AwWebResourceRequest request = new AwContentsClient.AwWebResourceRequest(
                 url, isMainFrame, hasUserGesture, method, requestHeaderNames, requestHeaderValues);
         AwContentsClient.AwWebResourceError error = new AwContentsClient.AwWebResourceError();
@@ -325,7 +326,16 @@ public class AwContentsClientBridge {
             // Android WebView does not notify the embedder of these situations using
             // this error code with the WebViewClient.onReceivedError callback.
             if (safebrowsingHit) {
-                error.errorCode = ErrorCodeConversionHelper.ERROR_UNSAFE_RESOURCE;
+                if (shouldOmitNotificationsForSafeBrowsingHit
+                        && AwFeatureList.isEnabled(
+                                AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+                    // With committed interstitials we don't fire these notifications when the
+                    // interstitial shows, we instead handle them once the interstitial is
+                    // dismissed.
+                    return;
+                } else {
+                    error.errorCode = ErrorCodeConversionHelper.ERROR_UNSAFE_RESOURCE;
+                }
             } else {
                 error.errorCode = ErrorCodeConversionHelper.convertErrorCode(error.errorCode);
             }

android_webview/java/src/org/chromium/android_webview/AwFeatures.java

@@ -11,6 +11,9 @@ package org.chromium.android_webview;
  */
 public final class AwFeatures {
     // Alphabetical:
+    public static final String SAFE_BROWSING_COMMITTED_INTERSTITIALS =
+            "SafeBrowsingCommittedInterstitials";
+
     public static final String WEBVIEW_CONNECTIONLESS_SAFE_BROWSING =
             "WebViewConnectionlessSafeBrowsing";
 

android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java

@@ -30,6 +30,8 @@ import org.chromium.android_webview.AwContents.InternalAccessDelegate;
 import org.chromium.android_webview.AwContents.NativeDrawFunctorFactory;
 import org.chromium.android_webview.AwContentsClient;
 import org.chromium.android_webview.AwContentsStatics;
+import org.chromium.android_webview.AwFeatureList;
+import org.chromium.android_webview.AwFeatures;
 import org.chromium.android_webview.AwSettings;
 import org.chromium.android_webview.AwWebContentsObserver;
 import org.chromium.android_webview.ErrorCodeConversionHelper;
@@ -437,7 +439,13 @@ public class SafeBrowsingTest {
                 mWebContentsObserver.getAttachedInterstitialPageHelper().getCallCount();
         final String responseUrl = mTestServer.getURL(path);
         mActivityTestRule.loadUrlAsync(mAwContents, responseUrl);
-        mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            // Wait for the interstitial to actually render.
+            mActivityTestRule.waitForVisualStateCallback(mAwContents);
+        } else {
+            mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
     }
 
     private void assertTargetPageHasLoaded(int pageColor) throws Exception {
@@ -647,8 +655,8 @@ public class SafeBrowsingTest {
         loadPathAndWaitForInterstitial(MALWARE_HTML_PATH);
         assertGreenPageNotShowing();
         assertTargetPageNotShowing(MALWARE_PAGE_BACKGROUND_COLOR);
-        // Assume that we are rendering the interstitial, since we see neither the previous page nor
-        // the target page
+        // Assume that we are rendering the interstitial, since we see neither the previous page
+        // nor the target page
     }
 
     @Test
@@ -659,9 +667,8 @@ public class SafeBrowsingTest {
         loadPathAndWaitForInterstitial(IFRAME_HTML_PATH);
         assertGreenPageNotShowing();
         assertTargetPageNotShowing(IFRAME_EMBEDDER_BACKGROUND_COLOR);
-
-        // Assume that we are rendering the interstitial, since we see neither the previous page nor
-        // the target page
+        // Assume that we are rendering the interstitial, since we see neither the previous page
+        // nor the target page
     }
 
     @Test
@@ -713,11 +720,17 @@ public class SafeBrowsingTest {
         loadGreenPage();
         loadPathAndWaitForInterstitial(MALWARE_HTML_PATH);
         waitForInterstitialDomToLoad();
-
         int interstitialCount =
                 mWebContentsObserver.getDetachedInterstitialPageHelper().getCallCount();
+        OnReceivedError2Helper errorHelper = mContentsClient.getOnReceivedError2Helper();
+        int errorCount = errorHelper.getCallCount();
         clickBackToSafety();
-        mWebContentsObserver.getDetachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            errorHelper.waitForCallback(errorCount);
+        } else {
+            mWebContentsObserver.getDetachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
 
         mActivityTestRule.waitForVisualStateCallback(mAwContents);
         assertTargetPageNotShowing(MALWARE_PAGE_BACKGROUND_COLOR);
@@ -731,11 +744,17 @@ public class SafeBrowsingTest {
         loadGreenPage();
         loadPathAndWaitForInterstitial(IFRAME_HTML_PATH);
         waitForInterstitialDomToLoad();
-
         int interstitialCount =
                 mWebContentsObserver.getDetachedInterstitialPageHelper().getCallCount();
+        OnReceivedError2Helper errorHelper = mContentsClient.getOnReceivedError2Helper();
+        int errorCount = errorHelper.getCallCount();
         clickBackToSafety();
-        mWebContentsObserver.getDetachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            errorHelper.waitForCallback(errorCount);
+        } else {
+            mWebContentsObserver.getDetachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
 
         mActivityTestRule.waitForVisualStateCallback(mAwContents);
         assertTargetPageNotShowing(IFRAME_EMBEDDER_BACKGROUND_COLOR);
@@ -1070,7 +1089,13 @@ public class SafeBrowsingTest {
         int interstitialCount =
                 mWebContentsObserver.getAttachedInterstitialPageHelper().getCallCount();
         mActivityTestRule.loadUrlAsync(mAwContents, WEB_UI_MALWARE_URL);
-        mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            // Wait for the interstitial to actually render.
+            mActivityTestRule.waitForVisualStateCallback(mAwContents);
+        } else {
+            mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
         waitForInterstitialDomToLoad();
     }
 
@@ -1082,7 +1107,13 @@ public class SafeBrowsingTest {
         int interstitialCount =
                 mWebContentsObserver.getAttachedInterstitialPageHelper().getCallCount();
         mActivityTestRule.loadUrlAsync(mAwContents, WEB_UI_PHISHING_URL);
-        mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            // Wait for the interstitial to actually render.
+            mActivityTestRule.waitForVisualStateCallback(mAwContents);
+        } else {
+            mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
         waitForInterstitialDomToLoad();
     }
 
@@ -1095,7 +1126,13 @@ public class SafeBrowsingTest {
         int interstitialCount =
                 mWebContentsObserver.getAttachedInterstitialPageHelper().getCallCount();
         mActivityTestRule.loadUrlAsync(mAwContents, WEB_UI_MALWARE_URL);
-        mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(interstitialCount);
+        if (AwFeatureList.isEnabled(AwFeatures.SAFE_BROWSING_COMMITTED_INTERSTITIALS)) {
+            // Wait for the interstitial to actually render.
+            mActivityTestRule.waitForVisualStateCallback(mAwContents);
+        } else {
+            mWebContentsObserver.getAttachedInterstitialPageHelper().waitForCallback(
+                    interstitialCount);
+        }
         waitForInterstitialDomToLoad();
     }
 

components/security_interstitials/content/security_interstitial_tab_helper.cc

@@ -72,6 +72,12 @@ bool SecurityInterstitialTabHelper::IsDisplayingInterstitial() const {
   return blocking_page_for_currently_committed_navigation_ != nullptr;
 }
 
+bool SecurityInterstitialTabHelper::IsInterstitialPendingForNavigation(
+    int64_t navigation_id) const {
+  return blocking_pages_for_navigations_.find(navigation_id) !=
+         blocking_pages_for_navigations_.end();
+}
+
 security_interstitials::SecurityInterstitialPage*
 SecurityInterstitialTabHelper::
     GetBlockingPageForCurrentlyCommittedNavigationForTesting() {

components/security_interstitials/content/security_interstitial_tab_helper.h

@@ -49,6 +49,11 @@ class SecurityInterstitialTabHelper
   // Whether this tab helper is tracking a currently-displaying interstitial.
   bool IsDisplayingInterstitial() const;
 
+  // Whether an interstitial has been associated for |navigation_id|, but hasn't
+  // committed yet. For checking if the interstitial has committed use
+  // IsDisplayingInterstitial.
+  bool IsInterstitialPendingForNavigation(int64_t navigation_id) const;
+
   security_interstitials::SecurityInterstitialPage*
   GetBlockingPageForCurrentlyCommittedNavigationForTesting();