Change Cache Storage callback to WeakPersistent instead of Persistent

This fixes a memory leak and renderer crash as described below. Change Cache Storage "open" method callback to have a WeakPersistent reference to CacheStorage to avoid a circular reference between them. Remove Persistent<CacheStorage> from callback for "delete" since it wasn't used. WebServiceWorkerCacheStorageImpl keeps the callback while waiting for mojo response from browser process which keeps CacheStorage alive, CacheStorage also has a reference to WebServiceWQorkerCacheStorage, which is implemented by WebServiceCacheStorageImpl, creating the circular reference, this situation leads to memory leak because those can't be garbage collected properly on some termination conditions, this leak in turn would cause renderer to crash when starting a new worker and trying to reuse pointer to address cleaned by Oilpan heap. When a worker is terminated with pending WithCacheCallback objects, the termination GC callback will access the Persistent handle. However, it will point to an object in a dead Oilpan heap and cause a segfault. Using a WeakPersistent is a workaround to prevent this crash, since the termination GC callback won't try to access it. In the future, Oilpan might be updated to handle this more gracefully see https://crbug.com/831117. The added test catches two conditions where renderer process was crashing: 1. When initializing Cache Storage after "close()". 2. Initializing Cache Storage before "close()" and issuing new calls, that trigger mojo after "close()". Bug: 831054 Change-Id: I6620d8107c00aed1c386c869dc1a793bc51d97fa Reviewed-on: https://chromium-review.googlesource.com/1011467 Commit-Queue: Luciano Pacheco (SYD) <lucmult@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#551262}

Change Cache Storage callback to WeakPersistent instead of Persistent
This fixes a memory leak and renderer crash as described below. Change Cache Storage "open" method callback to have a WeakPersistent reference to CacheStorage to avoid a circular reference between them. Remove Persistent<CacheStorage> from callback for "delete" since it wasn't used. WebServiceWorkerCacheStorageImpl keeps the callback while waiting for mojo response from browser process which keeps CacheStorage alive, CacheStorage also has a reference to WebServiceWQorkerCacheStorage, which is implemented by WebServiceCacheStorageImpl, creating the circular reference, this situation leads to memory leak because those can't be garbage collected properly on some termination conditions, this leak in turn would cause renderer to crash when starting a new worker and trying to reuse pointer to address cleaned by Oilpan heap. When a worker is terminated with pending WithCacheCallback objects, the termination GC callback will access the Persistent handle. However, it will point to an object in a dead Oilpan heap and cause a segfault. Using a WeakPersistent is a workaround to prevent this crash, since the termination GC callback won't try to access it. In the future, Oilpan might be updated to handle this more gracefully see https://crbug.com/831117. The added test catches two conditions where renderer process was crashing: 1. When initializing Cache Storage after "close()". 2. Initializing Cache Storage before "close()" and issuing new calls, that trigger mojo after "close()". Bug: 831054 Change-Id: I6620d8107c00aed1c386c869dc1a793bc51d97fa Reviewed-on: https://chromium-review.googlesource.com/1011467 Commit-Queue: Luciano Pacheco (SYD) <lucmult@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#551262}
5a45635a · Luciano Pacheco · Commit Bot · d501f82a · 5a45635a · 5a45635a
Commit 5a45635a authored Apr 17, 2018 by Luciano Pacheco Committed by Commit Bot Apr 17, 2018
4 changed files
--- a/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/worker-closer.js
+++ b/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/worker-closer.js
+console.log('worker-close.js starting.');
+fetch('');
+close();
+// Touching Cache Storage only after closing.
+caches.open('');
--- a/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/worker-closer2.js
+++ b/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/worker-closer2.js
+console.log('worker-close2.js starting.');
+// Initializing Cache Storage before closing.
+var c = caches.open('v1').then(cache => {
+  return cache;
+});
+
+fetch('');
+close();
+
+// Continue to use Cache Storage after closing.
+caches.open('');
+c.then(cache => {
+  console.log(cache.keys());
+});
--- a/third_party/WebKit/LayoutTests/http/tests/cachestorage/worker-deleted.html
+++ b/third_party/WebKit/LayoutTests/http/tests/cachestorage/worker-deleted.html
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>Cache Storage: Verify Worker calling close doesnt crash render process </title>
+<link rel="help" href="https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-storage">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+var worker_script = 'serviceworker/worker-closer.js';
+var worker_script2 = 'serviceworker/worker-closer2.js';
+test(function(t) {
+  var p = new Promise(function(resolve, reject) {
+      var worker1 = new Worker(worker_script);
+      var worker2 = new Worker(worker_script2);
+      return;
+    }).then(() => {
+      var worker1 = new Worker(worker_script);
+      var worker2 = new Worker(worker_script2);
+      return 'success';
+    }).then(result => {
+      assert_equals(result, 'success');
+    });
+  Promise.resolve(p);
+});
+</script>
+<body> Something
+</body>
+
+</html>
--- a/third_party/blink/renderer/modules/cachestorage/cache_storage.cc
+++ b/third_party/blink/renderer/modules/cachestorage/cache_storage.cc
@@ -82,6 +82,7 @@ class CacheStorage::WithCacheCallbacks final
    if (!resolver_->GetExecutionContext() ||
        resolver_->GetExecutionContext()->IsContextDestroyed())
      return;
+    DCHECK(cache_storage_) << "CacheStorage should outlive WithCacheCallbacks.";
    Cache* cache = Cache::Create(cache_storage_->scoped_fetcher_,
                                 base::WrapUnique(web_cache.release()));
    resolver_->Resolve(cache);
@@ -103,7 +104,16 @@ class CacheStorage::WithCacheCallbacks final

 private:
  String cache_name_;
-  Persistent<CacheStorage> cache_storage_;
+
+  // TODO(crbug.com/831117): Switch to Persistent<CacheStorage> once Oilpan can
+  // handle it on thread shutdown.
+  // |cache_storage_| is guaranteed to be alive during OnSuccess/OnError because
+  // CacheStorage is actually owned by GlobalCacheStorage, which is terminated
+  // when LocalDOMWindow/WorkerGlobalScope is terminated.
+  // Ownership chain:
+  // GlobalCacheStorage => CacheStorage => WebServiceWorkerCacheStorage(Impl) =>
+  // WithCacheCallbacks.
+  WeakPersistent<CacheStorage> cache_storage_;
  Persistent<ScriptPromiseResolver> resolver_;
 };

@@ -154,7 +164,6 @@ class CacheStorage::DeleteCallbacks final
                  CacheStorage* cache_storage,
                  ScriptPromiseResolver* resolver)
      : cache_name_(cache_name),
-        cache_storage_(cache_storage),
        resolver_(resolver) {}
  ~DeleteCallbacks() override = default;

@@ -181,7 +190,6 @@ class CacheStorage::DeleteCallbacks final

 private:
  String cache_name_;
-  Persistent<CacheStorage> cache_storage_;
  Persistent<ScriptPromiseResolver> resolver_;
 };