[webkit-changes] [233309] trunk/Source/WebCore

Thu, 28 Jun 2018 10:08:49 -0700

Title: [233309] trunk/Source/WebCore
Revision
233309
Author
[email protected]
Date
2018-06-28 10:07:31 -0700 (Thu, 28 Jun 2018)

Log Message

Fix encoding / decoding issues in ResourceLoadStatistics
https://bugs.webkit.org/show_bug.cgi?id=186890

Reviewed by Brent Fulgham.

* 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.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (233308 => 233309)


--- trunk/Source/WebCore/ChangeLog	2018-06-28 17:03:24 UTC (rev 233308)
+++ trunk/Source/WebCore/ChangeLog	2018-06-28 17:07:31 UTC (rev 233309)
@@ -1,3 +1,28 @@
+2018-06-28  Chris Dumez  <[email protected]>
+
+        Fix encoding / decoding issues in ResourceLoadStatistics
+        https://bugs.webkit.org/show_bug.cgi?id=186890
+
+        Reviewed by Brent Fulgham.
+
+        * 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-28  Sihui Liu  <[email protected]>
 
         Cookie API: cookie creation time is wrong

Modified: trunk/Source/WebCore/loader/ResourceLoadStatistics.cpp (233308 => 233309)


--- trunk/Source/WebCore/loader/ResourceLoadStatistics.cpp	2018-06-28 17:03:24 UTC (rev 233308)
+++ trunk/Source/WebCore/loader/ResourceLoadStatistics.cpp	2018-06-28 17:07:31 UTC (rev 233309)
@@ -40,9 +40,6 @@
 
 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 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 @@
     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 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,23 +127,34 @@
         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))
         return false;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to