Backed out 3 changesets (bug 1575974) for xpcshell perma fails on test_nsIProcess.js. CLOSED TREE

Backed out changeset e32925d2b886 (bug 1575974)
Backed out changeset a9512bb19ea7 (bug 1575974)
Backed out changeset 521655c73cd1 (bug 1575974)

toolkit/components/crashes/CrashService.jsm

@@ -65,7 +65,7 @@ function runMinidumpAnalyzer(minidumpPath, allThreads) {
             break;
           case "process-failed":
             gRunningProcesses.delete(process);
-            resolve();
+            reject();
             break;
           default:
             reject(new Error("Unexpected topic received " + topic));

toolkit/components/telemetry/app/TelemetrySend.jsm

@@ -315,17 +315,14 @@ var TelemetrySend = {
    * @param {String} aUrl The telemetry server URL
    * @param {String} aPingFilePath The path to the file holding the ping
    *        contents, if if sent successfully the pingsender will delete it.
-   * @param {callback} observer A function called with parameters
-            (subject, topic, data) and a topic of "process-finished" or
-            "process-failed" after pingsender completion.
    *
    * @throws NS_ERROR_FAILURE if we couldn't find or run the pingsender
    *         executable.
    * @throws NS_ERROR_NOT_IMPLEMENTED on Android as the pingsender is not
    *         available.
    */
-  testRunPingSender(url, pingPath, observer) {
-    return TelemetrySendImpl.runPingSender(url, pingPath, observer);
+  testRunPingSender(url, pingPath) {
+    TelemetrySendImpl.runPingSender(url, pingPath);
   },
 };
 
@@ -1556,7 +1553,7 @@ var TelemetrySendImpl = {
     };
   },
 
-  runPingSender(url, pingPath, observer) {
+  runPingSender(url, pingPath) {
     if (AppConstants.platform === "android") {
       throw Cr.NS_ERROR_NOT_IMPLEMENTED;
     }
@@ -1573,6 +1570,6 @@ var TelemetrySendImpl = {
     process.init(exe);
     process.startHidden = true;
     process.noShell = true;
-    process.runAsync([url, pingPath], 2, observer);
+    process.run(/* blocking */ false, [url, pingPath], 2);
   },
 };

toolkit/components/telemetry/pingsender/pingsender.cpp

@@ -4,7 +4,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include <cstdlib>
-#include <cstring>
 #include <ctime>
 #include <fstream>
 #include <iomanip>
@@ -26,25 +25,6 @@ const char* kContentEncodingHeader = "Content-Encoding: gzip";
 // to the server.
 const uint32_t kConnectionTimeoutMs = 30 * 1000;
 
-// Operate in std::string because nul bytes will be preserved
-bool IsValidDestination(std::string aHost) {
-  static const std::string kValidDestinations[] = {
-    "localhost",
-    "incoming.telemetry.mozilla.org",
-  };
-  for (auto destination : kValidDestinations) {
-    if (aHost == destination) {
-      return true;
-    }
-  }
-  return false;
-}
-
-bool IsValidDestination(char* aHost) {
-  return IsValidDestination(std::string(aHost));
-}
-
-
 /**
  * This shared function returns a Date header string for use in HTTP requests.
  * See "RFC 7231, section 7.1.1.2: Date" for its specifications.

toolkit/components/telemetry/pingsender/pingsender.h

@@ -16,7 +16,4 @@ namespace PingSender {
 // System-specific function to make an HTTP POST operation
 bool Post(const std::string& url, const std::string& payload);
 
-bool IsValidDestination(char* aUriEndingInHost);
-bool IsValidDestination(std::string aUriEndingInHost);
-
 }  // namespace PingSender

toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp

@@ -3,7 +3,6 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#include <algorithm>
 #include <cerrno>
 #include <cstring>
 #include <dlfcn.h>
@@ -27,7 +26,6 @@ class CurlWrapper {
   CurlWrapper();
   ~CurlWrapper();
   bool Init();
-  bool IsValidDestination(const string& url);
   bool Post(const string& url, const string& payload);
 
   // libcurl functions
@@ -41,16 +39,9 @@ class CurlWrapper {
   void (*easy_cleanup)(CURL*);
   void (*global_cleanup)(void);
 
-  CURLU* (*curl_url)();
-  CURLUcode (*curl_url_get)(CURLU *, CURLUPart, char **, unsigned int);
-  CURLUcode (*curl_url_set)(CURLU *, CURLUPart, const char *, unsigned int);
-  void (*curl_free)( char *);
-  void (*curl_url_cleanup)(CURLU *);
-
  private:
   void* mLib;
   void* mCurl;
-  bool mCanParseUrl;
 };
 
 CurlWrapper::CurlWrapper()
@@ -63,11 +54,6 @@ CurlWrapper::CurlWrapper()
       easy_strerror(nullptr),
       easy_cleanup(nullptr),
       global_cleanup(nullptr),
-      curl_url(nullptr),
-      curl_url_get(nullptr),
-      curl_url_set(nullptr),
-      curl_free(nullptr),
-      curl_url_cleanup(nullptr),
       mLib(nullptr),
       mCurl(nullptr) {}
 
@@ -129,12 +115,6 @@ bool CurlWrapper::Init() {
   *(void**)(&easy_cleanup) = dlsym(mLib, "curl_easy_cleanup");
   *(void**)(&global_cleanup) = dlsym(mLib, "curl_global_cleanup");
 
-  *(void**)(&curl_url) = dlsym(mLib, "curl_url");
-  *(void**)(&curl_url_set) = dlsym(mLib, "curl_url_set");
-  *(void**)(&curl_url_get) = dlsym(mLib, "curl_url_get");
-  *(void**)(&curl_free) = dlsym(mLib, "curl_free");
-  *(void**)(&curl_url_cleanup) = dlsym(mLib, "curl_url_cleanup");
-
   if (!easy_init || !easy_setopt || !easy_perform || !easy_getinfo ||
       !slist_append || !slist_free_all || !easy_strerror || !easy_cleanup ||
       !global_cleanup) {
@@ -142,13 +122,6 @@ bool CurlWrapper::Init() {
     return false;
   }
 
-  mCanParseUrl = true;
-  if (!curl_url || !curl_url_get || !curl_url_set || !curl_free ||
-      !curl_url_cleanup) {
-    mCanParseUrl = false;
-    PINGSENDER_LOG("WARNING: Do not have url parsing functions in libcurl\n");
-  }
-
   mCurl = easy_init();
 
   if (!mCurl) {
@@ -169,67 +142,6 @@ static size_t DummyWriteCallback(char* ptr, size_t size, size_t nmemb,
   return size * nmemb;
 }
 
-// If we can't use curl's URL parsing (which is safer) we have to fallback
-// to this handwritten one (which is only as safe as we are clever.)
-bool FallbackIsValidDestination(const string& aUrl) {
-  // Lowercase the url
-  string url = aUrl;
-  std::transform(url.begin(), url.end(), url.begin(),
-    [](unsigned char c){ return std::tolower(c); });
-  // Strip off the scheme in the beginning
-  if (url.find_first_of("http://") != std::string::npos) {
-    url = url.substr(7);
-  } else if (url.find_first_of("https://") != std::string::npos) {
-    url = url.substr(8);
-  }
-
-  // Remove any user information. If a @ appeared in the userinformation,
-  // it would need to be encoded.
-  unsigned long atStart = url.find_first_of("@");
-  url = (atStart == std::string::npos) ? url : url.substr(atStart + 1);
-
-  // Remove any path or fragment information, leaving us with a url that may contain
-  // host, and port.
-  unsigned long fragStart = url.find_first_of("#");
-  url = (fragStart == std::string::npos) ? url : url.substr(0, fragStart);
-  unsigned long pathStart = url.find_first_of("/");
-  url = (pathStart == std::string::npos) ? url : url.substr(0, pathStart);
-
-  // Remove the port, because we run tests targeting localhost:port
-  unsigned long portStart = url.find_last_of(":");
-  url = (portStart == std::string::npos) ? url : url.substr(0, portStart);
-
-  return ::IsValidDestination(url);
-}
-
-bool CurlWrapper::IsValidDestination(const string& aUrl) {
-  if (!mCanParseUrl) {
-    return FallbackIsValidDestination(aUrl);
-  }
-
-  bool ret = false;
-  CURLU *h = curl_url();
-  if (!h) {
-    return FallbackIsValidDestination(aUrl);
-  }
-
-  if (CURLUE_OK != curl_url_set(h, CURLUPART_URL, aUrl.c_str(), 0)) {
-    goto cleanup;
-  }
-
-  char *host;
-  if (CURLUE_OK != curl_url_get(h, CURLUPART_HOST, &host, 0)) {
-    goto cleanup;
-  }
-
-  ret = ::IsValidDestination(host);
-  curl_free(host);
-
-cleanup:
-  curl_url_cleanup(h);
-  return ret;
-}
-
 bool CurlWrapper::Post(const string& url, const string& payload) {
   easy_setopt(mCurl, CURLOPT_URL, url.c_str());
   easy_setopt(mCurl, CURLOPT_USERAGENT, kUserAgent);
@@ -284,10 +196,6 @@ bool Post(const string& url, const string& payload) {
   if (!curl.Init()) {
     return false;
   }
-  if (!curl.IsValidDestination(url)) {
-    PINGSENDER_LOG("ERROR: Invalid destination host\n");
-    return false;
-  }
 
   return curl.Post(url, payload);
 }

toolkit/components/telemetry/pingsender/pingsender_win.cpp

@@ -61,11 +61,6 @@ bool Post(const string& url, const string& payload) {
     return false;
   }
 
-  if (!IsValidDestination(host)) {
-    PINGSENDER_LOG("ERROR: Invalid destination host '%s'\n", host);
-    return false;
-  }
-
   ScopedHInternet internet(InternetOpen(kUserAgent,
                                         INTERNET_OPEN_TYPE_PRECONFIG,
                                         /* lpszProxyName */ NULL,

toolkit/components/telemetry/tests/unit/test_PingSender.js

@@ -96,24 +96,7 @@ add_task(async function test_pingSender() {
 
   // Try to send it using the pingsender.
   const url = "http://localhost:" + PingServer.port + "/submit/telemetry/";
-  TelemetrySend.testRunPingSender(url, pingPath, (_, topic, __) => {
-    switch (topic) {
-      case "process-finished": // finished indicates an exit code of 0
-        Assert.equal(
-          true,
-          true,
-          "Pingsender should be able to post to localhost"
-        );
-        break;
-      case "process-failed": // failed indicates an exit code != 0
-        Assert.equal(
-          true,
-          false,
-          "Pingsender should be able to post to localhost"
-        );
-        break;
-    }
-  });
+  TelemetrySend.testRunPingSender(url, pingPath);
 
   let req = await PingServer.promiseNextRequest();
   let ping = decodeRequestPayload(req);
@@ -149,42 +132,6 @@ add_task(async function test_pingSender() {
   // Check that the PingSender removed the pending ping.
   await waitForPingDeletion(data.id);
 
-  // Confirm we can't send a ping to another destination url
-  let bannedUris = [
-    "https://example.com",
-    "http://localhost.com",
-    "http://localHOST.com",
-    "http://localhost@example.com",
-    "http://localhost:bob@example.com",
-    "http://localhost:localhost@localhost.example.com",
-  ];
-  for (let indx in bannedUris) {
-    TelemetrySend.testRunPingSender(
-      bannedUris[indx],
-      pingPath,
-      (_, topic, __) => {
-        switch (topic) {
-          case "process-finished": // finished indicates an exit code of 0
-            Assert.equal(
-              false,
-              true,
-              "Pingsender should not be able to post to any banned urls: " +
-                bannedUris[indx]
-            );
-            break;
-          case "process-failed": // failed indicates an exit code != 0
-            Assert.equal(
-              true,
-              true,
-              "Pingsender should not be able to post to any banned urls: " +
-                bannedUris[indx]
-            );
-            break;
-        }
-      }
-    );
-  }
-
   // Shut down the failing server. We do this now, and not right after using it,
   // to make sure we're not interfering with the test.
   await new Promise(r => failingServer.stop(r));

xpcom/threads/nsProcessCommon.cpp

@@ -312,7 +312,7 @@ void nsProcess::ProcessComplete() {
   }
 
   const char* topic;
-  if (mExitValue != 0) {
+  if (mExitValue < 0) {
     topic = "process-failed";
   } else {
     topic = "process-finished";