[Offline Pages] Allow same-page snapshots for POST requests.

Previously we disabled POST requests from being snapshotted by OPC.
However, this inadvertently prevented SnapshotController from marking
a WebContents as snapshottable, so any POST requests would be unable to
complete - they would hang in downloading state until Chrome exited.
After that, the background loader would take over and error out because
it does not support non-GET requests and would use a GET HTTP request
for a POST resource which typically fails or redirects to a login page
of some kind.

Additionally, extends NavigationSimulator to use the method from
SetMethod when committing navigations, rather than the currently-hardcoded
"GET" method.

Bug: 859403
Change-Id: Ic10dd0b088da9e3ebf2a42e3cca6096eea449d16
Reviewed-on: https://chromium-review.googlesource.com/1125031Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573516}

chrome/browser/offline_pages/recent_tab_helper.cc

@@ -243,20 +243,18 @@ void RecentTabHelper::DidFinishNavigation(
   // Always reset so that posted tasks get canceled.
   snapshot_controller_->Reset();
 
-  // Check for conditions that should stop last_n from creating snapshots of
+  // Check for conditions that should stop us from creating snapshots of
   // this page:
   // - It is an error page.
-  // - The navigation is a POST as offline pages are never loaded for them.
   // - The navigated URL is not supported.
   // - The page being loaded is already an offline page.
   bool can_save =
-      !navigation_handle->IsErrorPage() && !navigation_handle->IsPost() &&
+      !navigation_handle->IsErrorPage() &&
       OfflinePageModel::CanSaveURL(web_contents()->GetLastCommittedURL()) &&
       current_offline_page == nullptr;
   DVLOG_IF(1, !can_save)
       << " - Page can not be saved for offline usage (reasons: "
       << !navigation_handle->IsErrorPage() << ", "
-      << !navigation_handle->IsPost() << ", "
       << OfflinePageModel::CanSaveURL(web_contents()->GetLastCommittedURL())
       << ", " << (current_offline_page == nullptr) << ")";
 
@@ -264,7 +262,12 @@ void RecentTabHelper::DidFinishNavigation(
 
   if (!can_save)
     snapshot_controller_->Stop();
+  // Last N should be disabled when:
+  // - Running on low end devices.
+  // - Viewing POST content for privacy considerations.
+  // - Disabled by flag.
   last_n_listen_to_tab_hidden_ = can_save && !delegate_->IsLowEndDevice() &&
+                                 !navigation_handle->IsPost() &&
                                  IsOffliningRecentPagesEnabled();
   DVLOG_IF(1, can_save && !last_n_listen_to_tab_hidden_)
       << " - Page can not be saved by last_n";

chrome/browser/offline_pages/recent_tab_helper_unittest.cc

@@ -105,6 +105,9 @@ class RecentTabHelperTest
   // navigation.
   void NavigateAndCommitTyped(const GURL& url);
 
+  // Navigates to the URL and commit as if a form had been submitted.
+  void NavigateAndCommitPost(const GURL& url);
+
   ClientId NewDownloadClientId();
 
   RecentTabHelper* recent_tab_helper() const { return recent_tab_helper_; }
@@ -272,6 +275,15 @@ void RecentTabHelperTest::NavigateAndCommitTyped(const GURL& url) {
   simulator->Commit();
 }
 
+void RecentTabHelperTest::NavigateAndCommitPost(const GURL& url) {
+  auto simulator =
+      content::NavigationSimulator::CreateRendererInitiated(url, main_rfh());
+  simulator->SetMethod("POST");
+  simulator->SetTransition(ui::PAGE_TRANSITION_FORM_SUBMIT);
+  simulator->Start();
+  simulator->Commit();
+}
+
 ClientId RecentTabHelperTest::NewDownloadClientId() {
   static int counter = 0;
   return ClientId(kDownloadNamespace,
@@ -1089,4 +1101,33 @@ TEST_F(RecentTabHelperTest, NoSaveIfTabIsClosing) {
                                          IsSavingSamePageEnum::kNewPage, 1);
 }
 
+TEST_F(RecentTabHelperTest, NoSaveOfflinePageCacheForPost) {
+  // Navigate and finish loading. Nothing should be saved, but we should record
+  // that it is possible to save the page.
+  NavigateAndCommitPost(kTestPageUrl);
+  histogram_tester()->ExpectUniqueSample("OfflinePages.CanSaveRecentPage", true,
+                                         1);
+
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  // Move the snapshot controller's time forward so it gets past timeouts.
+  FastForwardSnapshotController();
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // Tab is hidden with a fully loaded page. A snapshot save should not happen
+  // due to the POST method - OfflinePageCache is disabled.
+  recent_tab_helper()->OnVisibilityChanged(content::Visibility::HIDDEN);
+  RunUntilIdle();
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
+  histogram_tester()->ExpectTotalCount("OfflinePages.LastN.IsSavingSamePage",
+                                       0);
+
+  // A manual download should succeed despite being ineligible for OPC.
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L, "");
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+}
+
 }  // namespace offline_pages

content/public/test/navigation_simulator.cc

@@ -512,7 +512,7 @@ void NavigationSimulator::Commit() {
   params.gesture =
       has_user_gesture_ ? NavigationGestureUser : NavigationGestureAuto;
   params.contents_mime_type = contents_mime_type_;
-  params.method = "GET";
+  params.method = handle_->IsPost() ? "POST" : "GET";
   params.http_status_code = 200;
   params.history_list_was_cleared = false;
   params.original_request_url = navigation_url_;

content/test/navigation_simulator_unittest.cc

@@ -134,6 +134,36 @@ class CancellingNavigationSimulatorTest
   DISALLOW_COPY_AND_ASSIGN(CancellingNavigationSimulatorTest);
 };
 
+class MethodCheckingNavigationSimulatorTest : public NavigationSimulatorTest,
+                                              public WebContentsObserver {
+ public:
+  MethodCheckingNavigationSimulatorTest() = default;
+  ~MethodCheckingNavigationSimulatorTest() override = default;
+
+  void SetUp() override {
+    RenderViewHostImplTestHarness::SetUp();
+    contents()->GetMainFrame()->InitializeRenderFrameIfNeeded();
+    Observe(RenderViewHostImplTestHarness::web_contents());
+  }
+
+  void DidFinishNavigation(content::NavigationHandle* handle) override {
+    did_finish_navigation_ = true;
+    is_post_ = handle->IsPost();
+  }
+
+  bool did_finish_navigation() { return did_finish_navigation_; }
+  bool is_post() { return is_post_; }
+
+ private:
+  // set upon DidFinishNavigation.
+  bool did_finish_navigation_ = false;
+
+  // Not valid until |did_finish_navigation_| is true;
+  bool is_post_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(MethodCheckingNavigationSimulatorTest);
+};
+
 TEST_F(NavigationSimulatorTest, AutoAdvanceOff) {
   std::unique_ptr<NavigationSimulator> simulator =
       NavigationSimulator::CreateRendererInitiated(
@@ -166,6 +196,32 @@ TEST_F(NavigationSimulatorTest, AutoAdvanceOff) {
   simulator->Commit();
 }
 
+TEST_F(MethodCheckingNavigationSimulatorTest, SetMethodPostWithRedirect) {
+  std::unique_ptr<NavigationSimulator> simulator =
+      NavigationSimulator::CreateRendererInitiated(
+          GURL("https://example.test/"), main_rfh());
+  simulator->SetMethod("POST");
+  simulator->Start();
+  simulator->Redirect(GURL("https://example.test/2.html"));
+  simulator->Commit();
+
+  ASSERT_TRUE(did_finish_navigation());
+  EXPECT_FALSE(is_post())
+      << "If a POST request redirects, it should convert to a GET";
+}
+
+TEST_F(MethodCheckingNavigationSimulatorTest, SetMethodPost) {
+  std::unique_ptr<NavigationSimulator> simulator =
+      NavigationSimulator::CreateRendererInitiated(
+          GURL("https://example.test/"), main_rfh());
+  simulator->SetMethod("POST");
+  simulator->Start();
+  simulator->Commit();
+
+  ASSERT_TRUE(did_finish_navigation());
+  EXPECT_TRUE(is_post());
+}
+
 // Stress test the navigation simulator by having a navigation throttle cancel
 // the navigation at various points in the flow, both synchronously and
 // asynchronously.