CacheStorage: Keep used CacheStorage blink objects alive.

In crrev.com/c/1321209 for crbug.com/902488 we made cache_storage
hold backend disk_cache objects open as long as the blink
CacheStorage object is in existence.  The idea was that if a context
has used cache_storage once it is likely to do so again.

It turns out, however, that the blink CacheStorage object is often
garbage collected before the context is destroyed.  This makes our
logic to keep backends warmed less effective.

This CL fixes the problem by implementing HasPendingActivity() to
keep the CacheStorage blink objects that have been used alive until
the context is destroyed.  This also allows us to remove a number of
explicit CacheStorage references held by Cache and during operations.

Bug: 947512
Change-Id: Iee1fa152ec597038e7c90030e9861a2814e0579c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545932
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646447}

third_party/blink/renderer/modules/cache_storage/cache.cc

@@ -572,11 +572,9 @@ class Cache::CodeCacheHandleCallbackForPut final
 
 Cache* Cache::Create(
     GlobalFetch::ScopedFetcher* fetcher,
-    CacheStorage* cache_storage,
     mojom::blink::CacheStorageCacheAssociatedPtrInfo cache_ptr_info,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  return MakeGarbageCollected<Cache>(fetcher, cache_storage,
-                                     std::move(cache_ptr_info),
+  return MakeGarbageCollected<Cache>(fetcher, std::move(cache_ptr_info),
                                      std::move(task_runner));
 }
 
@@ -704,16 +702,14 @@ ScriptPromise Cache::keys(ScriptState* script_state,
 }
 
 Cache::Cache(GlobalFetch::ScopedFetcher* fetcher,
-             CacheStorage* cache_storage,
              mojom::blink::CacheStorageCacheAssociatedPtrInfo cache_ptr_info,
              scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : scoped_fetcher_(fetcher), cache_storage_(cache_storage) {
+    : scoped_fetcher_(fetcher) {
   cache_ptr_.Bind(std::move(cache_ptr_info), std::move(task_runner));
 }
 
 void Cache::Trace(blink::Visitor* visitor) {
   visitor->Trace(scoped_fetcher_);
-  visitor->Trace(cache_storage_);
   ScriptWrappable::Trace(visitor);
 }
 

third_party/blink/renderer/modules/cache_storage/cache.h

@@ -38,7 +38,6 @@ struct TypeConverter<CacheQueryOptionsPtr, const blink::CacheQueryOptions*> {
 
 namespace blink {
 
-class CacheStorage;
 class ExceptionState;
 class Response;
 class Request;
@@ -51,12 +50,10 @@ class MODULES_EXPORT Cache final : public ScriptWrappable {
 
  public:
   static Cache* Create(GlobalFetch::ScopedFetcher*,
-                       CacheStorage*,
                        mojom::blink::CacheStorageCacheAssociatedPtrInfo,
                        scoped_refptr<base::SingleThreadTaskRunner>);
 
   Cache(GlobalFetch::ScopedFetcher*,
-        CacheStorage*,
         mojom::blink::CacheStorageCacheAssociatedPtrInfo,
         scoped_refptr<base::SingleThreadTaskRunner>);
 
@@ -121,10 +118,6 @@ class MODULES_EXPORT Cache final : public ScriptWrappable {
                          const CacheQueryOptions*);
 
   Member<GlobalFetch::ScopedFetcher> scoped_fetcher_;
-  // Hold a reference to CacheStorage to keep |cache_ptr_| alive.
-  // This is required because |cache_ptr_| is associated with CacheStorage's
-  // mojo message pipe.
-  Member<CacheStorage> cache_storage_;
 
   mojom::blink::CacheStorageCacheAssociatedPtr cache_ptr_;
 

third_party/blink/renderer/modules/cache_storage/cache_storage.cc

@@ -101,6 +101,8 @@ ScriptPromise CacheStorage::open(ScriptState* script_state,
     return promise;
   }
 
+  ever_used_ = true;
+
   // Make sure to bind the CacheStorage object to keep the mojo interface
   // pointer alive during the operation.  Otherwise GC might prevent the
   // callback from ever being executed.
@@ -109,8 +111,7 @@ ScriptPromise CacheStorage::open(ScriptState* script_state,
       WTF::Bind(
           [](ScriptPromiseResolver* resolver,
              GlobalFetch::ScopedFetcher* fetcher, base::TimeTicks start_time,
-             int64_t trace_id, CacheStorage* cache_storage,
-             mojom::blink::OpenResultPtr result) {
+             int64_t trace_id, mojom::blink::OpenResultPtr result) {
             UMA_HISTOGRAM_TIMES("ServiceWorkerCache.CacheStorage.Renderer.Open",
                                 base::TimeTicks::Now() - start_time);
             if (!resolver->GetExecutionContext() ||
@@ -138,14 +139,14 @@ ScriptPromise CacheStorage::open(ScriptState* script_state,
                   TRACE_ID_GLOBAL(trace_id), TRACE_EVENT_FLAG_FLOW_IN, "status",
                   "success");
               // See https://bit.ly/2S0zRAS for task types.
-              resolver->Resolve(Cache::Create(
-                  fetcher, cache_storage, std::move(result->get_cache()),
-                  resolver->GetExecutionContext()->GetTaskRunner(
-                      blink::TaskType::kMiscPlatformAPI)));
+              resolver->Resolve(
+                  Cache::Create(fetcher, std::move(result->get_cache()),
+                                resolver->GetExecutionContext()->GetTaskRunner(
+                                    blink::TaskType::kMiscPlatformAPI)));
             }
           },
           WrapPersistent(resolver), WrapPersistent(scoped_fetcher_.Get()),
-          TimeTicks::Now(), trace_id, WrapPersistent(this)));
+          TimeTicks::Now(), trace_id));
 
   return promise;
 }
@@ -165,6 +166,8 @@ ScriptPromise CacheStorage::has(ScriptState* script_state,
     return promise;
   }
 
+  ever_used_ = true;
+
   // Make sure to bind the CacheStorage object to keep the mojo interface
   // pointer alive during the operation.  Otherwise GC might prevent the
   // callback from ever being executed.
@@ -172,8 +175,7 @@ ScriptPromise CacheStorage::has(ScriptState* script_state,
       cache_name, trace_id,
       WTF::Bind(
           [](ScriptPromiseResolver* resolver, base::TimeTicks start_time,
-             int64_t trace_id, CacheStorage* _,
-             mojom::blink::CacheStorageError result) {
+             int64_t trace_id, mojom::blink::CacheStorageError result) {
             UMA_HISTOGRAM_TIMES("ServiceWorkerCache.CacheStorage.Renderer.Has",
                                 base::TimeTicks::Now() - start_time);
             TRACE_EVENT_WITH_FLOW1(
@@ -195,8 +197,7 @@ ScriptPromise CacheStorage::has(ScriptState* script_state,
                 break;
             }
           },
-          WrapPersistent(resolver), TimeTicks::Now(), trace_id,
-          WrapPersistent(this)));
+          WrapPersistent(resolver), TimeTicks::Now(), trace_id));
 
   return promise;
 }
@@ -216,6 +217,8 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
     return promise;
   }
 
