Bug 1460590 part 1 - Fix our parsing of Server-Timing. r?valentin draft
authorNicholas Hurley <hurley@mozilla.com>
Fri, 01 Jun 2018 11:39:49 -0700
changeset 802951 9845c96c58c98f36dd6c0ca1e92593860e24bf78
parent 802711 9900cebb1f9000bd05731ba67736b7c51f7eb812
child 802952 88176655db1e9802cf2e5bcc89d640197a064de3
push id112007
push userbmo:hurley@mozilla.com
push dateFri, 01 Jun 2018 18:41:19 +0000
reviewersvalentin
bugs1460590
milestone62.0a1
Bug 1460590 part 1 - Fix our parsing of Server-Timing. r?valentin We were erroneously looking for the first reasonably-valued server-timing-param for each name. However, that's not how it works. We should really be looking for the first server-timing-param regardless of how reasonable its value is. MozReview-Commit-ID: LwaHFyCpteU
netwerk/protocol/http/nsServerTiming.cpp
netwerk/test/gtest/TestServerTimingHeader.cpp
testing/web-platform/meta/server-timing/server_timing_header-parsing.https.html.ini
--- a/netwerk/protocol/http/nsServerTiming.cpp
+++ b/netwerk/protocol/http/nsServerTiming.cpp
@@ -74,25 +74,33 @@ ServerTimingParser::Parse()
          ++pairIndex) {
       nsDependentCSubstring &currentName =
         parsedHeader.mValues[index].mValues[pairIndex].mName;
       nsDependentCSubstring &currentValue =
         parsedHeader.mValues[index].mValues[pairIndex].mValue;
 
       // We should only take the value from the first
       // occurrence of server-timing-param-name ("dur" and "desc").
+      // This is true whether or not the value makes any sense (or, indeed, if
+      // there even is a value).
       if (currentName.LowerCaseEqualsASCII("dur") &&
-          currentValue.BeginReading() &&
           !foundDuration) {
-        timingHeader->SetDuration(ParseDouble(currentValue));
+        if (currentValue.BeginReading()) {
+          timingHeader->SetDuration(ParseDouble(currentValue));
+        } else {
+          timingHeader->SetDuration(0.0);
+        }
         foundDuration = true;
       } else if (currentName.LowerCaseEqualsASCII("desc") &&
-                 !currentValue.IsEmpty() &&
                  !foundDescription) {
-        timingHeader->SetDescription(currentValue);
+        if (!currentValue.IsEmpty()) {
+          timingHeader->SetDescription(currentValue);
+        } else {
+          timingHeader->SetDescription(EmptyCString());
+        }
         foundDescription = true;
       }
 
       if (foundDuration && foundDescription) {
         break;
       }
     }
   }
--- a/netwerk/test/gtest/TestServerTimingHeader.cpp
+++ b/netwerk/test/gtest/TestServerTimingHeader.cpp
@@ -172,29 +172,29 @@ TEST(TestServerTimingHeader, HeaderParsi
   // duplicate param names
   testServerTimingHeader("metric;dur=123.4;dur=567.8",
                          {{"metric", "123.4", ""}});
   testServerTimingHeader("metric;desc=description1;desc=description2",
                          {{"metric", "0", "description1"}});
   testServerTimingHeader("metric;dur=foo;dur=567.8", {{"metric", "", ""}});
 
   // unspecified param values
-  testServerTimingHeader("metric;dur;dur=123.4", {{"metric", "123.4", ""}});
+  testServerTimingHeader("metric;dur;dur=123.4", {{"metric", "0", ""}});
   testServerTimingHeader("metric;desc;desc=description",
-                         {{"metric", "0", "description"}});
+                         {{"metric", "0", ""}});
 
   // param name case
   testServerTimingHeader("metric;DuR=123.4;DeSc=description",
                          {{"metric", "123.4", "description"}});
 
   // nonsense
   testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
-                         {{"metric", "123.4", ""}, {"metric2", "0", ""}});
+                         {{"metric", "0", ""}, {"metric2", "0", ""}});
   testServerTimingHeader("metric\"foo;dur;dur=123.4,metric2",
-                         {{"metric", "", ""}});
+                         {{"metric", "0", ""}});
 
   // nonsense - return zero entries
   testServerTimingHeader(" ", {});
   testServerTimingHeader("=", {});
   testServerTimingHeader("[", {});
   testServerTimingHeader("]", {});
   testServerTimingHeader(";", {});
   testServerTimingHeader(",", {});
@@ -224,9 +224,9 @@ TEST(TestServerTimingHeader, HeaderParsi
   testServerTimingHeader(" total;dur=123.4 ",
                          {{"total", "123.4", ""}});
 
   // test cases for comma in quoted string
   testServerTimingHeader("     metric ; desc=\"descr\\\"\\\";,=iption\";dur=123.4",
                          {{"metric", "123.4", "descr\"\";,=iption"}});
   testServerTimingHeader(" metric2;dur=\"123.4\";;desc=\",;\\\",;,\";;,  metric  ;  desc = \" \\\", ;\\\" \"; dur=123.4,",
                          {{"metric2", "123.4", ",;\",;,"}, {"metric", "123.4", " \", ;\" "}});
-}
\ No newline at end of file
+}
--- a/testing/web-platform/meta/server-timing/server_timing_header-parsing.https.html.ini
+++ b/testing/web-platform/meta/server-timing/server_timing_header-parsing.https.html.ini
@@ -1,28 +1,10 @@
 [server_timing_header-parsing.https.html]
   [Untitled]
     expected: FAIL
 
-  [59.js - count (0 ?== 1)]
-    expected: FAIL
-
-  [59.js - duration (0 ?== 123.4)]
-    expected: FAIL
-
-  [60.js - count (0 ?== 1)]
-    expected: FAIL
-
-  [60.js - duration (0 ?== 123.4)]
-    expected: FAIL
-
-  [61.js - description ( ?== description)]
-    expected: FAIL
-
-  [62.js - description ( ?== description)]
-    expected: FAIL
-
   [67.js - duration (0 ?== 123.4)]
     expected: FAIL
 
   [68.js - count (2 ?== 1)]
     expected: FAIL