AppCache: replace HasSecurityState() with ChildProcessSecurityPolicyImpl::Handle().

HasSecurityState() was a temporary solution to deal with security
checks on processes that have been destroyed, but which still have
messages in flight.  ChildProcessSecurityPolicy security checks on
those messages should still be able to reference the old security
state.  Previously, HasSecurityState() was used to detect that the
process was gone and just fail open.  Now, we have a better solution
in place in the form of ChildProcessSecurityPolicyImpl::Handle, which
extends the lifetime of a process's security state even after the
RenderProcessHost has gone away.  Switch AppCache to use this approach
instead, keeping the security state around while the corresponding
AppCacheHost is alive.

Bug: 943887
Change-Id: I7ebe7e127dec1c301ed61d811f0298c38778fd31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026314Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736562}

content/browser/appcache/appcache_host.cc

@@ -100,6 +100,11 @@ AppCacheHost::AppCacheHost(
       main_resource_blocked_(false),
       associated_cache_info_pending_(false) {
   service_->AddObserver(this);
+  if (process_id_ != ChildProcessHost::kInvalidUniqueID) {
+    security_policy_handle_ =
+        ChildProcessSecurityPolicyImpl::GetInstance()->CreateHandle(
+            process_id_);
+  }
 }
 
 AppCacheHost::~AppCacheHost() {
@@ -622,8 +627,11 @@ void AppCacheHost::NotifyMainResourceBlocked(const GURL& manifest_url) {
 
 void AppCacheHost::SetProcessId(int process_id) {
   DCHECK_EQ(process_id_, ChildProcessHost::kInvalidUniqueID);
+  DCHECK(!security_policy_handle_.is_valid());
   DCHECK_NE(process_id, ChildProcessHost::kInvalidUniqueID);
   process_id_ = process_id;
+  security_policy_handle_ =
+      ChildProcessSecurityPolicyImpl::GetInstance()->CreateHandle(process_id_);
 }
 
 base::WeakPtr<AppCacheHost> AppCacheHost::GetWeakPtr() {

content/browser/appcache/appcache_host.h

@@ -20,6 +20,7 @@
 #include "content/browser/appcache/appcache_group.h"
 #include "content/browser/appcache/appcache_service_impl.h"
 #include "content/browser/appcache/appcache_storage.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/common/appcache_interfaces.h"
 #include "content/common/content_export.h"
 #include "content/public/common/child_process_host.h"
@@ -177,6 +178,12 @@ class CONTENT_EXPORT AppCacheHost : public blink::mojom::AppCacheHost,
     DCHECK_NE(process_id_, ChildProcessHost::kInvalidUniqueID);
     return process_id_;
   }
+
+  using SecurityPolicyHandle = ChildProcessSecurityPolicyImpl::Handle;
+  SecurityPolicyHandle* security_policy_handle() {
+    return &security_policy_handle_;
+  }
+
   // SetProcessId may only be called once, and only if kInvalidUniqueID was
   // passed to the AppCacheHost's constructor (e.g. in a scenario where
   // NavigationRequest needs to delay specifying the |process_id| until
@@ -275,9 +282,20 @@ class CONTENT_EXPORT AppCacheHost : public blink::mojom::AppCacheHost,
   const base::UnguessableToken host_id_;
 
   // Identifies the renderer process associated with the AppCacheHost.  Used for
-  // security checks via ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin.
+  // selecting the appropriate AppCacheBackend and creating
+  // |security_policy_handle_|.
   int process_id_;
 
+  // Security policy handle for the renderer process associated with this
+  // AppCacheHost.  Used for performing CanAccessDataForOrigin() security
+  // checks.
+  //
+  // Using this handle allows these checks to work even after the corresponding
+  // RenderProcessHost has been destroyed, in the case where there are still
+  // in-flight appcache requests that need to be processed. See
+  // https://crbug.com/943887.
+  SecurityPolicyHandle security_policy_handle_;
+
   // Information about the host that created this one; the manifest
   // preferred by our creator influences which cache our main resource
   // should be loaded from.

content/browser/appcache/appcache_host_unittest.cc

@@ -188,6 +188,8 @@ TEST_F(AppCacheHostTest, Basic) {
   host.set_frontend_for_testing(&mock_frontend_);
   EXPECT_EQ(kHostIdForTest, host.host_id());
   EXPECT_EQ(kProcessIdForTest, host.process_id());
+  EXPECT_TRUE(host.security_policy_handle());
+  EXPECT_TRUE(host.security_policy_handle()->is_valid());
   EXPECT_EQ(&service_, host.service());
   EXPECT_EQ(nullptr, host.associated_cache());
   EXPECT_FALSE(host.is_selection_pending());
@@ -752,9 +754,11 @@ TEST_F(AppCacheHostTest, SelectCacheAfterProcessCleanup) {
   web_contents_.reset();
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(
+  // Since |host| for kProcessIdForTest is still alive, the corresponding
+  // SecurityState in ChildProcessSecurityPolicy should also be kept alive,
+  // allowing access for kDocumentURL.
+  EXPECT_TRUE(
       security_policy->CanAccessDataForOrigin(kProcessIdForTest, kDocumentURL));
-  EXPECT_FALSE(security_policy->HasSecurityState(kProcessIdForTest));
 
   // Verify that the document and manifest URLs do not trigger a bad message.
   {
@@ -803,9 +807,11 @@ TEST_F(AppCacheHostTest, ForeignEntryAfterProcessCleanup) {
   web_contents_.reset();
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(
+  // Since |host| for kProcessIdForTest is still alive, the corresponding
+  // SecurityState in ChildProcessSecurityPolicy should also be kept alive,
+  // allowing access for kDocumentURL.
+  EXPECT_TRUE(
       security_policy->CanAccessDataForOrigin(kProcessIdForTest, kDocumentURL));
-  EXPECT_FALSE(security_policy->HasSecurityState(kProcessIdForTest));
 
   // Verify that a document URL does not trigger a bad message.
   {

content/browser/appcache/appcache_subresource_url_factory.cc

@@ -14,7 +14,6 @@
 #include "content/browser/appcache/appcache_request.h"
 #include "content/browser/appcache/appcache_request_handler.h"
 #include "content/browser/appcache/appcache_url_loader_job.h"
-#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/loader/navigation_url_loader_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/public/browser/browser_thread.h"
@@ -371,18 +370,9 @@ void AppCacheSubresourceURLFactory::CreateLoaderAndStart(
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // TODO(943887): Replace HasSecurityState() call with something that can
-  // preserve security state after process shutdown. The security state check
-  // is a temporary solution to avoid crashes when this method is run after the
-  // process associated with |appcache_host_->process_id()| has been destroyed.
-  // It temporarily restores the old behavior of always allowing access if the
-  // process is gone.
-  // See https://crbug.com/910287 for details.
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   if (request.request_initiator.has_value() && appcache_host_ &&
-      !policy->CanAccessDataForOrigin(appcache_host_->process_id(),
-                                      request.request_initiator.value()) &&
-      policy->HasSecurityState(appcache_host_->process_id())) {
+      !appcache_host_->security_policy_handle()->CanAccessDataForOrigin(
+          request.request_initiator.value())) {
     static auto* initiator_origin_key = base::debug::AllocateCrashKeyString(
         "initiator_origin", base::debug::CrashKeySize::Size64);
     base::debug::SetCrashKeyString(