Bug 1551399 part 2. Stop using [array] in url-classifier's makeFindFullHashRequestV4. r=dimi,gcp
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 14 May 2019 09:57:16 +0000
changeset 473761 3d3527abff681615f5a3dde9bd844b60b80f292d
parent 473760 d4ee430e90c2c4b21d13b2f1de063928f1b53d1d
child 473762 7abc1485ca90e0283665aa49a73aff7578f20d6e
push id36013
push usercsabou@mozilla.com
push dateTue, 14 May 2019 16:01:08 +0000
treeherdermozilla-central@230016dbba05 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdimi, gcp
bugs1551399
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551399 part 2. Stop using [array] in url-classifier's makeFindFullHashRequestV4. r=dimi,gcp Differential Revision: https://phabricator.services.mozilla.com/D31022
toolkit/components/url-classifier/UrlClassifierHashCompleter.jsm
toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js
toolkit/components/url-classifier/tests/unit/test_platform_specific_threats.js
--- a/toolkit/components/url-classifier/UrlClassifierHashCompleter.jsm
+++ b/toolkit/components/url-classifier/UrlClassifierHashCompleter.jsm
@@ -525,19 +525,17 @@ HashCompleterRequest.prototype = {
     let prefixArray = Array.from(prefixSet).sort();
 
     log("Build v4 gethash request with " + JSON.stringify(tableNameArray) + ", "
                                          + JSON.stringify(stateArray) + ", "
                                          + JSON.stringify(prefixArray));
 
     return gUrlUtil.makeFindFullHashRequestV4(tableNameArray,
                                               stateArray,
-                                              prefixArray,
-                                              tableNameArray.length,
-                                              prefixArray.length);
+                                              prefixArray);
   },
 
   // Returns a string for the request body based on the contents of
   // this._requests.
   buildRequest: function HCR_buildRequest() {
     // Sometimes duplicate entries are sent to HashCompleter but we do not need
     // to propagate these to the server. (bug 633644)
     let prefixes = [];
--- a/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
+++ b/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
@@ -119,26 +119,24 @@ interface nsIUrlClassifierUtils : nsISup
                                in Array<ACString> aStatesBase64);
 
     /**
    * Make "find full hash" request by for the given prefixes.
    *
    * @param aListNames An array of list names represented in string.
    * @param aListStatesBase64 An array of list states represented in base64.
    * @param aPrefixes An array of prefixes for which we'd like to find full hashes..
-   * @param aListCount The array length of aListNames
-   * @param aPrefixCount The array length of aPrefixes
+   *
+   * The aListNames and aListStatesBase64 arrays must be the same length.
    *
    * @returns A base64url encoded string.
    */
-  ACString makeFindFullHashRequestV4([array, size_is(aListCount)] in string aListNames,
-                                     [array, size_is(aListCount)] in string aListStatesBase64,
-                                     [array, size_is(aPrefixCount)] in string aPrefixes,
-                                     in uint32_t aListCount,
-                                     in uint32_t aPrefixCount);
+  ACString makeFindFullHashRequestV4(in Array<ACString> aListNames,
+                                     in Array<ACString> aListStatesBase64,
+                                     in Array<ACString> aPrefixes);
 
   /**
    * Make ThreatHit report request body.
    *
    * @param aChannel channel which encountered the threat.
    * @param aListName listname represented in string.
    * @param aHashBase64 hash-based hit represented in base64.
    *
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -433,76 +433,77 @@ nsUrlClassifierUtils::MakeUpdateRequestV
   NS_ENSURE_SUCCESS(rv, rv);
 
   aRequest = out;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsUrlClassifierUtils::MakeFindFullHashRequestV4(const char** aListNames,
-                                                const char** aListStatesBase64,
-                                                const char** aPrefixesBase64,
-                                                uint32_t aListCount,
-                                                uint32_t aPrefixCount,
-                                                nsACString& aRequest) {
+nsUrlClassifierUtils::MakeFindFullHashRequestV4(
+    const nsTArray<nsCString>& aListNames,
+    const nsTArray<nsCString>& aListStatesBase64,
+    const nsTArray<nsCString>& aPrefixesBase64, nsACString& aRequest) {
+  if (aListNames.Length() != aListStatesBase64.Length()) {
+    return NS_ERROR_INVALID_ARG;
+  }
+
   FindFullHashesRequest r;
   r.set_allocated_client(CreateClientInfo());
 
   nsresult rv;
 
   //-------------------------------------------------------------------
   // Set up FindFullHashesRequest.threat_info.
   auto threatInfo = r.mutable_threat_info();
 
   PlatformType platform = GetPlatformType();
 
   // 1) Set threat types.
-  for (uint32_t i = 0; i < aListCount; i++) {
+  for (uint32_t i = 0; i < aListNames.Length(); i++) {
     // Add threat types.
     uint32_t threatType;
-    rv = ConvertListNameToThreatType(nsDependentCString(aListNames[i]),
-                                     &threatType);
+    rv = ConvertListNameToThreatType(aListNames[i], &threatType);
     NS_ENSURE_SUCCESS(rv, rv);
     if (!IsAllowedOnCurrentPlatform(threatType)) {
       NS_WARNING(
           nsPrintfCString(
               "Threat type %d (%s) is unsupported on current platform: %d",
-              threatType, aListNames[i], GetPlatformType())
+              threatType, aListNames[i].get(), GetPlatformType())
               .get());
       continue;
     }
     threatInfo->add_threat_types((ThreatType)threatType);
 
 #if defined(ANDROID)
     // Temporary hack to fix bug 1441345.
     if (((ThreatType)threatType == SOCIAL_ENGINEERING_PUBLIC) ||
         ((ThreatType)threatType == SOCIAL_ENGINEERING)) {
       platform = LINUX_PLATFORM;
     }
 #endif
 
     // Add client states for index 'i' only when the threat type is available
     // on current platform.
     nsCString stateBinary;
-    rv = Base64Decode(nsDependentCString(aListStatesBase64[i]), stateBinary);
+    rv = Base64Decode(aListStatesBase64[i], stateBinary);
     NS_ENSURE_SUCCESS(rv, rv);
     r.add_client_states(stateBinary.get(), stateBinary.Length());
   }
 
   // 2) Set platform type.
   threatInfo->add_platform_types(platform);
 
   // 3) Set threat entry type.
   threatInfo->add_threat_entry_types(URL);
 
   // 4) Set threat entries.
-  for (uint32_t i = 0; i < aPrefixCount; i++) {
+  for (const nsCString& prefix : aPrefixesBase64) {
     nsCString prefixBinary;
-    rv = Base64Decode(nsDependentCString(aPrefixesBase64[i]), prefixBinary);
+    rv = Base64Decode(prefix, prefixBinary);
     threatInfo->add_threat_entries()->set_hash(prefixBinary.get(),
                                                prefixBinary.Length());
   }
   //-------------------------------------------------------------------
 
   // Then serialize.
   std::string s;
   r.SerializeToString(&s);
--- a/toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
+++ b/toolkit/components/url-classifier/tests/gtest/TestFindFullHash.cpp
@@ -2,66 +2,46 @@
 #include "gtest/gtest.h"
 #include "nsUrlClassifierUtils.h"
 #include "mozilla/Base64.h"
 
 using namespace mozilla;
 using namespace mozilla::safebrowsing;
 
 namespace {
-
-// |Base64EncodedStringArray| and |MakeBase64EncodedStringArray|
-// works together to make us able to do things "literally" and easily.
-
-// Given a nsCString array, construct an object which can be implicitly
-// casted to |const char**|, where all owning c-style strings have been
-// base64 encoded. The memory life cycle of what the "cast operator"
-// returns is just as the object itself.
-class Base64EncodedStringArray {
- public:
-  Base64EncodedStringArray(nsCString aArray[], size_t N);
-  operator const char**() const { return (const char**)&mArray[0]; }
-
- private:
-  // Since we can't guarantee the layout of nsCString (can we?),
-  // an additional nsTArray<nsCString> is required to manage the
-  // allocated string.
-  nsTArray<const char*> mArray;
-  nsTArray<nsCString> mStringStorage;
-};
-
-// Simply used to infer the fixed-array size automatically.
 template <size_t N>
-Base64EncodedStringArray MakeBase64EncodedStringArray(nsCString (&aArray)[N]) {
-  return Base64EncodedStringArray(aArray, N);
-}
-
+void ToBase64EncodedStringArray(nsCString (&aInput)[N],
+                                nsTArray<nsCString>& aEncodedArray);
 }  // end of unnamed namespace.
 
 TEST(UrlClassifierFindFullHash, Request)
 {
   nsUrlClassifierUtils* urlUtil = nsUrlClassifierUtils::GetInstance();
 
-  const char* listNames[] = {"test-phish-proto", "test-unwanted-proto"};
+  nsTArray<nsCString> listNames;
+  listNames.AppendElement("test-phish-proto");
+  listNames.AppendElement("test-unwanted-proto");
 
   nsCString listStates[] = {nsCString("sta\x00te1", 7),
                             nsCString("sta\x00te2", 7)};
+  nsTArray<nsCString> listStateArray;
+  ToBase64EncodedStringArray(listStates, listStateArray);
 
   nsCString prefixes[] = {nsCString("\x00\x00\x00\x01", 4),
                           nsCString("\x00\x00\x00\x00\x01", 5),
                           nsCString("\x00\xFF\x00\x01", 4),
                           nsCString("\x00\xFF\x00\x01\x11\x23\xAA\xBC", 8),
                           nsCString("\x00\x00\x00\x01\x00\x01\x98", 7)};
+  nsTArray<nsCString> prefixArray;
+  ToBase64EncodedStringArray(prefixes, prefixArray);
 
   nsCString requestBase64;
   nsresult rv;
-  rv = urlUtil->MakeFindFullHashRequestV4(
-      listNames, MakeBase64EncodedStringArray(listStates),
-      MakeBase64EncodedStringArray(prefixes), ArrayLength(listNames),
-      ArrayLength(prefixes), requestBase64);
+  rv = urlUtil->MakeFindFullHashRequestV4(listNames, listStateArray,
+                                          prefixArray, requestBase64);
   ASSERT_TRUE(NS_SUCCEEDED(rv));
 
   // Base64 URL decode first.
   FallibleTArray<uint8_t> requestBinary;
   rv = Base64URLDecode(requestBase64, Base64URLDecodePaddingPolicy::Require,
                        requestBinary);
   ASSERT_TRUE(NS_SUCCEEDED(rv));
 
@@ -77,18 +57,18 @@ TEST(UrlClassifierFindFullHash, Request)
   }
 
   auto threatInfo = r.threat_info();
 
   // Compare threat types.
   ASSERT_EQ(threatInfo.threat_types_size(), (int)ArrayLength(listStates));
   for (int i = 0; i < threatInfo.threat_types_size(); i++) {
     uint32_t expectedThreatType;
-    rv = urlUtil->ConvertListNameToThreatType(nsCString(listNames[i]),
-                                              &expectedThreatType);
+    rv =
+        urlUtil->ConvertListNameToThreatType(listNames[i], &expectedThreatType);
     ASSERT_TRUE(NS_SUCCEEDED(rv));
     ASSERT_EQ(threatInfo.threat_types(i), expectedThreatType);
   }
 
   // Compare prefixes.
   ASSERT_EQ(threatInfo.threat_entries_size(), (int)ArrayLength(prefixes));
   for (int i = 0; i < threatInfo.threat_entries_size(); i++) {
     auto p = threatInfo.threat_entries(i).hash();
@@ -223,20 +203,20 @@ TEST(UrlClassifierFindFullHash, ParseReq
   NS_ENSURE_SUCCESS_VOID(rv);
 
   ASSERT_EQ(callbackCount, ArrayLength(EXPECTED_MATCH));
 }
 
 /////////////////////////////////////////////////////////////
 namespace {
 
-Base64EncodedStringArray::Base64EncodedStringArray(nsCString aArray[],
-                                                   size_t N) {
+template <size_t N>
+void ToBase64EncodedStringArray(nsCString (&aArray)[N],
+                                nsTArray<nsCString>& aEncodedArray) {
   for (size_t i = 0; i < N; i++) {
     nsCString encoded;
     nsresult rv = Base64Encode(aArray[i], encoded);
     NS_ENSURE_SUCCESS_VOID(rv);
-    mStringStorage.AppendElement(encoded);
-    mArray.AppendElement(encoded.get());
+    aEncodedArray.AppendElement(encoded);
   }
 }
 
 }  // namespace
--- a/toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js
+++ b/toolkit/components/url-classifier/tests/unit/test_hashcompleter_v4.js
@@ -77,19 +77,17 @@ add_test(function test_update_v4() {
   // Force table update.
   Services.prefs.setCharPref(PREF_NEXTUPDATETIME_V4, "1");
   gListManager.maybeToggleUpdateChecking();
 });
 
 add_test(function test_getHashRequestV4() {
   let request = gUrlUtil.makeFindFullHashRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                    [btoa(NEW_CLIENT_STATE)],
-                                                   [btoa("0123"), btoa("1234567"), btoa("1111")].sort(),
-                                                   1,
-                                                   3);
+                                                   [btoa("0123"), btoa("1234567"), btoa("1111")].sort());
   registerHandlerGethashV4("&$req=" + request);
   let completeFinishedCnt = 0;
 
   gCompleter.complete("0123", TEST_TABLE_DATA_V4.gethashUrl, TEST_TABLE_DATA_V4.tableName, {
     completionV4(hash, table, duration, fullhashes) {
       equal(hash, "0123");
       equal(table, TEST_TABLE_DATA_V4.tableName);
       equal(duration, 120);
@@ -182,19 +180,17 @@ add_test(function test_minWaitDuration()
         equal(status, Cr.NS_OK);
         run_next_test();
       },
     });
   };
 
   let request = gUrlUtil.makeFindFullHashRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                    [btoa(NEW_CLIENT_STATE)],
-                                                   [btoa("1234567")],
-                                                   1,
-                                                   1);
+                                                   [btoa("1234567")]);
   registerHandlerGethashV4("&$req=" + request);
 
   // The last gethash response contained a min wait duration 12 secs 10 nano
   // So subsequent requests can happen only after the min wait duration
   do_timeout(1000, failedComplete);
   do_timeout(2000, failedComplete);
   do_timeout(4000, failedComplete);
   do_timeout(13000, successComplete);
--- a/toolkit/components/url-classifier/tests/unit/test_platform_specific_threats.js
+++ b/toolkit/components/url-classifier/tests/unit/test_platform_specific_threats.js
@@ -26,24 +26,22 @@ function testMobileOnlyThreats() {
             "PHA (i.e. goog-harmful-proto) should be filtered on non-mobile platform.");
     }
   })();
 
   (function testFullHashRequest() {
     let requestWithPHA =
       urlUtils.makeFindFullHashRequestV4(["goog-phish-proto", "goog-harmful-proto"],
                                          ["", ""], // state.
-                                         [btoa("0123")], // prefix.
-                                         2, 1);
+                                         [btoa("0123")]); // prefix.
 
     let requestNoPHA =
       urlUtils.makeFindFullHashRequestV4(["goog-phish-proto"],
                                          [""], // state.
-                                         [btoa("0123")], // prefix.
-                                         1, 1);
+                                         [btoa("0123")]); // prefix.
 
     if (AppConstants.platform === "android") {
       notEqual(requestWithPHA, requestNoPHA,
                "PHA (i.e. goog-harmful-proto) shouldn't be filtered on mobile platform.");
     } else {
       equal(requestWithPHA, requestNoPHA,
             "PHA (i.e. goog-harmful-proto) should be filtered on non-mobile platform.");
     }