Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1704293002:
ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context (Closed)
|
Created:
4 years, 10 months ago by nhiroki Modified:
4 years, 10 months ago Reviewers:
falken CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context
Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were
asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache
writer did not check the validity of ServiceWorkerContextCore before attempting
to create the accessors, so it caused crashes when the context was already gone
due to storage failure etc.
This CL stops asynchronously creating those response accessors, and creates them
when ServiceWorkerCacheWriter is constructed so that the cache writer no longer
has to concern about the validity of the context. If the context is gone after
the accessors are created, diskcache acceess simply fails[1].
[1] https://codereview.chromium.org/1693303002/
BUG=584606
Committed: https://crrev.com/67f195ef19365fce612b2d84799a2c7b26c2df96
Cr-Commit-Position: refs/heads/master@{#376113}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : fix impl comment #Patch Set 3 : fix win bots #
Messages
Total messages: 21 (13 generated)
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore and that caused crashes when the context was gone because of shutdown etc. This CL stops asynchronously creating those response accessors and creates them when ServiceWorkerCacheWriter is constructed so that the writer no longer takes care of the validity of the context. BUG=584606 ==========
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore and that caused crashes when the context was gone because of shutdown etc. This CL stops asynchronously creating those response accessors and creates them when ServiceWorkerCacheWriter is constructed so that the writer no longer takes care of the validity of the context. BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the reader and writer, and it caused crashes if the context was gone due to shutdown etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the reader and writer, and it caused crashes if the context was gone due to shutdown etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, and it caused crashes when the context was already gone due to shutdown etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, and it caused crashes when the context was already gone due to shutdown etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, and it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, and it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they just fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they simply fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, they simply fail when accessing to the disk cache[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, diskcache acceess simply fails[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ==========
nhiroki@chromium.org changed reviewers: + falken@chromium.org
falken@, can you review this? Thanks! https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (left): https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:49: // multiple readers internally. Creating unused readers and writers wouldn't cost a lot because their ctors just set up fields and don't interact with the disk cache. Rather, we could reduce code complexity by creating the readers and writers synchronously.
lgtm, nice fix. This is yet another WeakPtr dereference crash... I sometimes think we should name these fields weak_* or something. Probably we just have to be more vigilant in code reviews. https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (left): https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:49: // multiple readers internally. On 2016/02/18 04:21:53, nhiroki wrote: > Creating unused readers and writers wouldn't cost a lot because their ctors just > set up fields and don't interact with the disk cache. Rather, we could reduce > code complexity by creating the readers and writers synchronously. Thanks for this comment, I was wondering why it was originally written this way and why it's ok to change it (and thanks to the the writer of the original code comment!). https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:44: // The existing reader may be null, in which case this instance will change "existing reader" to |compare_reader|
Patchset #2 (id:40001) has been deleted
Thank you for reviewing! https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1704293002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:44: // The existing reader may be null, in which case this instance will On 2016/02/18 04:43:59, falken wrote: > change "existing reader" to |compare_reader| Done.
On 2016/02/18 04:43:59, falken wrote: > lgtm, nice fix. > > This is yet another WeakPtr dereference crash... I sometimes think we should > name these fields weak_* or something. Probably we just have to be more vigilant > in code reviews. Yeah... in addition to that, we might have to have more tests for failure cases.
On 2016/02/18 07:15:40, nhiroki wrote:
> On 2016/02/18 04:43:59, falken wrote:
> > lgtm, nice fix.
> >
> > This is yet another WeakPtr dereference crash... I sometimes think we should
> > name these fields weak_* or something. Probably we just have to be more
> vigilant
> > in code reviews.
>
> Yeah... in addition to that, we might have to have more tests for failure
cases.
('failure cases' -> 'cases where the context is invalid')
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/1704293002/#ps80001 (title: "fix win bots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1704293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1704293002/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, diskcache acceess simply fails[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 ========== to ========== ServiceWorker: Stop asynchronously creating response accessors to fix crash due to null context Before this CL, ServiceWorkerResponseReader and ServiceWorkerResponseWriter were asynchronously created in ServiceWorkerCacheWriter. Unfortunately, the cache writer did not check the validity of ServiceWorkerContextCore before attempting to create the accessors, so it caused crashes when the context was already gone due to storage failure etc. This CL stops asynchronously creating those response accessors, and creates them when ServiceWorkerCacheWriter is constructed so that the cache writer no longer has to concern about the validity of the context. If the context is gone after the accessors are created, diskcache acceess simply fails[1]. [1] https://codereview.chromium.org/1693303002/ BUG=584606 Committed: https://crrev.com/67f195ef19365fce612b2d84799a2c7b26c2df96 Cr-Commit-Position: refs/heads/master@{#376113} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/67f195ef19365fce612b2d84799a2c7b26c2df96 Cr-Commit-Position: refs/heads/master@{#376113} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
