At first I thought this was because the CacheStorageCachePtr didn't use a RevocableInterface, which is still probably a good idea to have so that the persistents would be dropped, but it can't be that because the context is still alive in that DCHECK.

Just looking at the code it is not immediately obvious to me why the ScriptPromiseResolver is collected while not being resolved in a put operation.

I was wondering if the FetchDataLoader properly held itself alive, but it seems like there would be more errors in other places if that didn't work properly.

Crash id ef37c5285424fec2 is an example where match() fails to resolve:

0:014> dps create_stack_trace
089fe478  0f4a2e20 chrome_child!base::debug::StackTrace::StackTrace+0x20 [C:\b\c\b\win_asan\src\base\debug\stack_trace_win.cc @ 290]
089fe47c  13134977 chrome_child!blink::ScriptPromiseResolver::ScriptPromiseResolver+0x137 [C:\b\c\b\win_asan\src\third_party\blink\renderer\bindings\core\v8\script_promise_resolver.cc @ 22]
089fe480  127fd915 chrome_child!blink::ConstructTrait<blink::ScriptPromiseResolver,1>::Construct<blink::ScriptState *&>+0xf5 [C:\b\c\b\win_asan\src\third_party\blink\renderer\platform\heap\heap.h @ 588]
089fe484  14386505 chrome_child!blink::Cache::MatchImpl+0x29 [C:\b\c\b\win_asan\src\third_party\blink\renderer\modules\cache_storage\cache.cc @ 590]
089fe488  143864c1 chrome_child!blink::Cache::match+0xb5 [C:\b\c\b\win_asan\src\third_party\blink\renderer\modules\cache_storage\cache.cc @ 458]
089fe48c  1412cf48 chrome_child!blink::V8Cache::MatchMethodCallback+0x1c6 [C:\b\c\b\win_asan\src\out\Release\gen\third_party\blink\renderer\bindings\modules\v8\v8_cache.cc @ 355]
089fe490  0f72076d chrome_child!v8::internal::FunctionCallbackArguments::Call+0x22d [C:\b\c\b\win_asan\src\v8\src\api-arguments-inl.h @ 146]
089fe494  10c7b947 chrome_child!v8::internal::`anonymous namespace'::HandleApiCallHelper<0>+0x2b7 [C:\b\c\b\win_asan\src\v8\src\builtins\builtins-api.cc @ 111]
089fe498  00000000

This is interesting, because match has a much simpler implementation than put, so it's easier to analyze.

Should we be calling Detach() if the context is destroyed here?

https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/cache_storage/cache.cc?l=605&rcl=36d28a9b535e70c5b5389f852ee36152051e3d90

No, the DCHECK doesn't trigger when the context is destroyed: https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_promise_resolver.cc?l=41

Hmm, maybe the callback should hold a reference to the Cache object itself.  Its possible the Cache wrapper gets GC'd before the operation completes.  If this happens then perhaps then the Cache object and its mojo interface are destroyed.

Yes, I think you are right. That would explain those crashes!

Ok, I'll make a CL.  Is there any way to force a GC in a test?

On layout tests, you can use window.gc().

or asyncGC, from gc.js

I'm having trouble constructing a test that triggers this problem.  The test would need to reliably delay the disk operation long enough for the GC to complete.

I'm just going to fix the suspected bug for now.

https://chromium-review.googlesource.com/c/chromium/src/+/1364130

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

commit 20b1274003890d6d42807a41daf846948410f355
Author: Ben Kelly <wanderview@chromium.org>
Date: Thu Dec 06 03:09:11 2018

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: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614251}
[modify] https://crrev.com/20b1274003890d6d42807a41daf846948410f355/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/20b1274003890d6d42807a41daf846948410f355/third_party/blink/renderer/modules/cache_storage/cache_storage.cc

Leaving this open until we can see if the DCHECK crashes go away or not.

I don't see any more crashes (although I may not be querying the crash tool correctly).  Marking this fixed.

This fixes a subtle bug related to GC timing that can create unexplained behavior for web developers.  I'd like to merge it to M72 if possible.

Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit c61b8f5ed9db99dab3995f93a7f38ab6512c0e86
Author: Ben Kelly <wanderview@chromium.org>
Date: Wed Dec 12 03:34:54 2018

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: Victor Costan <pwnall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614251}(cherry picked from commit 20b1274003890d6d42807a41daf846948410f355)
Reviewed-on: https://chromium-review.googlesource.com/c/1373091
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#274}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/c61b8f5ed9db99dab3995f93a7f38ab6512c0e86/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/c61b8f5ed9db99dab3995f93a7f38ab6512c0e86/third_party/blink/renderer/modules/cache_storage/cache_storage.cc

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

Commit: c61b8f5ed9db99dab3995f93a7f38ab6512c0e86
Author: wanderview@chromium.org
Commiter: wanderview@chromium.org
Date: 2018-12-12 03:34:54 +0000 UTC

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: Victor Costan <pwnall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614251}(cherry picked from commit 20b1274003890d6d42807a41daf846948410f355)
Reviewed-on: https://chromium-review.googlesource.com/c/1373091
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#274}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}