Plumb network stack information about existence of cached copy

chrome/browser/errorpage_browsertest.cc

Index: chrome/browser/errorpage_browsertest.cc
diff --git a/chrome/browser/errorpage_browsertest.cc b/chrome/browser/errorpage_browsertest.cc
index e2afcbf59b7c601ef1b81187426d68a6d2e86b55..12b27e7d6d8eb51babefcd029cebd95852e7566e 100644
--- a/chrome/browser/errorpage_browsertest.cc
+++ b/chrome/browser/errorpage_browsertest.cc
@@ -6,6 +6,8 @@
 #include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browsing_data/browsing_data_helper.h"
+#include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -25,6 +27,11 @@
 #include "content/test/net/url_request_mock_http_job.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
+#include "net/http/failing_http_transaction_factory.h"
+#include "net/http/http_cache.h"
+#include "net/test/spawned_test_server/spawned_test_server.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_job_factory.h"
 
@@ -121,7 +128,6 @@ class ErrorPageTest : public InProcessBrowserTest {
   }
 };
 
-
 class TestFailProvisionalLoadObserver : public content::WebContentsObserver {
  public:
   explicit TestFailProvisionalLoadObserver(content::WebContents* contents)
@@ -148,6 +154,20 @@ class TestFailProvisionalLoadObserver : public content::WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(TestFailProvisionalLoadObserver);
 };
 
+void InterceptNetworkTransactions(net::URLRequestContextGetter* getter,
+                                  net::Error error) {
+  DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::IO));
+  net::HttpCache* cache(
+      getter->GetURLRequestContext()->http_transaction_factory()->GetCache());
+  DCHECK(cache);
+  scoped_ptr<net::FailingHttpTransactionFactory> factory(

[mmenke, 2014/01/28 17:27:23]

nit:  Can just make this a HttpTransactionFactory, and then pass with
factory.Pass().

[Randy Smith (Not in Mondays), 2014/01/28 22:52:59]

Done.

+      new net::FailingHttpTransactionFactory(cache->GetSession(), error));
+  // Throw away old version; since we're a browser test, we don't
+  // need to restore the old state.

[mmenke, 2014/01/28 17:27:23]

nit:  Don't use "we" in comments.

[Randy Smith (Not in Mondays), 2014/01/28 22:52:59]

Done.

+  cache->SetHttpNetworkTransactionFactoryForTesting(
+      factory.PassAs<net::HttpTransactionFactory>());
+}
+
 // See crbug.com/109669
 #if defined(USE_AURA) || defined(OS_WIN)
 #define MAYBE_DNSError_Basic DISABLED_DNSError_Basic
@@ -356,6 +376,70 @@ IN_PROC_BROWSER_TEST_F(ErrorPageTest, Page404) {
       1);
 }
 
+// Checks that when an error occurs, the stale cache status of the page
+// is correctly transferred.
+IN_PROC_BROWSER_TEST_F(ErrorPageTest, StaleCacheStatus) {
+  ASSERT_TRUE(test_server()->Start());
+  // Load cache with entry with "nocache" set, to create stale
+  // cache.
+  GURL test_url(test_server()->GetURL("files/nocache.html"));
+  NavigateToURLAndWaitForTitle(test_url, "Nocache Test Page", 1);
+
+  // Reload same URL after forcing an error from the the network layer;
+  // confirm that the error page is told the cached copy exists.
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
+      browser()->profile()->GetRequestContext();
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&InterceptNetworkTransactions, url_request_context_getter,
+                 // Note that we can't use an error that'll invoke the link
+                 // doctor.  In normal network error conditions that would
+                 // work (because the link doctor fetch would also fail,
+                 // putting us back in the main offline path), but
+                 // SetUrlRequestMocksEnabled() has also specfied a link
+                 // doctor mock, which will be accessible because it
+                 // won't go through the network cache.
+                 net::ERR_FAILED));
+
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      // With no link doctor load, there's only one navigation.
+      browser(), test_url, 1);
+
+  content::WebContents* wc =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  const char* js_cache_probe =
+      "(function () {\n"
+          "if ('staleCopyInCache' in templateData) {\n"
+              "return templateData.staleCopyInCache;\n"
+          "} else {\n"
+              "return 'Undefined';\n"
+          "}\n"
+      "})();";
+
+  scoped_ptr<base::Value> value =
+      content::ExecuteScriptAndGetValue(
+          wc->GetRenderViewHost(), js_cache_probe);
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_BOOLEAN)) << "Type is "
+                                                        << value->GetType();
+  bool result = false;
+  EXPECT_TRUE(value->GetAsBoolean(&result));
+  EXPECT_TRUE(result);

[mmenke, 2014/01/28 17:27:23]

optional:  It's simpler to do:

ASSERT_TRUE(content::ExecuteScriptAndExtractBool(...));
EXPECT_TRUE(result);

Though I suppose that doesn't separate out the cases where the script fails to
run, and where we don't get a bool.  I don't think I've ever seen the first
happen before, though.

[Randy Smith (Not in Mondays), 2014/01/28 22:52:59]

So it turns out that ExecuteScriptAndGetValue() and ExecuteScriptAndExtractBool
use very different methods to wait for the response to come back from the
renderer, and if I just do a straight replacement, the test hangs.  I believe
what's going on is that ExecuteScriptAndExtractBool uses a DOMOperationObserver
to wait for the response, and that the other uses of
ExecuteScriptAndExtractBool() call domAutomationController.send(), which trips
that observer.  

