Serve offline page for online URL on disconnected or bad networks

chrome/browser/android/offline_pages/offline_page_request_handler.cc

Index: chrome/browser/android/offline_pages/offline_page_request_handler.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_request_handler.cc b/chrome/browser/android/offline_pages/offline_page_request_handler.cc
index 0e9056c5c7865f3bcc91d783838862c1968bf5ed..ab395e43e4c640df5ea395467734838892b8a9a7 100644
--- a/chrome/browser/android/offline_pages/offline_page_request_handler.cc
+++ b/chrome/browser/android/offline_pages/offline_page_request_handler.cc
@@ -4,15 +4,45 @@
 
 #include "chrome/browser/android/offline_pages/offline_page_request_handler.h"
 
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string_tokenizer.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
+#include "chrome/browser/android/offline_pages/offline_page_tab_helper.h"
+#include "chrome/browser/android/offline_pages/offline_page_utils.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "components/offline_pages/offline_page_feature.h"
+#include "components/offline_pages/offline_page_model.h"
+#include "components/previews/previews_experiments.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "net/base/network_change_notifier.h"
+#include "net/nqe/network_quality_estimator.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_interceptor.h"
+#include "net/url_request/url_request_job.h"
 
 namespace offline_pages {
 
+const char kLoadingOfflinePageHeader[] = "X-chromium-offline";
+const char kLoadingOfflinePageReason[] = "reason=";
+const char kLoadingOfflinePageDueToNetError[] = "error";
+
 namespace {
 
+// This enum is used to tell all possible outcomes of handling network requests
+// that might serve offline contents.
+enum class RequestResult {
+  OFFLINE_PAGE_SERVED,
+  NO_TAB_ID,
+  NO_WEB_CONTENTS,
+  PAGE_NOT_FRESH,
+  OFFLINE_PAGE_NOT_FOUND
+};
+
 int kUserDataKey;  // Only address is used.
 
 // An interceptor to hijack requests and potentially service them based on
@@ -43,6 +73,193 @@ class OfflinePageRequestInterceptor : public net::URLRequestInterceptor {
   DISALLOW_COPY_AND_ASSIGN(OfflinePageRequestInterceptor);
 };
 
+NetworkState GetNetworkState(net::URLRequest* request) {

[Dmitry Titov, 2016/08/16 20:02:55]

This can be split into several functions and then main one calling those. It's
too big otherwise.

[jianli, 2016/08/18 22:46:35]

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  // If extra header kLoadingOfflinePageHeader is present, offline page should
+  // be loaded if available.
+  bool has_offline_header = false;
+  std::string value;
+  if (request->extra_request_headers().GetHeader(kLoadingOfflinePageHeader,
+                                                 &value)) {
+    std::string reason;
+    base::StringTokenizer tokenizer(value, ", ");
+    while (tokenizer.GetNext()) {
+      if (base::StartsWith(tokenizer.token(),
+                           kLoadingOfflinePageReason,
+                           base::CompareCase::INSENSITIVE_ASCII)) {
+        reason = tokenizer.token().substr(
+            arraysize(kLoadingOfflinePageReason) - 1);
+        break;
+      }
+    }
+    if (base::EqualsCaseInsensitiveASCII(reason,
+                                         kLoadingOfflinePageDueToNetError)) {
+      return NetworkState::FLAKY_NETWORK;
+    }
+    has_offline_header = true;
+  }
+
+  if (net::NetworkChangeNotifier::IsOffline())
+    return NetworkState::DISCONNECTED_NETWORK;
+
+  // NetworkQualityEstimator only works when it is enabled.
+  if (previews::IsOfflinePreviewsEnabled()) {
+    net::NetworkQualityEstimator* network_quality_estimator =
+        request->context() ? request->context()->network_quality_estimator()
+                           : nullptr;
+    if (network_quality_estimator) {
+      net::EffectiveConnectionType effective_connection_type =
+          network_quality_estimator->GetEffectiveConnectionType();
+      if (effective_connection_type >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE &&
+          effective_connection_type <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G) {
+        return NetworkState::PROHIBITIVELY_SLOW_NETWORK;
+      }
+    }
+  }
+
+  return has_offline_header ? NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK
+                            : NetworkState::CONNECTED_NETWORK;
+}
+
+AggregatedRequestResult RequestResultToAggregatedRequestResult(
+    RequestResult request_result, NetworkState network_state) {
+  if (request_result == RequestResult::NO_TAB_ID)
+    return AggregatedRequestResult::NO_TAB_ID;
+
+  if (request_result == RequestResult::NO_WEB_CONTENTS)
+    return AggregatedRequestResult::NO_WEB_CONTENTS;
+
+  if (request_result == RequestResult::PAGE_NOT_FRESH) {
+    DCHECK_EQ(NetworkState::PROHIBITIVELY_SLOW_NETWORK, network_state);
+    return
+        AggregatedRequestResult::PAGE_NOT_FRESH_ON_PROHIBITIVELY_SLOW_NETWORK;
+  }
+
+  if (request_result == RequestResult::OFFLINE_PAGE_NOT_FOUND) {
+    switch (network_state) {
+      case NetworkState::DISCONNECTED_NETWORK:
+        return AggregatedRequestResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK;
+      case NetworkState::PROHIBITIVELY_SLOW_NETWORK:
+        return AggregatedRequestResult::
+            PAGE_NOT_FOUND_ON_PROHIBITIVELY_SLOW_NETWORK;
+      case NetworkState::FLAKY_NETWORK:
+        return AggregatedRequestResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK;
+      case NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK:
+        return AggregatedRequestResult::PAGE_NOT_FOUND_ON_CONNECTED_NETWORK;
+      default:
+        NOTREACHED();
+    }
+  }
+
+  DCHECK_EQ(RequestResult::OFFLINE_PAGE_SERVED, request_result);
+  switch (network_state) {
+    case NetworkState::DISCONNECTED_NETWORK:
+      return AggregatedRequestResult::SHOW_OFFLINE_ON_DISCONNECTED_NETWORK;
+    case NetworkState::PROHIBITIVELY_SLOW_NETWORK:
+      return AggregatedRequestResult::
+          SHOW_OFFLINE_ON_PROHIBITIVELY_SLOW_NETWORK;
+    case NetworkState::FLAKY_NETWORK:
+      return AggregatedRequestResult::SHOW_OFFLINE_ON_FLAKY_NETWORK;
+    case NetworkState::CONNECTED_NETWORK:
+      return AggregatedRequestResult::SHOW_OFFLINE_ON_CONNECTED_NETWORK;
+    case NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK:
+      return AggregatedRequestResult::SHOW_OFFLINE_ON_CONNECTED_NETWORK;
+    default:
+      NOTREACHED();
+   }
+
+   return AggregatedRequestResult::AGGREGATED_REQUEST_RESULT_MAX;
+}
+
+void ReportRequestResult(RequestResult request_result,
+                         NetworkState network_state) {
+  OfflinePageRequestHandler::ReportAggregatedRequestResult(
+      RequestResultToAggregatedRequestResult(request_result, network_state));
+}
+
+void NotifyOfflineFilePathOnIO(base::WeakPtr<OfflinePageRequestJob> job,
+                               const base::FilePath& offline_file_path) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (!job)
+    return;
+
+  if (offline_file_path.empty())
+    job->FallbackToDefault();
+  else
+    job->SetOfflineFilePath(offline_file_path);
+}
+
+// Notifies OfflinePageRequestJob about the offline file path. Note that the
+// file path may be empty if not found or on error.
+void NotifyOfflineFilePathOnUI(base::WeakPtr<OfflinePageRequestJob> job,
+                               const base::FilePath& offline_file_path) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Delegates to IO thread since OfflinePageRequestJob should only be accessed
+  // from IO thread.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&NotifyOfflineFilePathOnIO, job, offline_file_path));
+}
+
+// Handles the result of finding an offline page.
+void SelectPageForOnlineURLDone(
+    NetworkState network_state,
+    base::WeakPtr<OfflinePageRequestJob> job,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    const OfflinePageItem* offline_page){
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  RequestResult request_result;
+  base::FilePath offline_file_path;
+
+  // If offline page is not found, fall through with empty file path such that
+  // OfflinePageRequestJob will be notified on failure.
+  if (offline_page) {

[Dmitry Titov, 2016/08/16 20:02:55]

early return would reduce the nesting level here.

[jianli, 2016/08/18 22:46:35]

Done.

+    // If the page is being loaded on a slow network, only use the offline page
+    // if it was created within the past day.
+    // TODO(romax): Make the constant be policy driven.
+    if (network_state == NetworkState::PROHIBITIVELY_SLOW_NETWORK &&
+        base::Time::Now() - offline_page->creation_time >
+            base::TimeDelta::FromDays(1)) {
+      request_result = RequestResult::PAGE_NOT_FRESH;

[Dmitry Titov, 2016/08/16 20:02:55]

It would be probbaly more readable if this code had structure:
if (cond1) {
  ..
  ReportRequestResult(...);
  return;
}

if (cond2) {
....

[jianli, 2016/08/18 22:46:35]

Done.

+    } else {
+      // |web_contents_getter| is passed from IO thread. We neeed to check if
+      // web contents is still valid.
+      content::WebContents* web_contents = web_contents_getter.Run();
+      if (web_contents) {
+        request_result = RequestResult::OFFLINE_PAGE_SERVED;
+
+        offline_file_path = offline_page->file_path;
+
+        // Since offline page will be loaded, it should be marked as accessed.
+        OfflinePageModel* offline_page_model =
+            OfflinePageModelFactory::GetForBrowserContext(
+                web_contents->GetBrowserContext());
+        DCHECK(offline_page_model);
+        offline_page_model->MarkPageAccessed(offline_page->offline_id);
+
+        // Save an cached copy of OfflinePageItem such that Tab code can get
+        // the loaded offline page immediately.
+        OfflinePageTabHelper* tab_helper =
+            OfflinePageTabHelper::FromWebContents(web_contents);
+        DCHECK(tab_helper);
+        tab_helper->SetOfflinePage(*offline_page);
+      } else {
+        request_result = RequestResult::NO_WEB_CONTENTS;
+      }
+    }
+  } else {
+    request_result = RequestResult::OFFLINE_PAGE_NOT_FOUND;
+  }
+
+  ReportRequestResult(request_result, network_state);
+  NotifyOfflineFilePathOnUI(job, offline_file_path);
+}
+
 }  // namespace
 
 // static
