| Summary: | Protect against null crash in fetchDiskCacheEntries | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, cgarcia, ews-watchlist, koivisto, rniwa, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alex Christensen
2019-04-29 18:02:20 PDT
Created attachment 368530 [details]
Patch
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
|