Backed out changeset d46155cc719e (bug 1305567) for various failures in browser-chrome, devtools, and xpcshell (e.g. test_streamupdater.js) tests. r=backout on a CLOSED TREE
authorSebastian Hengst <archaeopteryx@coole-files.de>
Thu, 29 Sep 2016 20:54:50 +0200
changeset 315791 e7aa4e7bb71e9ebbd428794c62c9ac5eba9da9c1
parent 315790 7e9f453f768f0fc533f6ace26b06bd8f8324543e
child 315792 c0122746911c6f83450b78eb236df7b961bca9a9
push id20634
push usercbook@mozilla.com
push dateFri, 30 Sep 2016 10:10:13 +0000
treeherderfx-team@afe79b010d13 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout
bugs1305567
milestone52.0a1
backs outd46155cc719e323b61da814f4db8e2e3bc07c9e3
Backed out changeset d46155cc719e (bug 1305567) for various failures in browser-chrome, devtools, and xpcshell (e.g. test_streamupdater.js) tests. r=backout on a CLOSED TREE
toolkit/components/url-classifier/content/listmanager.js
toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
toolkit/components/url-classifier/tests/unit/test_listmanager.js
--- a/toolkit/components/url-classifier/content/listmanager.js
+++ b/toolkit/components/url-classifier/content/listmanager.js
@@ -406,20 +406,21 @@ PROT_ListManager.prototype.makeUpdateReq
       // "saving states to HashStore".
       let statePrefName = "browser.safebrowsing.provider.google4.state." + listName;
       let stateBase64 = this.prefs_.getPref(statePrefName, "");
       stateArray.push(stateBase64);
     });
 
     let urlUtils = Cc["@mozilla.org/url-classifier/utils;1"]
                      .getService(Ci.nsIUrlClassifierUtils);
