Bug 1331881 - Minimum wait duration and negative cache duration should be passed even if there is no match. r?hchang draft
authorDimiL <dlee@mozilla.com>
Wed, 18 Jan 2017 17:34:03 +0800
changeset 462951 83501957badf6a0cd8af935a6bfa780c89955efe
parent 462769 80eac484366ad881c6a10bf81e8d9b8f7a676c75
child 542540 7b5755c2574c9d5a6d7ae32da8afc140ab64e5a2
push id41918
push userdlee@mozilla.com
push dateWed, 18 Jan 2017 09:34:35 +0000
reviewershchang
bugs1331881
milestone53.0a1
Bug 1331881 - Minimum wait duration and negative cache duration should be passed even if there is no match. r?hchang MozReview-Commit-ID: K5RcpmiXFYR
toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
--- a/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
+++ b/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
@@ -6,35 +6,45 @@
 /**
  * Some utility methods used by the url classifier.
  */
 
 interface nsIURI;
 
 /**
  * Interface for parseFindFullHashResponseV4 callback
- *
- * @param aCompleteHash A 32-byte complete hash string.
- * @param aTableNames The table names that this complete hash is associated with.
- *                    Since the server responded with a threat type, multiple
- *                    list names can be returned. The caller is reponsible
- *                    for filtering out the unrequested table names.
- *                    See |convertThreatTypeToListNames| for the format.
- * @param aMinWaitDuration See "FindFullHashesResponse" in safebrowsing.proto.
- * @param aNegCacheDuration See "FindFullHashesResponse" in safebrowsing.proto.
- * @param aPerHashCacheDuration See "FindFullHashesResponse" in safebrowsing.proto.
- *
  */
