Do not call OnStartSlowCheck when performing a full URL check.

1. Notifier::OnStartSlowCheck(), PauseReadingBodyFromNet(), and
   ResumeReadingBodyFromNet() were added with the assumption that
   Safe Browsing checks mostly return synchronously (except a tiny
   percentage) so calling PauseReadingBodyFromNet() in only those
   few cases was a good precautionary measure.
2. The "precaution" was against overzealous AVs that may warn the user
   if we cache an unsafe resource; note that the resource is not used
   until Safe Browsing confirms it is SAFE.
3. On mobile, since connection is usually slower, we did not enable
   this "precaution". That's a very good indication of how good this
   "precaution" is :)
4. Now, with SafeBrowsingRealTimeUrlLookup enabled, a larger percentage
   of navigations are going to hit PauseReadingBodyFromNet().
5. This would have been fine by itself but due to the special handling
   of PDFs and some other special types, as described in 1056696#c9,
   this completely breaks the loading of those types.
6. The "precaution" does not seem worth the breakage.

See https://crbug.com/1056696#c9 and https://crbug.com/1056696#c12
for full details.

R=xinghuilu

Bug: 1054978,1056696
Change-Id: I0f6f48831733996e0ab91a0c0c1ab240a1923c21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079338
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Auto-Submit: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745264}

components/safe_browsing/content/browser/browser_url_loader_throttle.cc

@@ -323,6 +323,9 @@ void BrowserURLLoaderThrottle::NotifySlowCheck() {
   // processing unsafe contents (e.g., writing unsafe contents into cache),
   // until we get the results. According to the results, we may resume reading
   // or cancel the resource load.
+  // For real time Safe Browsing checks, we continue reading the response body
+  // but, similar to hash-based checks, do not process it until we know it is
+  // SAFE.
   if (pending_slow_checks_ == 1)
     delegate_->PauseReadingBodyFromNet();
 }

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.cc

@@ -306,7 +306,8 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
                  &SafeBrowsingUrlCheckerImpl::OnTimeout);
 
     bool safe_synchronously;
-    if (CanPerformFullURLLookup(url)) {
+    bool can_perform_full_url_lookup = CanPerformFullURLLookup(url);
+    if (can_perform_full_url_lookup) {
       UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.RT.ResourceTypes.Checked",
                                 resource_type_);
       safe_synchronously = false;
@@ -364,10 +365,16 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
 
     // Only send out notification of starting a slow check if the database
     // manager actually supports fast checks (i.e., synchronous checks) but is
-    // not able to complete the check synchronously in this case.
+    // not able to complete the check synchronously in this case and we're doing
+    // hash-based checks.
     // Don't send out notification if the database manager doesn't support
-    // synchronous checks at all (e.g., on mobile).
-    if (!database_manager_->ChecksAreAlwaysAsync())
+    // synchronous checks at all (e.g., on mobile), or if performing a full URL
+    // check since we don't want to block resource fetch while we perform a full
+    // URL lookup. Note that we won't parse the response until the Safe Browsing
+    // check is complete and return SAFE, so there's no Safe Browsing bypass
+    // risk here.
+    if (!can_perform_full_url_lookup &&
+        !database_manager_->ChecksAreAlwaysAsync())
       urls_[next_index_].notifier.OnStartSlowCheck();
 
     break;