AppCache: fix a race when registering new backend on navigation

We currently have a race between:
- AppCacheBackendImpl::RegisterHost call coming over it's own mojo pipe and
  looking for precreated AppCacheHost;
- Destroying NavigationRequest, which destroys NavigationHandleImpl, which
  destroys AppCacheHavigationHandle which posts a task to destroy precreated
  AppCacheHost on IO thread.

To combat this, we take AppCacheHavigationHandle from committed NavigationRequest
before destroying it and keep it around in RenderFrameHostImpl. This measure
keeps in a common case when we commit navigation and reach for app cache host.

However, we still cannot assert that we always have a precreated host because
browser can decided to destroy the frame immediately (together with
RenderFrameHostImpl and AppCacheHavigationHandle), while renderer asks for the
AppCacheHost. The best thing we can do in this case is stall AppCacheHost
registration until renderer's frame is inevitably destroyed by the browser's
request.

This is a speculative fix for crashes in issue 941903. Some existing tests, e.g.
http/tests/misc/iframe-reparenting-id-collision.html, easily expose the race,
but since they are not using AppCache, we don't hit the crash point from the bug.

Bug: 941903
Change-Id: I3925d0b9ab9655235b524da561d0e8ba8345fb4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1529381Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642307}

content/browser/appcache/appcache_backend_impl.cc

@@ -51,6 +51,15 @@ void AppCacheBackendImpl::RegisterHost(
     // here on.
     host->set_frontend(std::move(frontend), render_frame_id);
   } else {
+    if (id < 0) {
+      // Negative ids correspond to precreated hosts. We should be able to
+      // retrieve one, but currently we have a race between this IPC and
+      // browser removing the corresponding frame and precreated AppCacheHost.
+      // Instead of crashing the renderer or returning wrong host, we do not
+      // bind any host and let renderer do nothing until it is destroyed by
+      // the request from browser.
+      return;
+    }
     host = std::make_unique<AppCacheHost>(id, process_id(), render_frame_id,
                                           std::move(frontend), service_);
   }

content/browser/appcache/appcache_navigation_handle.h

@@ -28,27 +28,26 @@ class ChromeAppCacheService;
 //   2) When the navigation request is sent to the IO thread, we include a
 //      pointer to the AppCacheNavigationHandleCore.
 //
-//   3. The AppCacheHost instance is created and its ownership is passed to the
-//      AppCacheNavigationHandleCore instance. Now the app cache host id is
-//      updated.
+//   3) The AppCacheHost instance is created and its ownership is passed to the
+//      AppCacheNavigationHandleCore instance.
 //
-//   4) The AppCacheNavigationHandleCore instance informs the
-//      AppCacheNavigationHandle instance on the UI thread that the app cache
-//      host id was updated.
-//
-//   5) When the navigation is ready to commit, the NavigationRequest will
+//   4) When the navigation is ready to commit, the NavigationRequest will
 //      update the CommitNavigationParams based on the id from the
 //      AppCacheNavigationHandle.
 //
-//   6) The commit leads to AppCache registrations happening from the renderer.
-//      This is via the IPC message AppCacheHostMsg_RegisterHost. The
-//      AppCacheDispatcherHost class which handles these IPCs will be informed
+//   5) The commit leads to AppCache registrations happening from the renderer.
+//      This is via the AppCacheBackend.RegisterHost mojo call. The
+//      AppCacheBackendImpl class which handles these calls will be informed
 //      about these hosts when the navigation commits. It will ignore the
 //      host registrations as they have already been registered. The
 //      ownership of the AppCacheHost is passed from the
-//      AppCacheNavigationHandle core to the AppCacheBackend.
-
-//   7) When the navigation finishes, the AppCacheNavigationHandle is
+//      AppCacheNavigationHandle core to the AppCacheBackendImpl.
+//
+//   6) Meanwhile, RenderFrameHostImpl takes ownership of
+//      AppCacheNavigationHandle once navigation commits, so that precreated
+//      AppCacheHost is not destroyed before IPC above reaches AppCacheBackend.
+//
+//   7) When the next navigation commits, previous AppCacheNavigationHandle is
 //      destroyed. The destructor of the AppCacheNavigationHandle posts a
 //      task to destroy the AppCacheNavigationHandleCore on the IO thread.
 

content/browser/frame_host/navigation_handle_impl.cc

@@ -538,6 +538,11 @@ void NavigationHandleImpl::InitAppCacheHandle(
       appcache_service, ChildProcessHost::kInvalidUniqueID));
 }
 
+std::unique_ptr<AppCacheNavigationHandle>
+NavigationHandleImpl::TakeAppCacheHandle() {
+  return std::move(appcache_handle_);
+}
+
 void NavigationHandleImpl::WillStartRequest(
     ThrottleChecksFinishedCallback callback) {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,

content/browser/frame_host/navigation_handle_impl.h

@@ -214,6 +214,8 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle,
     return appcache_handle_.get();
   }
 
+  std::unique_ptr<AppCacheNavigationHandle> TakeAppCacheHandle();
+
   typedef base::OnceCallback<void(NavigationThrottle::ThrottleCheckResult)>
       ThrottleChecksFinishedCallback;
 

content/browser/frame_host/render_frame_host_impl.cc

@@ -34,6 +34,7 @@
 #include "cc/base/switches.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 #include "content/browser/accessibility/browser_accessibility_state_impl.h"
+#include "content/browser/appcache/appcache_navigation_handle.h"
 #include "content/browser/background_fetch/background_fetch_service_impl.h"
 #include "content/browser/bluetooth/web_bluetooth_service_impl.h"
 #include "content/browser/browser_main_loop.h"
@@ -6281,6 +6282,8 @@ bool RenderFrameHostImpl::DidCommitNavigationInternal(
        committed_request->GetMimeType() == "message/rfc822");
 
   accessibility_reset_count_ = 0;
+  appcache_handle_ =
+      committed_request->navigation_handle()->TakeAppCacheHandle();
   frame_tree_node()->navigator()->DidNavigate(this, *validated_params,
                                               std::move(committed_request),
                                               is_same_document_navigation);

content/browser/frame_host/render_frame_host_impl.h

@@ -129,6 +129,7 @@ struct ResourceResponse;
 }  // namespace network
 
 namespace content {
+class AppCacheNavigationHandle;
 class AuthenticatorImpl;
 class FrameTree;
 class FrameTreeNode;
@@ -1743,6 +1744,11 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // NavigationRequest is for a cross-document navigation.
   std::unique_ptr<NavigationRequest> navigation_request_;
 
+  // Holds AppCacheNavigationHandle after navigation request has been committed,
+  // which keeps corresponding AppCacheHost alive while renderer asks for it.
+  // See AppCacheNavigationHandle comment for more details.
+  std::unique_ptr<AppCacheNavigationHandle> appcache_handle_;
+
   // Holds the cross-document NavigationRequests that are waiting to commit,
   // indexed by IDs. These are navigations that have passed ReadyToCommit stage
   // and are waiting for the renderer to send back a matching