@@ -64,9 +281,15 @@ void OfflinePageRequestHandler::InitializeHandler(
   if (request->method() != "GET")
     return;
 
+  // Bail out if no need to load offline page.
+  NetworkState network_state = GetNetworkState(request);
+  if (network_state == NetworkState::CONNECTED_NETWORK)
+    return;
+
   // Any more precise checks to see if the request should be intercepted are
   // asynchronous, so just create our handler in all cases.
-  request->SetUserData(&kUserDataKey, new OfflinePageRequestHandler());
+  request->SetUserData(&kUserDataKey,
+                       new OfflinePageRequestHandler(network_state));

[mmenke, 2016/08/17 17:09:28]

Rather than attach an object that does stuff to the URLRequest, I'd suggest just
attaching a POD type with the data you need.  Scope all the extra stuff you need
to do to the URLRequestJob subclass, as that's nicely destroyed between
redirects, promptly Killed() on cancellation, etc.  Then I'd move all the extra
logic to do stuff into the job's code, instead of the handler, and do it all on
Start(), so you don't have to worry about Start() not being called instantly.

[jianli, 2016/08/18 22:46:35]

Done.

 }
 
 // static
@@ -86,20 +309,94 @@ OfflinePageRequestHandler::CreateInterceptor(void* profile_id) {
       new OfflinePageRequestInterceptor(profile_id));
 }
 
-OfflinePageRequestHandler::OfflinePageRequestHandler() {
+// static
+void OfflinePageRequestHandler::ReportAggregatedRequestResult(
+    AggregatedRequestResult result) {
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.AggregatedRequestResult",
+      static_cast<int>(result),
+      static_cast<int>(AggregatedRequestResult::AGGREGATED_REQUEST_RESULT_MAX));
+}
+
+OfflinePageRequestHandler::OfflinePageRequestHandler(NetworkState network_state)
+    : network_state_(network_state),
+      weak_ptr_factory_(this) {
+  DCHECK_NE(NetworkState::CONNECTED_NETWORK, network_state);
 }
 
 OfflinePageRequestHandler::~OfflinePageRequestHandler() {
 }
 
+void OfflinePageRequestHandler::OnPrepareToRestart() {
+  use_default_ = true;
+  offline_job_.reset();
+}
+
 net::URLRequestJob* OfflinePageRequestHandler::MaybeCreateJob(
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate,
     void* profile_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  // TODO(jianli): to be implemented.
-  return nullptr;
+  // Fall back to default which is set when offline page is not found.
+  if (use_default_)
+    return nullptr;
+
+  OfflinePageRequestJob* offline_job =
+      new OfflinePageRequestJob(request, network_delegate, this);
+  offline_job_ = offline_job->GetWeakPtr();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&OfflinePageRequestHandler::GetOfflineFileURL,

[mmenke, 2016/08/17 17:09:28]

This do-two-things-at-once behavior seems weird to me.  You now have two cases
to worry about:  The job is started first, or we get the offline file URL first.
 Currently, we always start a job just after creating it, but seems like we
shouldn't rely on it, given that it's a separate method, with no documented
guarantee.  See earlier comment about a suggested redesign.

[jianli, 2016/08/18 22:46:35]

Done.

+                 weak_ptr_factory_.GetWeakPtr(),
+                 request->url(),
+                 profile_id,
+                 content::ResourceRequestInfo::ForRequest(request)->
+                     GetWebContentsGetterForRequest()));
+
+  return offline_job_.get();
+}
+
+void OfflinePageRequestHandler::GetOfflineFileURL(
+    const GURL& online_url,
+    void* profile_id,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter){
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // |profile_id| needs to be checked with ProfileManager::IsValidProfile
+  // before using it.
+  if (!g_browser_process->profile_manager()->IsValidProfile(profile_id)) {
+    FailedToGetOfflineFileURL();
+    return;
+  }
+  Profile* profile = reinterpret_cast<Profile*>(profile_id);
+
+  content::WebContents* web_contents = web_contents_getter.Run();

[mmenke, 2016/08/17 17:09:28]

web_contents_getter can return nullptr - the tab could have been closed since
the request was issued.

[jianli, 2016/08/18 22:46:35]

Done.

+  int tab_id;
+  if (!OfflinePageUtils::GetTabId(web_contents, &tab_id)) {
+    ReportRequestResult(RequestResult::NO_TAB_ID, network_state_);
+    FailedToGetOfflineFileURL();
+    return;
+  }
+
+  OfflinePageUtils::SelectPageForOnlineURL(
+      profile,
+      online_url,
+      tab_id,
+      base::Bind(&SelectPageForOnlineURLDone,
+                 network_state_,
+                 offline_job_,
+                 web_contents_getter));
+}
+
+void OfflinePageRequestHandler::FailedToGetOfflineFileURL() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Proceed with empty file path in order to notify the OfflinePageRequestJob
+  // about the failure.
+  base::FilePath empty_file_path;
+  NotifyOfflineFilePathOnUI(offline_job_, empty_file_path);

[mmenke, 2016/08/17 17:09:28]

BUG:  we're on the UI thread, this class is owned by the URLRequest, which lives
on the IO thread, and can be deleted at any time.  accessing offline_job_ here
is not safe.  I'd recommend you make all methods called on the UI thread not a
member of any class (Or members of a private, ref-counted class, though I don't
think that's needed), to avoid this sort of issue.

[jianli, 2016/08/18 22:46:35]

Done.

 }
 
 }  // namespace offline_pages