New issue
Advanced search Search tips

Issue 630036 link

Starred by 2 users
Status: Fixed
Owner:
jkarlin@chromium.org
Closed: Jul 2016
Cc:
jkarlin@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Hit DCHECK in content::CacheStorage::SimpleCacheLoader::CleanUpDeletedCache

Project Member Reported by ricea@chromium.org, Jul 20 2016 
What steps will reproduce the problem?
(1) run-webkit-tests -f

What is the expected output?

Does not crash in http/tests/cookies/same-site/popup-cross-site-post.html layout test

What do you see instead?

Crashes in http/tests/cookies/same-site/popup-cross-site-post.html layout test

Specifically, it hits a DCHECK in content::CacheStorage::SimpleCacheLoader::CleanUpDeletedCache

Excerpt from stack trace:

 9  libbase.so!logging::LogMessage::~LogMessage() + 0x2fa
    rsp = 0x00007f3d1bb3db00   rip = 0x00007f3d33f1d69a
    Found by: call frame info
10  libcontent.so!content::CacheStorage::SimpleCacheLoader::CleanUpDeletedCache(std::string const&) + 0x10c
    rbx = 0x000007e0ee8ad8c0   rsp = 0x00007f3d1bb3df50
    r12 = 0x00007f3d1bb3df98   r14 = 0x00007f3d1bb3e458
    r15 = 0x000007e0ee8ad8c0   rip = 0x00007f3d348acbac
    Found by: call frame info
11  libcontent.so!content::CacheStorage::DeleteCacheDidGetSize(std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> >, long) + 0x5f
    rbx = 0x000007e0ee8ad8c0   rsp = 0x00007f3d1bb3e450
    r12 = 0x000007e0eeaf3000   r13 = 0x0000000000000000
    r14 = 0x00007f3d1bb3e458   r15 = 0x00007f3d348acaa0
    rip = 0x00007f3d348aaf0f
    Found by: call frame info
12  libcontent.so!void base::internal::Invoker<base::internal::BindState<void (content::CacheStorage::*)(std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> >, long), base::WeakPtr<content::CacheStorage>, base::internal::PassedWrapper<std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> > > >, void (long)>::RunImpl<void (content::CacheStorage::* const&)(std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> >, long), std::tuple<base::WeakPtr<content::CacheStorage>, base::internal::PassedWrapper<std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> > > > const&, 0ul, 1ul>(void (content::CacheStorage::* const&)(std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> >, long), std::tuple<base::WeakPtr<content::CacheStorage>, base::internal::PassedWrapper<std::unique_ptr<content::CacheStorageCache, std::default_delete<content::CacheStorageCache> > > > const&, base::IndexSequence<0ul, 1ul>, long&&) + 0xc1
    rbx = 0x00007f3d348aaeb0   rsp = 0x00007f3d1bb3e480
    r12 = 0x000007e0eeaf3000   r13 = 0x0000000000000000
    r14 = 0x00007f3d1bb3e640   r15 = 0x0000000000000000
    rip = 0x00007f3d348b0841
    Found by: call frame info
13  libcontent.so!content::CacheStorageCache::PendingSizeCallback(base::Callback<void (long), (base::internal::CopyMode)1> const&, long) + 0x36
    rbx = 0x000007e0ee9b44a0   rsp = 0x00007f3d1bb3e640
    r12 = 0x00007f3d1bb3e648   r13 = 0x0000000000000000
    r14 = 0x000007e0eeaf3000   r15 = 0x0000000000000000
    rip = 0x00007f3d348b4f96
    Found by: call frame info

See attachment for full stack trace.

ToT was f7dbf39be31d8aa9214d5d84da613508d4e06491 (July 14th)
popup-cross-site-post-crash-log.txt
111 KB View Download

Comment 1 by jsb...@chromium.org, Jul 25 2016

Cc: jkarlin@chromium.org
Owner: cmumford@chromium.org
Status: Assigned (was: Untriaged)
cmumford@ - can you take a look?

Unlikely that it's triggered in http/tests/cookies/same-site/popup-cross-site-post.html  - more likely this is cleanup from a previous test running asynchronously.

jkarlin@ can probably advise.

Comment 2 by jkarlin@chromium.org, Jul 25 2016

Labels: -Pri-3 Pri-1
This looks pretty bad. It's crashing a lot of tests that run after the cache storage tests (e.g., canvas/ and cookies/ tests).

The problem is that the CacheStorage::SimpleCacheLoader keeps a map of cache names to its path and doesn't expect two caches with the same name to exist in parallel. But it's possible as of https://codereview.chromium.org/2111243003 and the new layout test in there triggers it.

Setup:
1) Cache A with name "foo" is created
2) Cache is doomed, but js hangs onto the handle.
3) Cache B with name "foo" is created

At this point the SimpleCacheLoader overwrites the name to directory entry, and we've lost the first one. Once A's last handle is gone it deletes cache B's directory in CleanUpDeletedCache. Once B's last handle is gone CleanUpDeletedCache's DCHECK fails because 
the name is no longer in the map.


Here is how this could be fixed:

SimpleCacheLoader would have two maps:

std::map<cache_name, path> cache_name_to_cache_dir_ // Active cache name to path map
std::map<CacheStorageCache*, path> doomed_cache_paths_ // Maps opened caches to their paths on disk.

And do the following:

1) Change CacheLoader::NotifyCacheDoomed to take a CacheStorageCache* instead of a cache name
2) Override SimpleCacheLoader::NotifyCacheDoomed  to remove cache_name from cache_paths_ and insert the CacheStorageCache* into doomed_cache_paths_.
3) Change CacheLoader::CleanUpDeletedCache to take a CacheStorageCache* 
4) Change SimpleCacheLoader::CleanUpDeletedCache to delete from doomed_cache_paths_
5) MemoryCacheLoader's functions would basically remain the same, they'd just get the cache name from the passed in CacheStorageCache.

Comment 3 by jkarlin@chromium.org, Jul 26 2016

Owner: jkarlin@chromium.org
Status: Started (was: Assigned)
Since this is blowing up tests we should land this one quickly. I'm at the office already so I'll take the bug.

Comment 4 by jkarlin@chromium.org, Jul 26 2016 

While debugging this I also discovered  issue 631467 .
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7680d8569c891a84c9eff3aa21d06c3584339965

commit 7680d8569c891a84c9eff3aa21d06c3584339965
Author: jkarlin <jkarlin@chromium.org>
Date: Tue Jul 26 21:26:04 2016

[CacheStorage] Deleting a cache could delete the wrong directory

The CacheStorageCache::CacheLoader maps cache names to paths. The problem is
that as of recent changes, it's possible to have two caches with the same name
exist simultaneously (the doomed one and a new one). If you then delete the
doomed one from disk, the CacheLoader will delete the new one's directory
instead since it maps name to path.

This CL fixes the above problem by keeping track of doomed caches and their
paths separately from live caches.

BUG= 630036 

Review-Url: https://codereview.chromium.org/2186433004
Cr-Commit-Position: refs/heads/master@{#407920}

[modify] https://crrev.com/7680d8569c891a84c9eff3aa21d06c3584339965/content/browser/cache_storage/cache_storage.cc
[modify] https://crrev.com/7680d8569c891a84c9eff3aa21d06c3584339965/content/browser/cache_storage/cache_storage_manager_unittest.cc

Comment 6 by jkarlin@chromium.org, Jul 29 2016

Status: Fixed (was: Started)
Confirmed that http/tests/cookies/same-site/popup-cross-site-post.html is no longer flaky. Closing as fixed.
►

Sign in to add a comment