[iOS] Fix logic for triggering Safe Browsing warnings for subframes

Error pages for unsafe subframes are triggered by associating
an UnsafeResource with the corresponding main frame item and
reloading that item. This reload is detected in
SafeBrowsingTabHelper::ShouldAllowRequest and an error
decision is created.

This detection logic was incorrectly also applied to
subframe reloads, triggering a crash since these can happen
while both the pending item and last committed item
are null. More specifically, this can happen when a subframe
triggers a reload using JavaScript, at the same time as the
user performs a back/forward navigation to a restore_session
URL.

This CL fixes this logic by only applying it to main frame
requests.

Bug: 1124308
Change-Id: Idc485b836e88c41116f16b5b42a964585666d9cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392398Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804282}

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper.mm

@@ -213,22 +213,25 @@ SafeBrowsingTabHelper::PolicyDecider::ShouldAllowRequest(
       pending_main_frame_query_->decision = CreateSafeBrowsingErrorDecision();
       return web::WebStatePolicyDecider::PolicyDecision::Allow();
     }
-  }
 
-  // Check to see if an error page should be displayed for a sub frame resource.
-  web::NavigationManager* navigation_manager =
-      web_state()->GetNavigationManager();
-  web::NavigationItem* reloaded_item = navigation_manager->GetPendingItem();
-  if (ui::PageTransitionCoreTypeIs(request_info.transition_type,
-                                   ui::PAGE_TRANSITION_RELOAD) &&
-      reloaded_item == navigation_manager->GetLastCommittedItem() &&
-      unsafe_resource_container->GetSubFrameUnsafeResource(reloaded_item)) {
-    // Store the safe browsing error decision without re-checking the URL.
-    // TODO(crbug.com/1064803): This should directly return the safe browsing
-    // error decision once error pages for cancelled requests are supported.
-    // For now, only cancelled response errors are displayed properly.
-    pending_main_frame_query_->decision = CreateSafeBrowsingErrorDecision();
-    return web::WebStatePolicyDecider::PolicyDecision::Allow();
+    // Error pages for unsafe subframes are triggered by associating an
+    // UnsafeResource with the corresponding main frame item and reloading that
+    // item. Check to see if this main frame request is a reload and has an
+    // associated sub frame UnsafeResource.
+    web::NavigationManager* navigation_manager =
+        web_state()->GetNavigationManager();
+    web::NavigationItem* reloaded_item = navigation_manager->GetPendingItem();
+    if (ui::PageTransitionCoreTypeIs(request_info.transition_type,
+                                     ui::PAGE_TRANSITION_RELOAD) &&
+        reloaded_item == navigation_manager->GetLastCommittedItem() &&
+        unsafe_resource_container->GetSubFrameUnsafeResource(reloaded_item)) {
+      // Store the safe browsing error decision without re-checking the URL.
+      // TODO(crbug.com/1064803): This should directly return the safe browsing
+      // error decision once error pages for cancelled requests are supported.
+      // For now, only cancelled response errors are displayed properly.
+      pending_main_frame_query_->decision = CreateSafeBrowsingErrorDecision();
+      return web::WebStatePolicyDecider::PolicyDecision::Allow();
+    }
   }
 
   // Create the URL checker and check for navigation safety on the IO thread.

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper_unittest.mm

@@ -540,6 +540,34 @@ TEST_P(SafeBrowsingTabHelperTest,
   EXPECT_EQ(kUnsafeResourceErrorCode, error.code);
 }
 
+// Tests the case of a subframe reload request that arrives when both the last
+// committed item and pending items are null, which happens in practice during
+// a back/forward navigation to a restore_session URL.
+TEST_P(SafeBrowsingTabHelperTest, StaleIframeReload) {
+  GURL unsafe_url("http://" + FakeSafeBrowsingService::kUnsafeHost);
+
+  // Ensure that the navigation manager's state is consistent with what happens
+  // during a back/forward navigation to a restore_session URL.
+  ASSERT_FALSE(navigation_manager_->GetLastCommittedItem());
+  ASSERT_FALSE(navigation_manager_->GetPendingItem());
+
+  auto request_decision =
+      ShouldAllowRequestUrl(unsafe_url, /*for_main_frame=*/false,
+                            ui::PageTransition::PAGE_TRANSITION_RELOAD);
+  EXPECT_TRUE(request_decision.ShouldAllowNavigation());
+
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  // Ensure that a subsequent main frame request is handled correctly.
+  GURL safe_url("http://chromium.test");
+  EXPECT_TRUE(ShouldAllowRequestUrl(safe_url).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  auto response_decision = ShouldAllowResponseUrl(safe_url);
+  EXPECT_TRUE(response_decision.ShouldAllowNavigation());
+}
+
 // Tests the case of a redirection chain, where all URLs in the chain are safe.
 TEST_P(SafeBrowsingTabHelperTest, SafeRedirectChain) {
   GURL url1("http://chromium1.test");