Override cache policy when changing a reload to a back/forward navigation.

In some cases, the RenderView performs a history navigation in response to a
reload when the WebFrame does not have the page in question committed. Override
the cache policy so the navigation revalidates the cache, rather than skipping
revalidation altogether.

BUG=331386
TEST=CrashRecoveryBrowserTest.ReloadCacheRevalidate

Review URL: https://codereview.chromium.org/101203007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243796 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/crash_recovery_browsertest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/files/file_path.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -13,6 +14,9 @@
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_transition_types.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::NavigationController;
@@ -31,6 +35,32 @@ void SimulateRendererCrash(Browser* browser) {
   observer.Wait();
 }
 
+// A request handler which returns a different result each time but stays fresh
+// into the far future.
+class CacheMaxAgeHandler {
+ public:
+  explicit CacheMaxAgeHandler(const std::string& path)
+      : path_(path), request_count_(0) { }
+
+  scoped_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request) {
+    if (request.relative_url != path_)
+      return scoped_ptr<net::test_server::HttpResponse>();
+
+    request_count_++;
+    scoped_ptr<net::test_server::BasicHttpResponse> response(
+        new net::test_server::BasicHttpResponse);
+    response->set_content(base::StringPrintf("<title>%d</title>",
+                                             request_count_));
+    response->set_content_type("text/html");
+    response->AddCustomHeader("Cache-Control", "max-age=99999");
+    return response.PassAs<net::test_server::HttpResponse>();
+  }
+ private:
+  std::string path_;
+  int request_count_;
+};
+
 }  // namespace
 
 class CrashRecoveryBrowserTest : public InProcessBrowserTest {
@@ -62,6 +92,37 @@ IN_PROC_BROWSER_TEST_F(CrashRecoveryBrowserTest, Reload) {
   EXPECT_NE(title_before_crash, title_after_crash);
 }
 
+// Test that reload after a crash forces a cache revalidation.
+IN_PROC_BROWSER_TEST_F(CrashRecoveryBrowserTest, ReloadCacheRevalidate) {
+  const char kTestPath[] = "/test";
+
+  // Use the test server so as not to bypass cache behavior. The title of the
+  // active tab should change only when this URL is reloaded.
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  embedded_test_server()->RegisterRequestHandler(
+      base::Bind(&CacheMaxAgeHandler::HandleRequest,
+                 base::Owned(new CacheMaxAgeHandler(kTestPath))));
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL(kTestPath));
+
+  base::string16 title_before_crash;
+  base::string16 title_after_crash;
+
+  ASSERT_TRUE(ui_test_utils::GetCurrentTabTitle(browser(),
+                                                &title_before_crash));
+  SimulateRendererCrash(browser());
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<NavigationController>(
+          &browser()->tab_strip_model()->GetActiveWebContents()->
+              GetController()));
+  chrome::Reload(browser(), CURRENT_TAB);
+  observer.Wait();
+  ASSERT_TRUE(ui_test_utils::GetCurrentTabTitle(browser(),
+                                                &title_after_crash));
+  EXPECT_NE(title_before_crash, title_after_crash);
+}
+
 // Tests that loading a crashed page in a new tab correctly updates the title.
 // There was an earlier bug (1270510) in process-per-site in which the max page
 // ID of the RenderProcessHost was stale, so the NavigationEntry in the new tab

content/renderer/render_view_impl.cc

@@ -1361,6 +1361,8 @@ void RenderViewImpl::OnNavigate(const ViewMsg_Navigate_Params& params) {
   FOR_EACH_OBSERVER(RenderViewObserver, observers_, Navigate(params.url));
 
   bool is_reload = IsReload(params);
+  WebURLRequest::CachePolicy cache_policy =
+      WebURLRequest::UseProtocolCachePolicy;
 
   // If this is a stale back/forward (due to a recent navigation the browser
   // didn't know about), ignore it.
@@ -1378,6 +1380,7 @@ void RenderViewImpl::OnNavigate(const ViewMsg_Navigate_Params& params) {
     // params.state.  Setting is_reload to false will treat this like a back
     // navigation to accomplish that.
     is_reload = false;
+    cache_policy = WebURLRequest::ReloadIgnoringCacheData;
 
     // We refresh timezone when a view is swapped in since timezone
     // can get out of sync when the system timezone is updated while
@@ -1411,10 +1414,9 @@ void RenderViewImpl::OnNavigate(const ViewMsg_Navigate_Params& params) {
 
   if (is_reload && frame->currentHistoryItem().isNull()) {
     // We cannot reload if we do not have any history state.  This happens, for
-    // example, when recovering from a crash.  Our workaround here is a bit of
-    // a hack since it means that reload after a crashed tab does not cause an
-    // end-to-end cache validation.
+    // example, when recovering from a crash.
     is_reload = false;
+    cache_policy = WebURLRequest::ReloadIgnoringCacheData;
   }
 
   pending_navigation_params_.reset(new ViewMsg_Navigate_Params(params));
@@ -1442,7 +1444,7 @@ void RenderViewImpl::OnNavigate(const ViewMsg_Navigate_Params& params) {
       // Ensure we didn't save the swapped out URL in UpdateState, since the
       // browser should never be telling us to navigate to swappedout://.
       CHECK(item.urlString() != WebString::fromUTF8(kSwappedOutURL));
-      frame->loadHistoryItem(item);
+      frame->loadHistoryItem(item, cache_policy);
     }
   } else if (!params.base_url_for_data_url.is_empty()) {
     // A loadData request with a specified base URL.
@@ -3461,6 +3463,11 @@ void RenderViewImpl::PopulateDocumentStateFromPending(
     // can result in stale data for pages that are set to expire. We explicitly
     // override that by setting the policy here so that as necessary we load
     // from the network.
+    //
+    // TODO(davidben): Remove this in favor of passing a cache policy to the
+    // loadHistoryItem call in OnNavigate. That requires not overloading
+    // UseProtocolCachePolicy to mean both "normal load" and "determine cache
+    // policy based on load type, etc".
     internal_data->set_cache_policy_override(
         WebURLRequest::UseProtocolCachePolicy);
   }