Protect against null crash in fetchDiskCacheEntries
Created attachment 368530 [details] Patch
<rdar://problem/47759337>
Attachment 368530 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkProcess.cpp:1275: Missing space before ( in switch( [whitespace/parens] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 368537 [details] Patch
Comment on attachment 368537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368537&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1277 > + case NetworkCache::Cache::TraversalResult::DecodeError: > + ASSERT_NOT_REACHED(); This shouldn't assert. The design is to handle serialization format changes (and corruption for any reason) silently. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1461 > + ASSERT_NOT_REACHED(); Same here.
Comment on attachment 368537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368537&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.h:124 > - void traverse(Function<void(const TraversalEntry*)>&&); > + enum class TraversalResult : bool { FinishedTraversing, DecodeError }; > + void traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&&); How about simply having a separate 'finished' bit ? All this seems bit too much.
Comment on attachment 368537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368537&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1277 >> + ASSERT_NOT_REACHED(); > > This shouldn't assert. The design is to handle serialization format changes (and corruption for any reason) silently. Ok. >> Source/WebKit/NetworkProcess/cache/NetworkCache.h:124 >> + void traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&&); > > How about simply having a separate 'finished' bit ? All this seems bit too much. That would make it so the function signature expresses that there is a state where the TraversalEntry could be non null and finished could be true, which is not the case. This way expresses that we either have a valid TraversalEntry or one of two other states. I think this is better unless you feel strongly.
Comment on attachment 368537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368537&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:484 > if (!entry) { > - traverseHandler(nullptr); > + traverseHandler(makeUnexpected(TraversalResult::DecodeError)); > return; > } Actually, can't we simply remove the traverseHandler() call here? There is nothing useful a client can do there.
Comment on attachment 368537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368537&action=review >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:484 >> } > > Actually, can't we simply remove the traverseHandler() call here? There is nothing useful a client can do there. Wow, that's so elegant. That makes this whole patch a 1 line change.
Created attachment 368707 [details] Patch
http://trac.webkit.org/r244850