Ensure NavigationHandle outlives NavigationURLLoader.

NavigationURLLoaderImpl holds raw pointers to objects owned by
NavigationHandleImpl, namely AppCacheNavigationHandle. Therefore
the handle should outlive the loader.

If the handle is destroyed before the loader the following sequence can
happen:
1. (UI) |navigation_handle_| is destroyed. This destroys |appcache_handle_|
    which posts a task to the IO thread to destroy
   |appcache_handle_->core_|.
2. (Background thread) Another thread (e.g., ServiceWorkerDatabase)
   posts a task to the IO thread that results in |request_controller_|
   being called.
3. (UI) |loader_| is destroyed. This posts a task to the IO thread to destroy
   |request_controller_|.
4. (IO) |appcache_handle_->core_| is destroyed.
5. (IO) The task from 2. runs, and accesses the raw pointer to
   appcache_handle_->core.

No test added because I don't see a good way to test this.

Bug: 857005
Change-Id: I7c44312a2072836f53475559ddfe9ca1fdc8e18a
Reviewed-on: https://chromium-review.googlesource.com/1132715Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574110}

content/browser/frame_host/navigation_request.cc

@@ -726,7 +726,11 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
       FrameMsg_Navigate_Type::IsSameDocument(common_params_.navigation_type));
 
   // Reset the NavigationHandle, which is now incorrectly marked as
-  // same-document.
+  // same-document. Ensure |loader_| does not exist as it can hold raw pointers
+  // to objects owned by the handle (see the comment in the header).
+  // TODO(falken): Turn this CHECK to a DCHECK if it holds, or else call
+  // loader_.reset() manually.
+  CHECK(!loader_);
   navigation_handle_.reset();
 
   // Convert the navigation type to the appropriate cross-document one.

content/browser/frame_host/navigation_request.h

@@ -374,6 +374,11 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
 
   NavigationState state_;
 
+  // It's important to ensure |navigation_handle_| outlives |loader_|, since
+  // the loader holds raw pointers to objects owned by the navigation handle
+  // (namely, the AppCache and service worker handles). So, declare the handle
+  // before the loader.
+  std::unique_ptr<NavigationHandleImpl> navigation_handle_;
   std::unique_ptr<NavigationURLLoader> loader_;
 
   // These next items are used in browser-initiated navigations to store
@@ -405,8 +410,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   // process expects to be notified if the navigation is aborted.
   bool from_begin_navigation_;
 
-  std::unique_ptr<NavigationHandleImpl> navigation_handle_;
-
   // Holds objects received from OnResponseStarted while the WillProcessResponse
   // checks are performed by the NavigationHandle. Once the checks have been
   // completed, these objects will be used to continue the navigation.