+  ever_used_ = true;
+
   // Make sure to bind the CacheStorage object to keep the mojo interface
   // pointer alive during the operation.  Otherwise GC might prevent the
   // callback from ever being executed.
@@ -223,8 +226,7 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
       cache_name, trace_id,
       WTF::Bind(
           [](ScriptPromiseResolver* resolver, base::TimeTicks start_time,
-             int64_t trace_id, CacheStorage* _,
-             mojom::blink::CacheStorageError result) {
+             int64_t trace_id, mojom::blink::CacheStorageError result) {
             UMA_HISTOGRAM_TIMES(
                 "ServiceWorkerCache.CacheStorage.Renderer.Delete",
                 base::TimeTicks::Now() - start_time);
@@ -248,8 +250,7 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
                 break;
             }
           },
-          WrapPersistent(resolver), TimeTicks::Now(), trace_id,
-          WrapPersistent(this)));
+          WrapPersistent(resolver), TimeTicks::Now(), trace_id));
 
   return promise;
 }
@@ -267,6 +268,8 @@ ScriptPromise CacheStorage::keys(ScriptState* script_state) {
     return promise;
   }
 
+  ever_used_ = true;
+
   // Make sure to bind the CacheStorage object to keep the mojo interface
   // pointer alive during the operation.  Otherwise GC might prevent the
   // callback from ever being executed.
@@ -274,7 +277,7 @@ ScriptPromise CacheStorage::keys(ScriptState* script_state) {
       trace_id,
       WTF::Bind(
           [](ScriptPromiseResolver* resolver, base::TimeTicks start_time,
-             int64_t trace_id, CacheStorage* _, const Vector<String>& keys) {
+             int64_t trace_id, const Vector<String>& keys) {
             UMA_HISTOGRAM_TIMES("ServiceWorkerCache.CacheStorage.Renderer.Keys",
                                 base::TimeTicks::Now() - start_time);
             TRACE_EVENT_WITH_FLOW1(
@@ -286,8 +289,7 @@ ScriptPromise CacheStorage::keys(ScriptState* script_state) {
               return;
             resolver->Resolve(keys);
           },
-          WrapPersistent(resolver), TimeTicks::Now(), trace_id,
-          WrapPersistent(this)));
+          WrapPersistent(resolver), TimeTicks::Now(), trace_id));
 
   return promise;
 }
@@ -333,6 +335,8 @@ ScriptPromise CacheStorage::MatchImpl(ScriptState* script_state,
     return promise;
   }
 
