QuotaManagerProxy::GetUsageAndQuota not running its callback |
||||||||||||||||
Issue descriptionSteps to reproduce: 1. Open an incognito tab 2. Go to https://events.google.com/io2016/ 3. Wait for "Caching Complete!" toast If you don't see the toast (say 1 in 10 times or so), then you've reproduced the problem. The ServiceWorker is stuck in the installing step, waiting for a cache.put() to resolve. Printfs show that the put operation isn't resolving because the call to quota_manager_proxy_->GetUsageAndQuota isn't running its callback. It seems the QuotaManager gets stuck and none of the subsequent calls work. This started happening with M50, which is the first stable release in which CacheStorage checks quota in Put operations. See https://codereview.chromium.org/1636613002/ I've only seen this happen in incognito windows (as it's easier to test there), not sure if it happens in non-incognito. But it's verified to happen on OSX and Linux desktop.
,
Apr 22 2016
Thanks Eric. That could be related. One thing that GetUsageAndQuota would do is query CacheStorage for its size which would lead to a memory cache operation in incognito mode. Perhaps that's what is failing to return.
,
Apr 22 2016
Narrowed it down to CacheStorage::Size not running its callback so moving it over to Blink>Storage>CacheStorage.
,
Apr 22 2016
Here is what's happening: 1. Cache A is opened 2. Cache B is opened 3. Cache A::Put is called and blocks on initialization 4. Cache B::Put is called and blocks on initialization 5. Cache A initializes and calls QuotaManager::GetUsageAndQuota() 6. Size() gets called on Cache A and Cache B for GetUsageAndQuota 7. Cache A runs its size immediately because it's initialized 8. Cache B blocks Size until initialization completes, putting it behind the Put operation in the queue 9. Cache B initializes, and starts the next operation, B::Put 10. B::Put calls QuotaManager::GetUsageAndQuota, which is blocked on the first call to GetUsageAndQuota, which is waiting for the pending B::Size() operation which is next in queue but won't run until B::Put finishes 11. Deadlock I really really dislike that CacheStorageCache::Put() calls out to another class that needs to run a CacheStorageCache operation. I'm open to suggestions as to how that could be avoided. In the meanwhile, I've created https://codereview.chromium.org/1916473002 which runs pending Size() operations immediately after initialization, before scheduled operations. It makes sense and it works, but it makes me sad.
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b14d719f48c5d2b1f2f61fec372ee01e5f938a8 commit 8b14d719f48c5d2b1f2f61fec372ee01e5f938a8 Author: jkarlin <jkarlin@chromium.org> Date: Fri Apr 22 18:52:02 2016 [CacheStorage] Prioritize Size operations over other scheduled operations This CL runs pending Size() operations immediately after backend initialization instead of in scheduler order. This is because scheduled operations might depend on the QuotaManager which may call Size() mid-operation. BUG= 605663 Review URL: https://codereview.chromium.org/1916473002 Cr-Commit-Position: refs/heads/master@{#389184} [modify] https://crrev.com/8b14d719f48c5d2b1f2f61fec372ee01e5f938a8/content/browser/cache_storage/cache_storage_cache.cc [modify] https://crrev.com/8b14d719f48c5d2b1f2f61fec372ee01e5f938a8/content/browser/cache_storage/cache_storage_cache.h [modify] https://crrev.com/8b14d719f48c5d2b1f2f61fec372ee01e5f938a8/content/browser/cache_storage/cache_storage_cache_unittest.cc
,
Apr 22 2016
Will let this run over the weekend and hope to merge on Monday (4/25) assuming things look good.
,
Apr 23 2016
Your change meets the bar and is auto-approved for M51 (branch: 2704)
,
Apr 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee24d582b9e8cf99fdab2efc28dd6b39021540ca commit ee24d582b9e8cf99fdab2efc28dd6b39021540ca Author: Josh Karlin <jkarlin@chromium.org> Date: Mon Apr 25 11:09:03 2016 [CacheStorage] Prioritize Size operations over other scheduled operations This CL runs pending Size() operations immediately after backend initialization instead of in scheduler order. This is because scheduled operations might depend on the QuotaManager which may call Size() mid-operation. BUG= 605663 Review URL: https://codereview.chromium.org/1916473002 Cr-Commit-Position: refs/heads/master@{#389184} (cherry picked from commit 8b14d719f48c5d2b1f2f61fec372ee01e5f938a8) Review URL: https://codereview.chromium.org/1915893002 . Cr-Commit-Position: refs/branch-heads/2704@{#212} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/ee24d582b9e8cf99fdab2efc28dd6b39021540ca/content/browser/cache_storage/cache_storage_cache.cc [modify] https://crrev.com/ee24d582b9e8cf99fdab2efc28dd6b39021540ca/content/browser/cache_storage/cache_storage_cache.h [modify] https://crrev.com/ee24d582b9e8cf99fdab2efc28dd6b39021540ca/content/browser/cache_storage/cache_storage_cache_unittest.cc
,
Apr 29 2016
Requesting stable merge. This bug can cause pages to fail to load due to CacheStorage locking up. No problems found in the dev and canary bake in.
,
Apr 29 2016
[Automated comment] Request affecting a post-stable build (M50), manual review required.
,
May 6 2016
Approving merge to M50 branch 2661, based on comment #9. Please merge before 1:00 PM PST on Monday (05/09) so we can take it for next week stable refresh.
,
May 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e09345bdc7a8dccf7baa38a3959d7ba7380fc54c commit e09345bdc7a8dccf7baa38a3959d7ba7380fc54c Author: Josh Karlin <jkarlin@chromium.org> Date: Fri May 06 19:03:36 2016 [CacheStorage] Prioritize Size operations over other scheduled operations This CL runs pending Size() operations immediately after backend initialization instead of in scheduler order. This is because scheduled operations might depend on the QuotaManager which may call Size() mid-operation. BUG= 605663 Review URL: https://codereview.chromium.org/1916473002 Cr-Commit-Position: refs/heads/master@{#389184} (cherry picked from commit 8b14d719f48c5d2b1f2f61fec372ee01e5f938a8) Review URL: https://codereview.chromium.org/1957663004 . Cr-Commit-Position: refs/branch-heads/2661@{#663} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/e09345bdc7a8dccf7baa38a3959d7ba7380fc54c/content/browser/cache_storage/cache_storage_cache.cc [modify] https://crrev.com/e09345bdc7a8dccf7baa38a3959d7ba7380fc54c/content/browser/cache_storage/cache_storage_cache.h [modify] https://crrev.com/e09345bdc7a8dccf7baa38a3959d7ba7380fc54c/content/browser/cache_storage/cache_storage_cache_unittest.cc
,
May 6 2016
,
May 11 2016
Tested the issue on Windows 7, Mac 10.11.4, Ubuntu 14.04 using 50.0.2661.102.Observed that "Caching Complete!" toast is seen. Please find attached screencast. Marking it as TE-Verified.
,
May 27 2016
Just for reference: commit 8b14d719f48c5d2b1f2f61fec372ee01e5f938a8 was: initially in 52.0.2716.0 merged to 50.0.2661.98 (as e09345bdc7a8dccf7baa38a3959d7ba7380fc54c) merged to 51.0.2704.27 (as ee24d582b9e8cf99fdab2efc28dd6b39021540ca) (generated by "git find-releases")
,
Jun 16 2016
Note that there is another way this hang can occur, see continued discussion in issue 620713 . |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by ericbide...@google.com
, Apr 21 2016