Attachment 343259 Details for Bug 186890 – Patch

[patch] Patch

bug-186890-20180621125346.patch (text/plain), 6.64 KB, created by Chris Dumez on 2018-06-21 12:53:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-21 12:53:47 PDT

Size: 6.64 KB

patch

obsolete

>Subversion Revision: 233045
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2edaebb382ec7000e2018424f76c4089d66e01f6..5bac0ae16753e6f45af5761a140b3e2cab0a3016 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-06-21  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix encoding / decoding issues in ResourceLoadStatistics
>+        https://bugs.webkit.org/show_bug.cgi?id=186890
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * loader/ResourceLoadStatistics.cpp:
>+        (WebCore::encodeHashCountedSet):
>+        (WebCore::encodeHashSet):
>+        Do not return early if the container we're trying to encode is empty. Instead,
>+        have the encoder encode an empty array. This is important for encoding / decoding
>+        to be fully symmetric. Otherwise, when trying to decode one of these empty containers,
>+        the decoder would fail (silently since we were ignoring decoding errors). Worse, the
>+        decoder might succeed but actually be decoding the *next* container in the file, since
>+        we have several HashCountedSets / HashSets encoded one after another.
>+
>+        (WebCore::decodeHashCountedSet):
>+        (WebCore::decodeHashSet):
>+        Return a boolean to indicate if the decoding suceeded or not.
>+
>+        (WebCore::ResourceLoadStatistics::decode):
>+        Check for container decoding errors and return false when decoding fails.
>+        Otherwise, we would just silently keep going.
>+
> 2018-06-21  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Bad optional access in WebCore::ContentSecurityPolicySource::portMatches
>diff --git a/Source/WebCore/loader/ResourceLoadStatistics.cpp b/Source/WebCore/loader/ResourceLoadStatistics.cpp
>index 40599d71f7551f6658510eca5ccd7f246ef52a78..a398baedb318c44e120d7f4f41559a8f537fc4b2 100644
>--- a/Source/WebCore/loader/ResourceLoadStatistics.cpp
>+++ b/Source/WebCore/loader/ResourceLoadStatistics.cpp
>@@ -40,9 +40,6 @@ typedef WTF::HashMap<String, unsigned, StringHash, HashTraits<String>, HashTrait
> 
> static void encodeHashCountedSet(KeyedEncoder& encoder, const String& label, const HashCountedSet<String>& hashCountedSet)
> {
>-    if (hashCountedSet.isEmpty())
>-        return;
>-
>     encoder.encodeObjects(label, hashCountedSet.begin(), hashCountedSet.end(), [](KeyedEncoder& encoderInner, const ResourceLoadStatisticsValue& origin) {
>         encoderInner.encodeString("origin", origin.key);
>         encoderInner.encodeUInt32("count", origin.value);
>@@ -51,9 +48,6 @@ static void encodeHashCountedSet(KeyedEncoder& encoder, const String& label, con
> 
> static void encodeHashSet(KeyedEncoder& encoder, const String& label, const HashSet<String>& hashSet)
> {
>-    if (hashSet.isEmpty())
>-        return;
>-    
>     encoder.encodeObjects(label, hashSet.begin(), hashSet.end(), [](KeyedEncoder& encoderInner, const String& origin) {
>         encoderInner.encodeString("origin", origin);
>     });
>@@ -94,10 +88,10 @@ void ResourceLoadStatistics::encode(KeyedEncoder& encoder) const
>     encoder.encodeUInt32("timesAccessedAsFirstPartyDueToStorageAccessAPI", timesAccessedAsFirstPartyDueToStorageAccessAPI);
> }
> 
>-static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<String>& hashCountedSet)
>+static bool decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<String>& hashCountedSet)
> {
>     Vector<String> ignore;
>-    decoder.decodeObjects(label, ignore, [&hashCountedSet](KeyedDecoder& decoderInner, String& origin) {
>+    return decoder.decodeObjects(label, ignore, [&hashCountedSet](KeyedDecoder& decoderInner, String& origin) {
>         if (!decoderInner.decodeString("origin", origin))
>             return false;
>         
>@@ -110,16 +104,17 @@ static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, Has
>     });
> }
> 
>-static void decodeHashSet(KeyedDecoder& decoder, const String& label, HashSet<String>& hashSet)
>+static bool decodeHashSet(KeyedDecoder& decoder, const String& label, HashSet<String>& hashSet)
> {
>-    Vector<String> ignore;
>-    decoder.decodeObjects(label, ignore, [&hashSet](KeyedDecoder& decoderInner, String& origin) {
>-        if (!decoderInner.decodeString("origin", origin))
>-            return false;
>-        
>-        hashSet.add(origin);
>-        return true;
>+    Vector<String> origins;
>+    bool success = decoder.decodeObjects(label, origins, [](KeyedDecoder& decoderInner, String& origin) {
>+        return decoderInner.decodeString("origin", origin);
>     });
>+    if (!success)
>+        return false;
>+
>+    hashSet.add(origins.begin(), origins.end());
>+    return true;
> }
> 
> bool ResourceLoadStatistics::decode(KeyedDecoder& decoder, unsigned modelVersion)
>@@ -132,22 +127,33 @@ bool ResourceLoadStatistics::decode(KeyedDecoder& decoder, unsigned modelVersion
>         return false;
> 
>     // Storage access
>-    decodeHashSet(decoder, "storageAccessUnderTopFrameOrigins", storageAccessUnderTopFrameOrigins);
>+    if (!decodeHashSet(decoder, "storageAccessUnderTopFrameOrigins", storageAccessUnderTopFrameOrigins))
>+        return false;
> 
>     // Top frame stats
>     if (modelVersion >= 11) {
>-        decodeHashCountedSet(decoder, "topFrameUniqueRedirectsTo", topFrameUniqueRedirectsTo);
>-        decodeHashCountedSet(decoder, "topFrameUniqueRedirectsFrom", topFrameUniqueRedirectsFrom);
>+        if (!decodeHashCountedSet(decoder, "topFrameUniqueRedirectsTo", topFrameUniqueRedirectsTo))
>+            return false;
>+
>+        if (!decodeHashCountedSet(decoder, "topFrameUniqueRedirectsFrom", topFrameUniqueRedirectsFrom))
>+            return false;
>     }
> 
>     // Subframe stats
>-    decodeHashCountedSet(decoder, "subframeUnderTopFrameOrigins", subframeUnderTopFrameOrigins);
>+    if (!decodeHashCountedSet(decoder, "subframeUnderTopFrameOrigins", subframeUnderTopFrameOrigins))
>+        return false;
>     
>     // Subresource stats
>-    decodeHashCountedSet(decoder, "subresourceUnderTopFrameOrigins", subresourceUnderTopFrameOrigins);
>-    decodeHashCountedSet(decoder, "subresourceUniqueRedirectsTo", subresourceUniqueRedirectsTo);
>-    if (modelVersion >= 11)
>-        decodeHashCountedSet(decoder, "subresourceUniqueRedirectsFrom", subresourceUniqueRedirectsFrom);
>+    if (!decodeHashCountedSet(decoder, "subresourceUnderTopFrameOrigins", subresourceUnderTopFrameOrigins))
>+        return false;
>+
>+    if (!decodeHashCountedSet(decoder, "subresourceUniqueRedirectsTo", subresourceUniqueRedirectsTo))
>+        return false;
>+
>+    if (modelVersion >= 11) {
>+        if (!decodeHashCountedSet(decoder, "subresourceUniqueRedirectsFrom", subresourceUniqueRedirectsFrom))
>+            return false;
>+    }
> 
>     // Prevalent Resource
>     if (!decoder.decodeBool("isPrevalentResource", isPrevalentResource))

Actions: View | Formatted Diff | Diff

Attachments on bug 186890: 343259 | 343296