Attachment 368537 Details for Bug 197399 – Patch

[patch] Patch

bug-197399-20190429212216.patch (text/plain), 7.97 KB, created by Alex Christensen on 2019-04-29 21:22:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-04-29 21:22:17 PDT

Size: 7.97 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 244762)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,19 @@
>+2019-04-29  Alex Christensen  <achristensen@webkit.org>
>+
>+        Protect against null crash in fetchDiskCacheEntries
>+        https://bugs.webkit.org/show_bug.cgi?id=197399
>+        <rdar://problem/47759337>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If the call to Entry::decodeStorageRecord returns nullptr when Cache::traverse's call to Storage::traverse has not received
>+        its final callback, it will call the Function from fetchDiskCacheEntries with nullptr twice.  That Function assumes that a
>+        call with nullptr indicates that it has completed traversal, so it calls its CompletionHandler.  If we get a call indicating
>+        that decoding has failed, don't call the CompletionHandler.  Wait until we are done traversing.
>+
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::fetchDiskCacheEntries):
>+
> 2019-04-29  Truitt Savell  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r244755.
>Index: Source/WebKit/NetworkProcess/NetworkProcess.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/NetworkProcess.cpp	(revision 244761)
>+++ Source/WebKit/NetworkProcess/NetworkProcess.cpp	(working copy)
>@@ -1269,25 +1269,30 @@ static void fetchDiskCacheEntries(Networ
>     }
>     
>     HashMap<SecurityOriginData, uint64_t> originsAndSizes;
>-    cache->traverse([fetchOptions, completionHandler = WTFMove(completionHandler), originsAndSizes = WTFMove(originsAndSizes)](auto* traversalEntry) mutable {
>-        if (!traversalEntry) {
>+    cache->traverse([fetchOptions, completionHandler = WTFMove(completionHandler), originsAndSizes = WTFMove(originsAndSizes)](auto traversalEntry) mutable {
>+        if (!traversalEntry.has_value()) {
>             Vector<WebsiteData::Entry> entries;
>-
>-            for (auto& originAndSize : originsAndSizes)
>-                entries.append(WebsiteData::Entry { originAndSize.key, WebsiteDataType::DiskCache, originAndSize.value });
>-
>-            RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), entries = WTFMove(entries)] () mutable {
>-                completionHandler(entries);
>-            });
>-
>+            switch (traversalEntry.error()) {
>+            case NetworkCache::Cache::TraversalResult::DecodeError:
>+                ASSERT_NOT_REACHED();
>+                break;
>+            case NetworkCache::Cache::TraversalResult::FinishedTraversing: {
>+                entries.reserveInitialCapacity(originsAndSizes.size());
>+                for (auto& originAndSize : originsAndSizes)
>+                    entries.uncheckedAppend(WebsiteData::Entry { originAndSize.key, WebsiteDataType::DiskCache, originAndSize.value });
>+                RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), entries = WTFMove(entries)] () mutable {
>+                    completionHandler(entries);
>+                });
>+            }
>+            }
>             return;
>         }
> 
>-        auto url = traversalEntry->entry.response().url();
>-        auto result = originsAndSizes.add({url.protocol().toString(), url.host().toString(), url.port()}, 0);
>+        auto url = traversalEntry->get().entry.response().url();
>+        auto result = originsAndSizes.add({ url.protocol().toString(), url.host().toString(), url.port() }, 0);
> 
>         if (fetchOptions.contains(WebsiteDataFetchOption::ComputeSizes))
>-            result.iterator->value += traversalEntry->entry.sourceStorageRecord().header.size() + traversalEntry->recordInfo.bodySize;
>+            result.iterator->value += traversalEntry->get().entry.sourceStorageRecord().header.size() + traversalEntry->get().recordInfo.bodySize;
>     });
> }
> 
>@@ -1444,15 +1449,21 @@ static void clearDiskCacheEntries(Networ
>         originsToDelete.add(origin.securityOrigin());
> 
>     Vector<NetworkCache::Key> cacheKeysToDelete;
>-    cache->traverse([cache, completionHandler = WTFMove(completionHandler), originsToDelete = WTFMove(originsToDelete), cacheKeysToDelete = WTFMove(cacheKeysToDelete)](auto* traversalEntry) mutable {
>-        if (traversalEntry) {
>-            if (originsToDelete.contains(SecurityOrigin::create(traversalEntry->entry.response().url())))
>-                cacheKeysToDelete.append(traversalEntry->entry.key());
>+    cache->traverse([cache, completionHandler = WTFMove(completionHandler), originsToDelete = WTFMove(originsToDelete), cacheKeysToDelete = WTFMove(cacheKeysToDelete)](auto traversalEntry) mutable {
>+        if (traversalEntry.has_value()) {
>+            if (originsToDelete.contains(SecurityOrigin::create(traversalEntry->get().entry.response().url())))
>+                cacheKeysToDelete.append(traversalEntry->get().entry.key());
>             return;
>         }
> 
>-        cache->remove(cacheKeysToDelete, WTFMove(completionHandler));
>-        return;
>+        switch (traversalEntry.error()) {
>+        case NetworkCache::Cache::TraversalResult::DecodeError:
>+            ASSERT_NOT_REACHED();
>+            break;
>+        case NetworkCache::Cache::TraversalResult::FinishedTraversing:
>+            cache->remove(cacheKeysToDelete, WTFMove(completionHandler));
>+            break;
>+        }
>     });
> }
> 
>Index: Source/WebKit/NetworkProcess/cache/NetworkCache.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/cache/NetworkCache.cpp	(revision 244759)
>+++ Source/WebKit/NetworkProcess/cache/NetworkCache.cpp	(working copy)
>@@ -455,7 +455,7 @@ void Cache::remove(const Vector<Key>& ke
>     m_storage->remove(keys, WTFMove(completionHandler));
> }
> 
>-void Cache::traverse(Function<void(const TraversalEntry*)>&& traverseHandler)
>+void Cache::traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&& traverseHandler)
> {
>     // Protect against clients making excessive traversal requests.
>     const unsigned maximumTraverseCount = 3;
>@@ -463,7 +463,7 @@ void Cache::traverse(Function<void(const
>         WTFLogAlways("Maximum parallel cache traverse count exceeded. Ignoring traversal request.");
> 
>         RunLoop::main().dispatch([traverseHandler = WTFMove(traverseHandler)] () mutable {
>-            traverseHandler(nullptr);
>+            traverseHandler(makeUnexpected(TraversalResult::FinishedTraversing));
>         });
>         return;
>     }
>@@ -473,18 +473,18 @@ void Cache::traverse(Function<void(const
>     m_storage->traverse(resourceType(), { }, [this, protectedThis = makeRef(*this), traverseHandler = WTFMove(traverseHandler)] (const Storage::Record* record, const Storage::RecordInfo& recordInfo) mutable {
>         if (!record) {
>             --m_traverseCount;
>-            traverseHandler(nullptr);
>+            traverseHandler(makeUnexpected(TraversalResult::FinishedTraversing));
>             return;
>         }
> 
>         auto entry = Entry::decodeStorageRecord(*record);
>         if (!entry) {
>-            traverseHandler(nullptr);
>+            traverseHandler(makeUnexpected(TraversalResult::DecodeError));
>             return;
>         }
> 
>         TraversalEntry traversalEntry { *entry, recordInfo };
>-        traverseHandler(&traversalEntry);
>+        traverseHandler({ traversalEntry });
>     });
> }
> 
>Index: Source/WebKit/NetworkProcess/cache/NetworkCache.h
>===================================================================
>--- Source/WebKit/NetworkProcess/cache/NetworkCache.h	(revision 244759)
>+++ Source/WebKit/NetworkProcess/cache/NetworkCache.h	(working copy)
>@@ -120,7 +120,8 @@ public:
>         const Entry& entry;
>         const Storage::RecordInfo& recordInfo;
>     };
>-    void traverse(Function<void(const TraversalEntry*)>&&);
>+    enum class TraversalResult : bool { FinishedTraversing, DecodeError };
>+    void traverse(Function<void(Expected<std::reference_wrapper<const TraversalEntry>, TraversalResult>&&)>&&);
>     void remove(const Key&);
>     void remove(const WebCore::ResourceRequest&);
>     void remove(const Vector<Key>&, Function<void()>&&);

Actions: View | Formatted Diff | Diff

Attachments on bug 197399: 368530 | 368537 | 368707