Skip to content

Commit

Permalink
[CacheStorage] Don't store open caches in Blink
Browse files Browse the repository at this point in the history
Blink objects were storing cache objects so that they could be returned quickly
on subsequent open calls. This is problematic as there can be many contexts
(different windows, workers, service workers, and dev tools) opening/deleting
caches. If one of them deletes the cache then all of the others need to be
invalidated. Rather than synchronize the various contexts, for now just remove
the stored caches from Blink.

BUG=639034

Review-Url: https://codereview.chromium.org/2270413003
Cr-Commit-Position: refs/heads/master@{#414147}
  • Loading branch information
jkarlin authored and Commit bot committed Aug 24, 2016
1 parent b90b789 commit db792de
Show file tree
Hide file tree
Showing 5 changed files with 3 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ PASS CacheStorage.open with an empty name
PASS CacheStorage.open with no arguments
PASS CacheStorage.has with existing cache
PASS CacheStorage.has with nonexistent cache
FAIL CacheStorage.open with existing cache assert_not_equals: CacheStorage.open should return a new Cache object each time its called. got disallowed value object "[object Cache]"
PASS CacheStorage.open with existing cache
PASS CacheStorage.delete with existing cache
PASS CacheStorage.delete with nonexistent cache
FAIL CacheStorage names are DOMStrings not USVStrings assert_true: keys should include cache with bad name expected true got false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ PASS CacheStorage.open with an empty name
PASS CacheStorage.open with no arguments
PASS CacheStorage.has with existing cache
PASS CacheStorage.has with nonexistent cache
FAIL CacheStorage.open with existing cache assert_not_equals: CacheStorage.open should return a new Cache object each time its called. got disallowed value object "[object Cache]"
PASS CacheStorage.open with existing cache
PASS CacheStorage.delete with existing cache
PASS CacheStorage.delete with nonexistent cache
FAIL CacheStorage names are DOMStrings not USVStrings assert_true: keys should include cache with bad name expected true got false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ PASS CacheStorage.open with an empty name
PASS CacheStorage.open with no arguments
PASS CacheStorage.has with existing cache
PASS CacheStorage.has with nonexistent cache
FAIL CacheStorage.open with existing cache assert_not_equals: CacheStorage.open should return a new Cache object each time its called. got disallowed value object "[object Cache]"
PASS CacheStorage.open with existing cache
PASS CacheStorage.delete with existing cache
PASS CacheStorage.delete with nonexistent cache
FAIL CacheStorage names are DOMStrings not USVStrings assert_true: keys should include cache with bad name expected true got false
Expand Down
14 changes: 0 additions & 14 deletions third_party/WebKit/Source/modules/cachestorage/CacheStorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ class CacheStorage::WithCacheCallbacks final : public WebServiceWorkerCacheStora
if (!m_resolver->getExecutionContext() || m_resolver->getExecutionContext()->activeDOMObjectsAreStopped())
return;
Cache* cache = Cache::create(m_cacheStorage->m_scopedFetcher, wrapUnique(webCache.release()));
m_cacheStorage->m_nameToCacheMap.set(m_cacheName, cache);
m_resolver->resolve(cache);
m_resolver.clear();
}
Expand Down Expand Up @@ -151,7 +150,6 @@ class CacheStorage::DeleteCallbacks final : public WebServiceWorkerCacheStorage:

void onSuccess() override
{
m_cacheStorage->m_nameToCacheMap.remove(m_cacheName);
if (!m_resolver->getExecutionContext() || m_resolver->getExecutionContext()->activeDOMObjectsAreStopped())
return;
m_resolver->resolve(true);
Expand Down Expand Up @@ -219,12 +217,6 @@ ScriptPromise CacheStorage::open(ScriptState* scriptState, const String& cacheNa
ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
const ScriptPromise promise = resolver->promise();

if (m_nameToCacheMap.contains(cacheName)) {
Cache* cache = m_nameToCacheMap.find(cacheName)->value;
resolver->resolve(cache);
return promise;
}

if (m_webCacheStorage)
m_webCacheStorage->dispatchOpen(new WithCacheCallbacks(cacheName, this, resolver), cacheName);
else
Expand All @@ -241,11 +233,6 @@ ScriptPromise CacheStorage::has(ScriptState* scriptState, const String& cacheNam
ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
const ScriptPromise promise = resolver->promise();

if (m_nameToCacheMap.contains(cacheName)) {
resolver->resolve(true);
return promise;
}

if (m_webCacheStorage)
m_webCacheStorage->dispatchHas(new Callbacks(resolver), cacheName);
else
Expand Down Expand Up @@ -339,7 +326,6 @@ void CacheStorage::dispose()
DEFINE_TRACE(CacheStorage)
{
visitor->trace(m_scopedFetcher);
visitor->trace(m_nameToCacheMap);
}

} // namespace blink
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ class CacheStorage final : public GarbageCollectedFinalized<CacheStorage>, publi

Member<GlobalFetch::ScopedFetcher> m_scopedFetcher;
std::unique_ptr<WebServiceWorkerCacheStorage> m_webCacheStorage;
HeapHashMap<String, Member<Cache>> m_nameToCacheMap;
};

} // namespace blink
Expand Down

0 comments on commit db792de

Please sign in to comment.