+  ever_used_ = true;
+
   // Make sure to bind the CacheStorage object to keep the mojo interface
   // pointer alive during the operation.  Otherwise GC might prevent the
   // callback from ever being executed.
@@ -341,7 +345,7 @@ ScriptPromise CacheStorage::MatchImpl(ScriptState* script_state,
       WTF::Bind(
           [](ScriptPromiseResolver* resolver, base::TimeTicks start_time,
              const MultiCacheQueryOptions* options, int64_t trace_id,
-             CacheStorage* _, mojom::blink::MatchResultPtr result) {
+             mojom::blink::MatchResultPtr result) {
             base::TimeDelta elapsed = base::TimeTicks::Now() - start_time;
             if (!options->hasCacheName() || options->cacheName().IsEmpty()) {
               UMA_HISTOGRAM_LONG_TIMES(
@@ -382,14 +386,14 @@ ScriptPromise CacheStorage::MatchImpl(ScriptState* script_state,
             }
           },
           WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
-          trace_id, WrapPersistent(this)));
+          trace_id));
 
   return promise;
 }
 
 CacheStorage::CacheStorage(ExecutionContext* context,
                            GlobalFetch::ScopedFetcher* fetcher)
-    : scoped_fetcher_(fetcher) {
+    : ContextClient(context), scoped_fetcher_(fetcher), ever_used_(false) {
   // See https://bit.ly/2S0zRAS for task types.
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
       context->GetTaskRunner(blink::TaskType::kMiscPlatformAPI);
@@ -411,9 +415,21 @@ CacheStorage::CacheStorage(ExecutionContext* context,
 
 CacheStorage::~CacheStorage() = default;
 
+bool CacheStorage::HasPendingActivity() const {
+  // Once the CacheStorage has been used once we keep it alive until the
+  // context goes away.  This allows us to use the existence of this
+  // context as a hint to optimizations such as keeping backend disk_caches
+  // open in the browser process.
+  //
+  // Note, this also keeps the CacheStorage alive during active Cache and
+  // CacheStorage operations.
+  return ever_used_;
+}
+
 void CacheStorage::Trace(blink::Visitor* visitor) {
   visitor->Trace(scoped_fetcher_);
   ScriptWrappable::Trace(visitor);
+  ContextClient::Trace(visitor);
 }
 
 bool CacheStorage::IsAllowed(ScriptState* script_state) {

third_party/blink/renderer/modules/cache_storage/cache_storage.h

@@ -9,7 +9,9 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "third_party/blink/public/mojom/cache_storage/cache_storage.mojom-blink.h"
+#include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise.h"
+#include "third_party/blink/renderer/core/execution_context/context_lifecycle_observer.h"
 #include "third_party/blink/renderer/core/fetch/global_fetch.h"
 #include "third_party/blink/renderer/modules/cache_storage/cache.h"
 #include "third_party/blink/renderer/modules/cache_storage/multi_cache_query_options.h"
@@ -21,8 +23,11 @@
 
 namespace blink {
 
-class CacheStorage final : public ScriptWrappable {
+class CacheStorage final : public ScriptWrappable,
+                           public ActiveScriptWrappable<CacheStorage>,
+                           public ContextClient {
   DEFINE_WRAPPERTYPEINFO();
+  USING_GARBAGE_COLLECTED_MIXIN(CacheStorage);
 
  public:
   static CacheStorage* Create(ExecutionContext*, GlobalFetch::ScopedFetcher*);
@@ -39,6 +44,7 @@ class CacheStorage final : public ScriptWrappable {
                       const MultiCacheQueryOptions*,
                       ExceptionState&);
 
+  bool HasPendingActivity() const override;
   void Trace(blink::Visitor*) override;
 
  private:
@@ -52,6 +58,7 @@ class CacheStorage final : public ScriptWrappable {
 
   RevocableInterfacePtr<mojom::blink::CacheStorage> cache_storage_ptr_;
   base::Optional<bool> allowed_;
+  bool ever_used_;
 
   DISALLOW_COPY_AND_ASSIGN(CacheStorage);
 };

third_party/blink/renderer/modules/cache_storage/cache_storage.idl

@@ -4,6 +4,7 @@
 
 // See https://w3c.github.io/ServiceWorker/#cachestorage-interface
 [
+    ActiveScriptWrappable,
     SecureContext,
     Exposed=(Window,Worker)
 ] interface CacheStorage {

third_party/blink/renderer/modules/cache_storage/cache_test.cc

@@ -282,7 +282,7 @@ class CacheStorageTest : public PageTestBase {
         mojo::AssociatedBinding<mojom::blink::CacheStorageCache>>(
         cache_.get(), std::move(request));
     return Cache::Create(
-        fetcher, nullptr /* cache_storage */, cache_ptr.PassInterface(),
+        fetcher, cache_ptr.PassInterface(),
         blink::scheduler::GetSingleThreadTaskRunnerForTesting());
   }