Skip to content

GitLab

T
tangled
Commit 20b12740 authored by Ben Kelly's avatar Ben Kelly Committed by Commit Bot
Browse files
Options

CacheStorage: Hold DOM binding objects alive during operations. 

We are seeing some DCHECKs trigger that suggest the owning Cache object
is destroyed while an outstanding operation is in progress.  This can
in theory happen in code that operates on the cache, but then discards
its reference while waiting for the promise to settle.

Bug: 912141
Change-Id: Ic82e8b86bb427808df4d7f16326b50998cf6b031
Reviewed-on: https://chromium-review.googlesource.com/c/1364130
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: default avatarVictor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614251}
parent 8b38b5d3
Hide whitespace changes
Inline Side-by-side
Showing with 56 additions and 20 deletions
third_party/blink/renderer/modules/cache_storage/cache.cc
View file @ 20b12740
......@@ -188,12 +188,15 @@ class Cache::BarrierCallbackForPut final
if (--number_of_remaining_operations_ != 0)
return;
MaybeReportInstalledScripts();
// Make sure to bind the Cache object to keep the mojo interface pointer
// alive during the operation. Otherwise GC might prevent the callback
// from ever being executed.
cache_->cache_ptr_->Batch(
std::move(batch_operations_),
RuntimeEnabledFeatures::CacheStorageAddAllRejectsDuplicatesEnabled(),
WTF::Bind(
[](const String& method_name, ScriptPromiseResolver* resolver,
TimeTicks start_time,
TimeTicks start_time, Cache* _,
mojom::blink::CacheStorageVerboseErrorPtr error) {
ExecutionContext* context = resolver->GetExecutionContext();
if (!context || context->IsContextDestroyed())
......@@ -232,7 +235,8 @@ class Cache::BarrierCallbackForPut final
CacheStorageError::CreateException(error->value, message));
}
},
method_name_, WrapPersistent(resolver_.Get()), TimeTicks::Now()));
method_name_, WrapPersistent(resolver_.Get()), TimeTicks::Now(),
WrapPersistent(cache_.Get())));
}
void OnError(const String& error_message) {
......@@ -594,11 +598,14 @@ ScriptPromise Cache::MatchImpl(ScriptState* script_state,
return promise;
}
// Make sure to bind the Cache object to keep the mojo interface pointer
// alive during the operation. Otherwise GC might prevent the callback
// from ever being executed.
cache_ptr_->Match(
request->CreateFetchAPIRequest(), ToQueryParams(options),
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
const CacheQueryOptions* options,
const CacheQueryOptions* options, Cache* _,
mojom::blink::MatchResultPtr result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
......@@ -631,7 +638,8 @@ ScriptPromise Cache::MatchImpl(ScriptState* script_state,
*result->get_response()));
}
},
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options)));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
WrapPersistent(this)));
return promise;
}
......@@ -652,11 +660,14 @@ ScriptPromise Cache::MatchAllImpl(ScriptState* script_state,
}
}
// Make sure to bind the Cache object to keep the mojo interface pointer
// alive during the operation. Otherwise GC might prevent the callback
// from ever being executed.
cache_ptr_->MatchAll(
std::move(fetch_api_request), ToQueryParams(options),
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
const CacheQueryOptions* options,
const CacheQueryOptions* options, Cache* _,
mojom::blink::MatchAllResultPtr result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
......@@ -687,7 +698,8 @@ ScriptPromise Cache::MatchAllImpl(ScriptState* script_state,
resolver->Resolve(responses);
}
},
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options)));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
WrapPersistent(this)));
return promise;
}
......@@ -745,12 +757,15 @@ ScriptPromise Cache::DeleteImpl(ScriptState* script_state,
operation->request = request->CreateFetchAPIRequest();
operation->match_params = ToQueryParams(options);
// Make sure to bind the Cache object to keep the mojo interface pointer
// alive during the operation. Otherwise GC might prevent the callback
// from ever being executed.
cache_ptr_->Batch(
std::move(batch_operations),
RuntimeEnabledFeatures::CacheStorageAddAllRejectsDuplicatesEnabled(),
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
const CacheQueryOptions* options,
const CacheQueryOptions* options, Cache* _,
mojom::blink::CacheStorageVerboseErrorPtr error) {
ExecutionContext* context = resolver->GetExecutionContext();
if (!context || context->IsContextDestroyed())
......@@ -793,7 +808,8 @@ ScriptPromise Cache::DeleteImpl(ScriptState* script_state,
kJSMessageSource, kWarningMessageLevel, message));
}
},
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options)));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
WrapPersistent(this)));
return promise;
}
......@@ -903,11 +919,14 @@ ScriptPromise Cache::KeysImpl(ScriptState* script_state,
}
}
// Make sure to bind the Cache object to keep the mojo interface pointer
// alive during the operation. Otherwise GC might prevent the callback
// from ever being executed.
cache_ptr_->Keys(
std::move(fetch_api_request), ToQueryParams(options),
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
const CacheQueryOptions* options,
const CacheQueryOptions* options, Cache* _,
mojom::blink::CacheKeysResultPtr result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
......@@ -938,7 +957,8 @@ ScriptPromise Cache::KeysImpl(ScriptState* script_state,
resolver->Resolve(requests);
}
},
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options)));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
WrapPersistent(this)));
return promise;
}
......
third_party/blink/renderer/modules/cache_storage/cache_storage.cc
View file @ 20b12740
......@@ -32,12 +32,15 @@ ScriptPromise CacheStorage::open(ScriptState* script_state,
const String& cache_name) {
ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
// 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.
cache_storage_ptr_->Open(
cache_name,
WTF::Bind(
[](ScriptPromiseResolver* resolver,
GlobalFetch::ScopedFetcher* fetcher, TimeTicks start_time,
mojom::blink::OpenResultPtr result) {
CacheStorage* _, mojom::blink::OpenResultPtr result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
return;
......@@ -60,7 +63,7 @@ ScriptPromise CacheStorage::open(ScriptState* script_state,
}
},
WrapPersistent(resolver), WrapPersistent(scoped_fetcher_.Get()),
TimeTicks::Now()));
TimeTicks::Now(), WrapPersistent(this)));
return resolver->Promise();
}
......@@ -69,11 +72,14 @@ ScriptPromise CacheStorage::has(ScriptState* script_state,
const String& cache_name) {
ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
// 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.
cache_storage_ptr_->Has(
cache_name,
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
mojom::blink::CacheStorageError result) {
CacheStorage* _, mojom::blink::CacheStorageError result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
return;
......@@ -91,7 +97,7 @@ ScriptPromise CacheStorage::has(ScriptState* script_state,
break;
}
},
WrapPersistent(resolver), TimeTicks::Now()));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(this)));
return resolver->Promise();
}
......@@ -100,11 +106,14 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
const String& cache_name) {
ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
// 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.
cache_storage_ptr_->Delete(
cache_name,
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
mojom::blink::CacheStorageError result) {
CacheStorage* _, mojom::blink::CacheStorageError result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
return;
......@@ -123,7 +132,7 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
break;
}
},
WrapPersistent(resolver), TimeTicks::Now()));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(this)));
return resolver->Promise();
}
......@@ -131,8 +140,11 @@ ScriptPromise CacheStorage::Delete(ScriptState* script_state,
ScriptPromise CacheStorage::keys(ScriptState* script_state) {
ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
// 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.
cache_storage_ptr_->Keys(WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
[](ScriptPromiseResolver* resolver, TimeTicks start_time, CacheStorage* _,
const Vector<String>& keys) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
......@@ -141,7 +153,7 @@ ScriptPromise CacheStorage::keys(ScriptState* script_state) {
TimeTicks::Now() - start_time);
resolver->Resolve(keys);
},
WrapPersistent(resolver), TimeTicks::Now()));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(this)));
return resolver->Promise();
}
......@@ -172,11 +184,14 @@ ScriptPromise CacheStorage::MatchImpl(ScriptState* script_state,
return promise;
}
// 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.
cache_storage_ptr_->Match(
request->CreateFetchAPIRequest(), Cache::ToQueryParams(options),
WTF::Bind(
[](ScriptPromiseResolver* resolver, TimeTicks start_time,
const CacheQueryOptions* options,
const CacheQueryOptions* options, CacheStorage* _,
mojom::blink::MatchResultPtr result) {
if (!resolver->GetExecutionContext() ||
resolver->GetExecutionContext()->IsContextDestroyed())
......@@ -213,7 +228,8 @@ ScriptPromise CacheStorage::MatchImpl(ScriptState* script_state,
*result->get_response()));
}
},
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options)));
WrapPersistent(resolver), TimeTicks::Now(), WrapPersistent(options),
WrapPersistent(this)));
return promise;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment