Bug 1364068 - Properly cleanup libcurl in the pingsender. r?gsvelto draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Wed, 24 May 2017 19:27:33 +0200
changeset 585937 c7f38bca9ee85aa2e2ce0ba780f62043b438c567
parent 585936 7584114529b4205259c8fd286d3cf98b23ab69ab
child 630836 ebaac54e67f5aa1162b762d426fa0fd70cb728d9
push id61235
push useralessio.placitelli@gmail.com
push dateMon, 29 May 2017 10:02:53 +0000
reviewersgsvelto
bugs1364068
milestone55.0a1
Bug 1364068 - Properly cleanup libcurl in the pingsender. r?gsvelto This additionally changes exit() calls with |return VALUE| so that we are sure to call the destructors and valgrind doesn't complain. Moreover, this disables the 'new-profile' ping when Firefox is ran on test profiles. MozReview-Commit-ID: BlGE9w6yGCL
testing/profiles/prefs_general.js
toolkit/components/telemetry/pingsender/pingsender.cpp
toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
--- a/testing/profiles/prefs_general.js
+++ b/testing/profiles/prefs_general.js
@@ -241,16 +241,19 @@ user_pref('browser.contentHandlers.types
 user_pref('browser.contentHandlers.types.1.uri', 'http://test1.example.org/rss?url=%%s')
 user_pref('browser.contentHandlers.types.2.uri', 'http://test1.example.org/rss?url=%%s')
 user_pref('browser.contentHandlers.types.3.uri', 'http://test1.example.org/rss?url=%%s')
 user_pref('browser.contentHandlers.types.4.uri', 'http://test1.example.org/rss?url=%%s')
 user_pref('browser.contentHandlers.types.5.uri', 'http://test1.example.org/rss?url=%%s')
 
 // We want to collect telemetry, but we don't want to send in the results.
 user_pref('toolkit.telemetry.server', 'https://%(server)s/telemetry-dummy/');
+// Don't new-profile' ping on new profiles during tests, otherwise the testing framework
+// might wait on the pingsender to finish and slow down tests.
+user_pref("toolkit.telemetry.newProfilePing.enabled", false);
 
 // A couple of preferences with default values to test that telemetry preference
 // watching is working.
 user_pref('toolkit.telemetry.test.pref1', true);
 user_pref('toolkit.telemetry.test.pref2', false);
 
 // We don't want to hit the real Firefox Accounts server for tests.  We don't
 // actually need a functioning FxA server, so just set it to something that
--- a/toolkit/components/telemetry/pingsender/pingsender.cpp
+++ b/toolkit/components/telemetry/pingsender/pingsender.cpp
@@ -151,41 +151,41 @@ int main(int argc, char* argv[])
   if (argc == 3) {
     url = argv[1];
     pingPath = argv[2];
   } else {
     PINGSENDER_LOG("Usage: pingsender URL PATH\n"
                    "Send the payload stored in PATH to the specified URL using "
                    "an HTTP POST message\n"
                    "then delete the file after a successful send.\n");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
   }
 
   string ping(ReadPing(pingPath));
 
   if (ping.empty()) {
     PINGSENDER_LOG("ERROR: Ping payload is empty\n");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
   }
 
   // Compress the ping using gzip.
   string gzipPing(GzipCompress(ping));
 
   // In the unlikely event of failure to gzip-compress the ping, don't
   // attempt to send it uncompressed: Telemetry will pick it up and send
   // it compressed.
   if (gzipPing.empty()) {
     PINGSENDER_LOG("ERROR: Ping compression failed\n");
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
   }
 
   if (!Post(url, gzipPing)) {
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
   }
 
   // If the ping was successfully sent, delete the file.
   if (!pingPath.empty() && std::remove(pingPath.c_str())) {
     // We failed to remove the pending ping file.
-    exit(EXIT_FAILURE);
+    return EXIT_FAILURE;
   }
 
-  exit(EXIT_SUCCESS);
+  return EXIT_SUCCESS;
 }
--- a/toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
+++ b/toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp
@@ -36,42 +36,48 @@ public:
   CURL* (*easy_init)(void);
   CURLcode (*easy_setopt)(CURL*, CURLoption, ...);
   CURLcode (*easy_perform)(CURL*);
   CURLcode (*easy_getinfo)(CURL*, CURLINFO, ...);
   curl_slist* (*slist_append)(curl_slist*, const char*);
   void (*slist_free_all)(curl_slist*);
   const char* (*easy_strerror)(CURLcode);
   void (*easy_cleanup)(CURL*);
+  void (*global_cleanup)(void);
 
 private:
   void* mLib;
   void* mCurl;
 };
 
 CurlWrapper::CurlWrapper()
   : easy_init(nullptr)
   , easy_setopt(nullptr)
   , easy_perform(nullptr)
   , easy_getinfo(nullptr)
   , slist_append(nullptr)
   , slist_free_all(nullptr)
   , easy_strerror(nullptr)
   , easy_cleanup(nullptr)
+  , global_cleanup(nullptr)
   , mLib(nullptr)
   , mCurl(nullptr)
 {}
 
 CurlWrapper::~CurlWrapper()
 {
   if(mLib) {
     if(mCurl && easy_cleanup) {
       easy_cleanup(mCurl);
     }
 
+    if (global_cleanup) {
+      global_cleanup();
+    }
+
     dlclose(mLib);
   }
 }
 
 bool
 CurlWrapper::Init()
 {
   // libcurl might show up under different names, try them all until we find it
@@ -111,25 +117,27 @@ CurlWrapper::Init()
   *(void**) (&easy_init) = dlsym(mLib, "curl_easy_init");
   *(void**) (&easy_setopt) = dlsym(mLib, "curl_easy_setopt");
   *(void**) (&easy_perform) = dlsym(mLib, "curl_easy_perform");
   *(void**) (&easy_getinfo) = dlsym(mLib, "curl_easy_getinfo");
   *(void**) (&slist_append) = dlsym(mLib, "curl_slist_append");
   *(void**) (&slist_free_all) = dlsym(mLib, "curl_slist_free_all");
   *(void**) (&easy_strerror) = dlsym(mLib, "curl_easy_strerror");
   *(void**) (&easy_cleanup) = dlsym(mLib, "curl_easy_cleanup");
+  *(void**) (&global_cleanup) = dlsym(mLib, "curl_global_cleanup");
 
   if (!easy_init ||
       !easy_setopt ||
       !easy_perform ||
       !easy_getinfo ||
       !slist_append ||
       !slist_free_all ||
       !easy_strerror ||
-      !easy_cleanup) {
+      !easy_cleanup ||
+      !global_cleanup) {
     PINGSENDER_LOG("ERROR: libcurl is missing one of the required symbols\n");
     return false;
   }
 
   mCurl = easy_init();
 
   if (!mCurl) {
     PINGSENDER_LOG("ERROR: Could not initialize libcurl\n");