-
-    streamerMap.requestPayload = urlUtils.makeUpdateRequestV4(tableArray,
-                                                              stateArray,
-                                                              tableArray.length);
+    let requestPayload =  urlUtils.makeUpdateRequestV4(tableArray,
+                                                       stateArray,
+                                                       tableArray.length);
+    // Use a base64-encoded request.
+    streamerMap.requestPayload = btoa(requestPayload);
     streamerMap.isPostRequest = false;
   } else {
     // Build the request. For each table already in the database, include the
     // chunk data from the database
     var lines = tableData.split("\n");
     for (var i = 0; i < lines.length; i++) {
       var fields = lines[i].split(";");
       var name = fields[0];
--- a/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
+++ b/toolkit/components/url-classifier/nsIUrlClassifierUtils.idl
@@ -52,14 +52,14 @@ interface nsIUrlClassifierUtils : nsISup
 
   /**
    * Make update request for given lists and their states.
    *
    * @param aListNames An array of list name represented in string.
    * @param aState An array of states (encoded in base64 format) for each list.
    * @param aCount The array length of aList and aState.
    *
-   * @returns A base64url encoded string.
+   * @returns A string to store request. Not null-terminated.
    */
   ACString makeUpdateRequestV4([array, size_is(aCount)] in string aListNames,
                                [array, size_is(aCount)] in string aStatesBase64,
                                in uint32_t aCount);
 };
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -30,18 +30,16 @@ static const char* gQuitApplicationMessa
 
 // Limit the list file size to 32mb
 const uint32_t MAX_FILE_SIZE = (32 * 1024 * 1024);
 
 #undef LOG
 
 // MOZ_LOG=UrlClassifierStreamUpdater:5
 static mozilla::LazyLogModule gUrlClassifierStreamUpdaterLog("UrlClassifierStreamUpdater");
-
-#define LOG_ENABLED() MOZ_LOG_TEST(gUrlClassifierStreamUpdaterLog, mozilla::LogLevel::Debug)
 #define LOG(args) TrimAndLog args
 
 // Calls nsIURLFormatter::TrimSensitiveURLs to remove sensitive
 // info from the logging message.
 static void TrimAndLog(const char* aFmt, ...)
 {
   nsString raw;
 
@@ -675,23 +673,17 @@ nsUrlClassifierStreamUpdater::OnStartReq
     LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
 
     // It's possible for mDownloadErrorCallback to be null on shutdown.
     if (mDownloadErrorCallback) {
       mDownloadErrorCallback->HandleEvent(strStatus);
     }
 
     mDownloadError = true;
-
     status = NS_ERROR_ABORT;
-    if (LOG_ENABLED()) {
-      // We can't return an error just yet because we need to read the body
-      // in order to see the error message.
-      status = NS_OK;
-    }
   } else if (NS_SUCCEEDED(status)) {
     MOZ_ASSERT(mDownloadErrorCallback);
     mBeganStream = true;
     LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
     rv = mDBService->BeginStream(mStreamTable);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
@@ -702,55 +694,44 @@ nsUrlClassifierStreamUpdater::OnStartReq
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::OnDataAvailable(nsIRequest *request,
                                               nsISupports* context,
                                               nsIInputStream *aIStream,
                                               uint64_t aSourceOffset,
                                               uint32_t aLength)
 {
-  if (!mDBService && !mDownloadError) {
+  if (!mDBService)
     return NS_ERROR_NOT_INITIALIZED;
-  }
 
   LOG(("OnDataAvailable (%d bytes)", aLength));
 
   if (aSourceOffset > MAX_FILE_SIZE) {
     LOG(("OnDataAvailable::Abort because exceeded the maximum file size(%lld)", aSourceOffset));
     return NS_ERROR_FILE_TOO_BIG;
   }
 
   nsresult rv;
 
   // Copy the data into a nsCString
   nsCString chunk;
   rv = NS_ConsumeStream(aIStream, aLength, chunk);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  if (mDownloadError) {
-    mDownloadErrorMessage.Append(chunk);
-    return NS_OK;
-  }
-
   //LOG(("Chunk (%d): %s\n\n", chunk.Length(), chunk.get()));
   rv = mDBService->UpdateStream(chunk);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* context,
                                             nsresult aStatus)
 {
-  if (mDownloadError) {
-    LOG(("Download error message: %s", mDownloadErrorMessage.get()));
-    return NS_ERROR_ABORT; // The value doesn't matter.
-  }
-
   if (!mDBService)
     return NS_ERROR_NOT_INITIALIZED;
 
   LOG(("OnStopRequest (status %x, beganStream %s, this=%p)", aStatus,
        mBeganStream ? "true" : "false", this));
 
   nsresult rv;
 
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -93,13 +93,11 @@ private:
     nsCString mUrl;
     nsCString mTable;
   };
   nsTArray<PendingUpdate> mPendingUpdates;
 
   nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
-
-  nsCString mDownloadErrorMessage;
 };
 
 #endif // nsUrlClassifierStreamUpdater_h_
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -113,17 +113,17 @@ InitListUpdateRequest(ThreatType aThreat
   contraints->add_supported_compressions(RAW);
   aListUpdateRequest->set_allocated_constraints(contraints);
 
   // Only set non-empty state.
   if (aStateBase64[0] != '\0') {
     nsCString stateBinary;
     nsresult rv = Base64Decode(nsCString(aStateBase64), stateBinary);
     if (NS_SUCCEEDED(rv)) {
-      aListUpdateRequest->set_state(stateBinary.get(), stateBinary.Length());
+      aListUpdateRequest->set_state(stateBinary.get());
     }
   }
 }
 
 static ClientInfo*
 CreateClientInfo()
 {
   ClientInfo* c = new ClientInfo();
@@ -292,21 +292,17 @@ nsUrlClassifierUtils::MakeUpdateRequestV
     InitListUpdateRequest(static_cast<ThreatType>(threatType), aStatesBase64[i], lur);
   }
 
   // Then serialize.
   std::string s;
   r.SerializeToString(&s);
 
   nsCString out;
-  nsresult rv = Base64URLEncode(s.size(),
-                                (const uint8_t*)s.c_str(),
-                                Base64URLEncodePaddingPolicy::Include,
-                                out);
-  NS_ENSURE_SUCCESS(rv, rv);
+  out.Assign(s.c_str(), s.size());
 
   aRequest = out;
 
   return NS_OK;
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // non-interface methods
--- a/toolkit/components/url-classifier/tests/unit/test_listmanager.js
+++ b/toolkit/components/url-classifier/tests/unit/test_listmanager.js
@@ -67,19 +67,16 @@ let gHttpServV4 = null;
 // (in terms of "update URL") in test_update_all_tables().
 let gUpdatedCntForTableData = 0; // For TEST_TABLE_DATA_LIST.
 let gIsV4Updated = false;   // For TEST_TABLE_DATA_V4.
 
 const NEW_CLIENT_STATE = 'sta\0te';
 
 prefBranch.setBoolPref("browser.safebrowsing.debug", true);
 
-// The "\xFF\xFF" is to generate a base64 string with "/".
-prefBranch.setCharPref("browser.safebrowsing.id", "Firefox\xFF\xFF");
-
 // Register tables.
 TEST_TABLE_DATA_LIST.forEach(function(t) {
   gListManager.registerTable(t.tableName,
                              t.providerName,
                              t.updateUrl,
                              t.gethashUrl);
 });
 
@@ -151,17 +148,17 @@ const SERVER_INVOLVED_TEST_CASE_LIST = [
     gUpdateResponse = "n:1000\n";
 
     // We test the request against the query string since v4 request
     // would be appened to the query string. The request is generated
     // by protobuf API (binary) then encoded to base64 format.
     let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                   [""],
                                                   1);
-    gExpectedQueryV4 = "&$req=" + requestV4;
+    gExpectedQueryV4 = "&$req=" + btoa(requestV4);
 
     forceTableUpdate();
   },
 
 ];
 
 SERVER_INVOLVED_TEST_CASE_LIST.forEach(t => add_test(t));
 
@@ -171,17 +168,17 @@ add_test(function test_partialUpdateV4()
   gListManager.enableUpdate(TEST_TABLE_DATA_V4.tableName);
 
   // Since the new client state has been responded and saved in
   // test_update_all_tables, this update request should send
   // a partial update to the server.
   let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],
                                                 [btoa(NEW_CLIENT_STATE)],
                                                 1);
-  gExpectedQueryV4 = "&$req=" + requestV4;
+  gExpectedQueryV4 = "&$req=" + btoa(requestV4);
 
   forceTableUpdate();
 });
 
 // Tests nsIUrlListManager.getGethashUrl.
 add_test(function test_getGethashUrl() {
   TEST_TABLE_DATA_LIST.forEach(function (t) {
     equal(gListManager.getGethashUrl(t.tableName), t.gethashUrl);
@@ -239,18 +236,16 @@ function run_test() {
     // Not on the spec. Found in Chromium source code...
     equal(request.getHeader("X-HTTP-Method-Override"), "POST");
 
     // V4 update request uses GET.
     equal(request.method, "GET");
 
     // V4 append the base64 encoded request to the query string.
     equal(request.queryString, gExpectedQueryV4);
-    equal(request.queryString.indexOf('+'), -1);
-    equal(request.queryString.indexOf('/'), -1);
 
     // Respond a V2 compatible content for now. In the future we can
     // send a meaningful response to test Bug 1284178 to see if the
     // update is successfully stored to database.
     response.setHeader("Content-Type",
                        "application/vnd.google.safebrowsing-update", false);
     response.setStatusLine(request.httpVersion, 200, "OK");