-[scriptable, function, uuid(fbb9684a-a0aa-11e6-88b0-08606e456b8a)]
+[scriptable, uuid(fbb9684a-a0aa-11e6-88b0-08606e456b8a)]
 interface nsIUrlClassifierParseFindFullHashCallback : nsISupports {
+  /**
+   * Callback when a match is found in full hash response. This callback may be
+   * called multiple times when there are more than one matches in response.
+   *
+   * @param aCompleteHash A 32-byte complete hash string.
+   * @param aTableNames The table names that this complete hash is associated with.
+   *                    Since the server responded with a threat type, multiple
+   *                    list names can be returned. The caller is reponsible
+   *                    for filtering out the unrequested table names.
+   *                    See |convertThreatTypeToListNames| for the format.
+   * @param aPerHashCacheDuration See "FindFullHashesResponse" in safebrowsing.proto.
+   *
+   */
   void onCompleteHashFound(in ACString aCompleteHash,
                            in ACString aTableNames,
-                           in unsigned long aMinWaitDuration,
-                           in unsigned long aNegCacheDuration,
                            in unsigned long aPerHashCacheDuration);
+
+  /**
+   * Callback when full hash response is received.
+   *
+   * @param aMinWaitDuration See "FindFullHashesResponse" in safebrowsing.proto.
+   * @param aNegCacheDuration See "FindFullHashesResponse" in safebrowsing.proto.
+   *
+   */
+  void onResponseParsed(in unsigned long aMinWaitDuration,
+                        in unsigned long aNegCacheDuration);
 };
 
 [scriptable, uuid(e4f0e59c-b922-48b0-a7b6-1735c1f96fed)]
 interface nsIUrlClassifierUtils : nsISupports
 {
   /**
    * Get the lookup string for a given URI.  This normalizes the hostname,
    * url-decodes the string, and strips off the protocol.
--- a/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
+++ b/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ -520,42 +520,49 @@ HashCompleterRequest.prototype = {
 
     let length = this._response.length;
     while (start != length) {
       start = this.handleTable(start);
     }
   },
 
   handleResponseV4: function HCR_handleResponseV4() {
-    let callback = (aCompleteHash,
-                    aTableNames,
-                    aMinWaitDuration,
-                    aNegCacheDuration,
-                    aPerHashCacheDuration) => {
-      log("V4 response callback: " + JSON.stringify(aCompleteHash) + ", " +
-          aTableNames + ", " +
-          aMinWaitDuration + ", " +
-          aNegCacheDuration + ", " +
-          aPerHashCacheDuration);
+    let callback = {
+      onCompleteHashFound : (aCompleteHash,
+                             aTableNames,
+                             aPerHashCacheDuration) => {
+        log("V4 fullhash response complete hash found callback: " +
+            JSON.stringify(aCompleteHash) + ", " +
+            aTableNames + ", CacheDuration(" + aPerHashCacheDuration + ")");
 
-      // Filter table names which we didn't requested.
-      let filteredTables = aTableNames.split(",").filter(name => {
-        return this.tableNames.get(name);
-      });
-      if (0 === filteredTables.length) {
-        log("ERROR: Got complete hash which is from unknown table.");
-        return;
-      }
-      if (filteredTables.length > 1) {
-        log("WARNING: Got complete hash which has ambigious threat type.");
-      }
+        // Filter table names which we didn't requested.
+        let filteredTables = aTableNames.split(",").filter(name => {
+          return this.tableNames.get(name);
+        });
+        if (0 === filteredTables.length) {
+          log("ERROR: Got complete hash which is from unknown table.");
+          return;
+        }
+        if (filteredTables.length > 1) {
+          log("WARNING: Got complete hash which has ambigious threat type.");
+        }
 
-      this.handleItem(aCompleteHash, filteredTables[0], 0);
+        this.handleItem(aCompleteHash, filteredTables[0], 0);
+
+        // TODO: Bug 1311935 - Implement v4 cache.
+      },
 
-      // TODO: Bug 1311935 - Implement v4 cache.
+      onResponseParsed : (aMinWaitDuration,
+                          aNegCacheDuration) => {
+        log("V4 fullhash response parsed callback: " +
+            "MinWaitDuration(" + aMinWaitDuration + "), " +
+            "NegativeCacheDuration(" + aNegCacheDuration + ")");
+
+        // TODO: Bug 1311935 - Implement v4 cache.
+      },
     };
 
     gUrlUtil.parseFindFullHashResponseV4(this._response, callback);
   },
 
   // This parses a table entry in the response body and calls |handleItem|
   // for complete hash in the table entry.
   handleTable: function HCR_handleTable(aStart) {
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -444,33 +444,35 @@ nsUrlClassifierUtils::ParseFindFullHashR
   if (!r.ParseFromArray(aResponse.BeginReading(), aResponse.Length())) {
     NS_WARNING("Invalid response");
     Telemetry::Accumulate(Telemetry::URLCLASSIFIER_COMPLETION_ERROR,
                           PARSING_FAILURE);
     return NS_ERROR_FAILURE;
   }
 
   bool hasUnknownThreatType = false;
-  auto minWaitDuration = DurationToMs(r.minimum_wait_duration());
-  auto negCacheDuration = DurationToMs(r.negative_cache_duration());
+
   for (auto& m : r.matches()) {
     nsCString tableNames;
     nsresult rv = ConvertThreatTypeToListNames(m.threat_type(), tableNames);
     if (NS_FAILED(rv)) {
       hasUnknownThreatType = true;
       continue; // Ignore un-convertable threat type.
     }
     auto& hash = m.threat().hash();
     aCallback->OnCompleteHashFound(nsCString(hash.c_str(), hash.length()),
                                    tableNames,
-                                   minWaitDuration,
-                                   negCacheDuration,
                                    DurationToMs(m.cache_duration()));
   }
 
+  auto minWaitDuration = DurationToMs(r.minimum_wait_duration());
+  auto negCacheDuration = DurationToMs(r.negative_cache_duration());
+
+  aCallback->OnResponseParsed(minWaitDuration, negCacheDuration);
+
   Telemetry::Accumulate(Telemetry::URLCLASSIFIER_COMPLETION_ERROR,
                         hasUnknownThreatType ? UNKNOWN_THREAT_TYPE : SUCCESS);
 
   return NS_OK;
 }
 
 //////////////////////////////////////////////////////////
 // nsIObserver
--- a/toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
+++ b/toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
@@ -141,51 +141,53 @@ public:
   explicit MyParseCallback(uint32_t& aCallbackCount)
     : mCallbackCount(aCallbackCount)
   {
   }
 
   NS_IMETHOD
   OnCompleteHashFound(const nsACString& aCompleteHash,
                       const nsACString& aTableNames,
-                      uint32_t aMinWaitDuration,
-                      uint32_t aNegCacheDuration,
                       uint32_t aPerHashCacheDuration) override
   {
     Verify(aCompleteHash,
            aTableNames,
-           aMinWaitDuration,
-           aNegCacheDuration,
            aPerHashCacheDuration);
 
     return NS_OK;
   }
 
+  NS_IMETHOD
+  OnResponseParsed(uint32_t aMinWaitDuration,
+                   uint32_t aNegCacheDuration) override
+  {
+    VerifyDuration(aMinWaitDuration, EXPECTED_MIN_WAIT_DURATION);
+    VerifyDuration(aNegCacheDuration, EXPECTED_NEG_CACHE_DURATION);
+
+    return NS_OK;
+  }
+
 private:
   void
   Verify(const nsACString& aCompleteHash,
          const nsACString& aTableNames,
-         uint32_t aMinWaitDuration,
-         uint32_t aNegCacheDuration,
          uint32_t aPerHashCacheDuration)
   {
     auto expected = EXPECTED_MATCH[mCallbackCount];
 
     ASSERT_TRUE(aCompleteHash.Equals(expected.mCompleteHash));
 
     // Verify aTableNames
     nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
       do_GetService("@mozilla.org/url-classifier/utils;1");
     nsCString tableNames;
     nsresult rv = urlUtil->ConvertThreatTypeToListNames(expected.mThreatType, tableNames);
     ASSERT_TRUE(NS_SUCCEEDED(rv));
     ASSERT_TRUE(aTableNames.Equals(tableNames));
 
-    VerifyDuration(aMinWaitDuration, EXPECTED_MIN_WAIT_DURATION);
-    VerifyDuration(aNegCacheDuration, EXPECTED_NEG_CACHE_DURATION);
     VerifyDuration(aPerHashCacheDuration, expected.mPerHashCacheDuration);
 
     mCallbackCount++;
   }
 
   void
   VerifyDuration(uint32_t aToVerify, const MyDuration& aExpected)
   {