I'm willing to shift over to that method if you'd like, and part of me thinks I
should (for compatibility with the rest of the file) but having dug into the two
pathways a bit, I find myself preferring ExecuteScriptAndGetValue() because it
doesn't rely on DomAutomationController, only on mechanisms inside
RenderViewHostImpl (and presumably RenderView).  So I figured I'd report back
and let you make the call :-}.

[mmenke, 2014/01/28 23:10:22]

My general feeling on this is that ExecuteScriptAndExtractBool is closer to what
we want.  If there's a problem with its implementation, then rather than not use
it, we should modify it to use content::ExecuteScriptAndGetValue behind the
scenes.

In order to use it, you need to replace the "return" statements with
"chrome.send(result)".  Sorry I didn't mention that, I had just assumed they
used the same mechanism.

[Randy Smith (Not in Mondays), 2014/01/29 21:14:10]

This makes sense.  Done.  (I used ExecuteScriptAndExtractString, since I really
didn't want to lose the distinction between "variable not there" and one of the
values of the variable.)

+
+  // Clear the cache and reload the same URL; confirm the error page is told
+  // that there is no cached copy.
+  BrowsingDataRemover* remover =
+      BrowsingDataRemover::CreateForUnboundedRange(browser()->profile());
+  remover->Remove(BrowsingDataRemover::REMOVE_CACHE,
+                  BrowsingDataHelper::UNPROTECTED_WEB);
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), test_url, 1);
+  value = content::ExecuteScriptAndGetValue(
+      wc->GetRenderViewHost(), js_cache_probe);
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_BOOLEAN));
+  EXPECT_TRUE(value->GetAsBoolean(&result));
+  EXPECT_FALSE(result);

[mmenke, 2014/01/28 17:27:23]

Suggest making this into a function.  4 copies seems a bit much.

[Randy Smith (Not in Mondays), 2014/01/28 22:52:59]

Done.

+}
+
 // Returns Javascript code that executes plain text search for the page.
 // Pass into content::ExecuteScriptAndExtractBool as |script| parameter.
 std::string GetTextContentContainsStringScript(
@@ -391,7 +475,7 @@ class AddressUnreachableProtocolHandler
 // requests.  ERR_NAME_NOT_RESOLVED is more typical, but need to use a different
 // error for the Link Doctor and the original page to validate the right page
 // is being displayed.
-class ErrorPageLinkDoctorFailTest : public InProcessBrowserTest {
+class ErrorPageLinkDoctorFailTest : public ErrorPageTest {
  public:
   // InProcessBrowserTest:
   virtual void SetUpOnMainThread() OVERRIDE {
@@ -447,6 +531,65 @@ IN_PROC_BROWSER_TEST_F(ErrorPageLinkDoctorFailTest, LinkDoctorFail) {
   EXPECT_TRUE(result);
 }
 
+// Checks that when an error occurs and a link doctor load fails, the stale
+// cache status of the page is correctly transferred.  Most logic copied
+// from StaleCacheStatus above.
+IN_PROC_BROWSER_TEST_F(ErrorPageLinkDoctorFailTest,
+                       StaleCacheStatusFailedLinkDoctor) {
+  ASSERT_TRUE(test_server()->Start());
+  // Load cache with entry with "nocache" set, to create stale
+  // cache.
+  GURL test_url(test_server()->GetURL("files/nocache.html"));
+  NavigateToURLAndWaitForTitle(test_url, "Nocache Test Page", 1);
+
+  // Reload same URL after forcing an error from the the network layer;
+  // confirm that the error page is told the cached copy exists.
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
+      browser()->profile()->GetRequestContext();
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&InterceptNetworkTransactions, url_request_context_getter,
+                 net::ERR_CONNECTION_FAILED));
+
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      // With no link doctor load, there's only one navigation.
+      browser(), test_url, 1);

[mmenke, 2014/01/28 17:27:23]

If Link Doctor is failing properly, this should be 2.

[Randy Smith (Not in Mondays), 2014/01/28 22:52:59]

Done.

+
+  content::WebContents* wc =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  const char* js_cache_probe =
+      "(function () {\n"
+          "if ('staleCopyInCache' in templateData) {\n"
+              "return templateData.staleCopyInCache;\n"
+          "} else {\n"
+              "return 'Undefined';\n"
+          "}\n"
+      "})();";
+
+  scoped_ptr<base::Value> value =
+      content::ExecuteScriptAndGetValue(
+          wc->GetRenderViewHost(), js_cache_probe);
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_BOOLEAN)) << "Type is "
+                                                        << value->GetType();
+  bool result = false;
+  EXPECT_TRUE(value->GetAsBoolean(&result));
+  EXPECT_TRUE(result);
+
+  // Clear the cache and reload the same URL; confirm the error page is told
+  // that there is no cached copy.
+  BrowsingDataRemover* remover =
+      BrowsingDataRemover::CreateForUnboundedRange(browser()->profile());
+  remover->Remove(BrowsingDataRemover::REMOVE_CACHE,
+                  BrowsingDataHelper::UNPROTECTED_WEB);
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), test_url, 1);
+  value = content::ExecuteScriptAndGetValue(
+      wc->GetRenderViewHost(), js_cache_probe);
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_BOOLEAN));
+  EXPECT_TRUE(value->GetAsBoolean(&result));
+  EXPECT_FALSE(result);
+}
+
 // A test fixture that simulates failing requests for an IDN domain name.
 class ErrorPageForIDNTest : public InProcessBrowserTest {
  public: