PlzNavigate: AppCache support.

content/browser/appcache/appcache_dispatcher_host.cc

Index: content/browser/appcache/appcache_dispatcher_host.cc
diff --git a/content/browser/appcache/appcache_dispatcher_host.cc b/content/browser/appcache/appcache_dispatcher_host.cc
index 2eb90dd5189e5630c4b3baadc6cb9d40995e4791..67d216e15dbefd87de7351aaba5301de2d232d63 100644
--- a/content/browser/appcache/appcache_dispatcher_host.cc
+++ b/content/browser/appcache/appcache_dispatcher_host.cc
@@ -4,12 +4,22 @@
 
 #include "content/browser/appcache/appcache_dispatcher_host.h"
 
+#include <map>
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/bad_message.h"
 #include "content/common/appcache_messages.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/user_metrics.h"
+#include "content/public/common/browser_side_navigation_policy.h"
+
+namespace {
+
+typedef std::map<int, content::AppCacheDispatcherHost*> ProcessIdToHostMap;

[michaeln, 2016/12/03 00:58:58]

would you mind using a using alias for this?

[ananta, 2016/12/03 14:55:04]

Done.

+base::LazyInstance<ProcessIdToHostMap> g_process_id_host_map;

[michaeln, 2016/12/03 00:58:58]

too bad we need this, does this do the "leaky" behavior by default?

[ananta, 2016/12/03 14:55:04]

By default it uses the DefaultLazyInstanceTraits which deletes the instance
during exit

+
+}  // namespace
 
 namespace content {
 
@@ -21,22 +31,21 @@ AppCacheDispatcherHost::AppCacheDispatcherHost(
       frontend_proxy_(this),
       process_id_(process_id),
       weak_factory_(this) {
+  g_process_id_host_map.Get()[process_id] = this;

[michaeln, 2016/12/03 00:58:58]

i think we have thread-safety issues, the message filters are created on the UI
thread, what thread is this map accessed on?

[ananta, 2016/12/03 14:55:04]

The map is populated on the UI thread and is only accessed on the IO thread.

 }
 
 void AppCacheDispatcherHost::OnChannelConnected(int32_t peer_pid) {
-  if (appcache_service_.get()) {
-    backend_impl_.Initialize(
-        appcache_service_.get(), &frontend_proxy_, process_id_);
-    get_status_callback_ =
-        base::Bind(&AppCacheDispatcherHost::GetStatusCallback,
-                    weak_factory_.GetWeakPtr());
-    start_update_callback_ =
-        base::Bind(&AppCacheDispatcherHost::StartUpdateCallback,
-                    weak_factory_.GetWeakPtr());
-    swap_cache_callback_ =
-        base::Bind(&AppCacheDispatcherHost::SwapCacheCallback,
-                    weak_factory_.GetWeakPtr());
-  }
+  if (!appcache_service_.get())
+    return;
+
+  backend_impl_.Initialize(appcache_service_.get(), &frontend_proxy_,
+                           process_id_);
+  get_status_callback_ = base::Bind(&AppCacheDispatcherHost::GetStatusCallback,
+                                    weak_factory_.GetWeakPtr());
+  start_update_callback_ = base::Bind(
+      &AppCacheDispatcherHost::StartUpdateCallback, weak_factory_.GetWeakPtr());
+  swap_cache_callback_ = base::Bind(&AppCacheDispatcherHost::SwapCacheCallback,
+                                    weak_factory_.GetWeakPtr());
 }
 
 bool AppCacheDispatcherHost::OnMessageReceived(const IPC::Message& message) {
@@ -62,10 +71,45 @@ bool AppCacheDispatcherHost::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-AppCacheDispatcherHost::~AppCacheDispatcherHost() {}
+void AppCacheDispatcherHost::RegisterPendingHost(int host_id) {

[michaeln, 2016/12/03 00:58:58]

please put the method bodies in the .cc file in the same order as their
declaration in the .h file

[ananta, 2016/12/03 14:55:04]

This function has gone

+  DCHECK(IsBrowserSideNavigationEnabled());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(host_id != kAppCacheNoHostId);
+  DCHECK(pending_hosts_.find(host_id) == pending_hosts_.end());
+  pending_hosts_.insert(host_id);
+}
+
+AppCacheDispatcherHost::~AppCacheDispatcherHost() {
+  ProcessIdToHostMap::iterator index =
+      g_process_id_host_map.Get().find(process_id_);
+  if ((index != g_process_id_host_map.Get().end()) && index->second == this)

[michaeln, 2016/12/03 00:58:58]

do we ever expect second != this?

[ananta, 2016/12/03 14:55:04]

I think there are cases when a new AppDispatcherHost is created for the same
pid.
I had a DCHECK in here for an error check an it was firing at times. Changed
this to a 
DCHECK. Will see if the run passes.

[ananta, 2016/12/03 22:42:35]

So there are cases where multiple hosts are created for the same pid due to
multiple renderer processes getting launched for the same navigation. The ones
created earlier are eventually destroyed. The check for index->second == this is
necessary

+    g_process_id_host_map.Get().erase(index);
+}
+
+void AppCacheDispatcherHost::RegisterPrecreatedHost(
+    std::unique_ptr<AppCacheHost> host) {
+  DCHECK(host.get());
+  DCHECK(IsBrowserSideNavigationEnabled());
+
+  // Before switching the backends we mark the host as pending so we can avoid
+  // registering them again in the AppCacheHostMsg_RegisterHost IPC.
+  RegisterPendingHost(host->host_id());

[michaeln, 2016/12/03 00:58:58]

if this is the only callsite for this->RegisterPendingHost(), i think you could
put the pending_hosts_.insert() call right in this method body and nix the extra
method

or better yet, maybe the BackendImpl can deal with the pendingness all
internally and "do the right thing" when RegisterHost() is called?

[ananta, 2016/12/03 14:55:04]

I moved the pending hosts to the backend. However we still need the
RegisterPrecreatedHost
method here because the renderer has not registered with the host and in turn
the appcache service when this is called (during commitnavigation).

+  host->set_frontend(&frontend_proxy_);
+  backend_impl_.RegisterPrecreatedHost(std::move(host));
+}
 
 void AppCacheDispatcherHost::OnRegisterHost(int host_id) {
   if (appcache_service_.get()) {
+    // PlzNavigate. If the |host_id| is the pending_backends_ list, it means
+    // it was registered via the RegisterPendingHost() function. We remove the
+    // host from the pending list and return without registering the host.
+    if (IsBrowserSideNavigationEnabled()) {
+      auto found = pending_hosts_.find(host_id);
+      if (found != pending_hosts_.end()) {
+        pending_hosts_.erase(found);
+        return;
+      }
+    }
     if (!backend_impl_.RegisterHost(host_id)) {
       bad_message::ReceivedBadMessage(this, bad_message::ACDH_REGISTER);
     }
@@ -94,8 +138,7 @@ void AppCacheDispatcherHost::OnSelectCache(
     int64_t cache_document_was_loaded_from,
     const GURL& opt_manifest_url) {
   if (appcache_service_.get()) {
-    if (!backend_impl_.SelectCache(host_id,
-                                   document_url,
+    if (!backend_impl_.SelectCache(host_id, document_url,
                                    cache_document_was_loaded_from,
                                    opt_manifest_url)) {
       bad_message::ReceivedBadMessage(this, bad_message::ACDH_SELECT_CACHE);
@@ -108,8 +151,8 @@ void AppCacheDispatcherHost::OnSelectCache(
 void AppCacheDispatcherHost::OnSelectCacheForWorker(
     int host_id, int parent_process_id, int parent_host_id) {
   if (appcache_service_.get()) {
-    if (!backend_impl_.SelectCacheForWorker(
-            host_id, parent_process_id, parent_host_id)) {
+    if (!backend_impl_.SelectCacheForWorker(host_id, parent_process_id,
+                                            parent_host_id)) {
       bad_message::ReceivedBadMessage(
           this, bad_message::ACDH_SELECT_CACHE_FOR_WORKER);
     }
@@ -134,8 +177,8 @@ void AppCacheDispatcherHost::OnMarkAsForeignEntry(
     const GURL& document_url,
     int64_t cache_document_was_loaded_from) {
   if (appcache_service_.get()) {
-    if (!backend_impl_.MarkAsForeignEntry(
-            host_id, document_url, cache_document_was_loaded_from)) {
+    if (!backend_impl_.MarkAsForeignEntry(host_id, document_url,
+                                          cache_document_was_loaded_from)) {
       bad_message::ReceivedBadMessage(this,
                                       bad_message::ACDH_MARK_AS_FOREIGN_ENTRY);
     }
@@ -158,8 +201,8 @@ void AppCacheDispatcherHost::OnGetStatus(int host_id, IPC::Message* reply_msg) {
 
   pending_reply_msg_.reset(reply_msg);
   if (appcache_service_.get()) {
-    if (!backend_impl_.GetStatusWithCallback(
-            host_id, get_status_callback_, reply_msg)) {
+    if (!backend_impl_.GetStatusWithCallback(host_id, get_status_callback_,
+                                             reply_msg)) {
       bad_message::ReceivedBadMessage(this, bad_message::ACDH_GET_STATUS);
     }
     return;
@@ -179,8 +222,8 @@ void AppCacheDispatcherHost::OnStartUpdate(int host_id,
 
   pending_reply_msg_.reset(reply_msg);
   if (appcache_service_.get()) {
-    if (!backend_impl_.StartUpdateWithCallback(
-            host_id, start_update_callback_, reply_msg)) {
+    if (!backend_impl_.StartUpdateWithCallback(host_id, start_update_callback_,
+                                               reply_msg)) {
       bad_message::ReceivedBadMessage(this, bad_message::ACDH_START_UPDATE);
     }
     return;
@@ -199,8 +242,8 @@ void AppCacheDispatcherHost::OnSwapCache(int host_id, IPC::Message* reply_msg) {
 
   pending_reply_msg_.reset(reply_msg);
   if (appcache_service_.get()) {
-    if (!backend_impl_.SwapCacheWithCallback(
-            host_id, swap_cache_callback_, reply_msg)) {
+    if (!backend_impl_.SwapCacheWithCallback(host_id, swap_cache_callback_,
+                                             reply_msg)) {
       bad_message::ReceivedBadMessage(this, bad_message::ACDH_SWAP_CACHE);
     }
     return;
@@ -231,4 +274,14 @@ void AppCacheDispatcherHost::SwapCacheCallback(bool result, void* param) {
   Send(pending_reply_msg_.release());
 }
 
+// static
+scoped_refptr<AppCacheDispatcherHost> AppCacheDispatcherHost::GetHostForProcess(
+    int process_id) {
+  ProcessIdToHostMap::iterator index =
+      g_process_id_host_map.Get().find(process_id);
+  if (index == g_process_id_host_map.Get().end())
+    return scoped_refptr<AppCacheDispatcherHost>();
+  return index->second;
+}
+
 }  // namespace content