Bug 1334278 - change mozilla::Smprintf to return a UniquePtr; r=froydnj

Change mozilla::Smprintf and friends to return a UniquePtr, rather than
relying on manual memory management.  (Though after this patch there are
still a handful of spots needing SmprintfFree.)

MozReview-Commit-ID: COa4nzIX5qa

chrome/nsChromeRegistry.cpp

@@ -55,13 +55,12 @@ nsChromeRegistry::LogMessage(const char* aMsg, ...)
 
   va_list args;
   va_start(args, aMsg);
-  char* formatted = mozilla::Vsmprintf(aMsg, args);
+  mozilla::SmprintfPointer formatted = mozilla::Vsmprintf(aMsg, args);
   va_end(args);
   if (!formatted)
     return;
 
-  console->LogStringMessage(NS_ConvertUTF8toUTF16(formatted).get());
-  mozilla::SmprintfFree(formatted);
+  console->LogStringMessage(NS_ConvertUTF8toUTF16(formatted.get()).get());
 }
 
 void
@@ -80,7 +79,7 @@ nsChromeRegistry::LogMessageWithContext(nsIURI* aURL, uint32_t aLineNumber, uint
 
   va_list args;
   va_start(args, aMsg);
-  char* formatted = mozilla::Vsmprintf(aMsg, args);
+  mozilla::SmprintfPointer formatted = mozilla::Vsmprintf(aMsg, args);
   va_end(args);
   if (!formatted)
     return;
@@ -89,11 +88,10 @@ nsChromeRegistry::LogMessageWithContext(nsIURI* aURL, uint32_t aLineNumber, uint
   if (aURL)
     aURL->GetSpec(spec);
 
-  rv = error->Init(NS_ConvertUTF8toUTF16(formatted),
+  rv = error->Init(NS_ConvertUTF8toUTF16(formatted.get()),
                    NS_ConvertUTF8toUTF16(spec),
                    EmptyString(),
                    aLineNumber, 0, flags, "chrome registration");
-  mozilla::SmprintfFree(formatted);
 
   if (NS_FAILED(rv))
     return;

dom/plugins/base/nsPluginsDirWin.cpp

@@ -103,7 +103,7 @@ static char* GetVersion(void* verbuf)
                       HIWORD(fileInfo->dwFileVersionMS),
                       LOWORD(fileInfo->dwFileVersionMS),
                       HIWORD(fileInfo->dwFileVersionLS),
-                      LOWORD(fileInfo->dwFileVersionLS));
+                      LOWORD(fileInfo->dwFileVersionLS)).release();
   }
 
   return nullptr;

dom/webbrowserpersist/nsWebBrowserPersist.cpp

@@ -2028,7 +2028,7 @@ nsWebBrowserPersist::CalculateUniqueFilename(nsIURI *aURI)
 
             if (base.IsEmpty() || duplicateCounter > 1)
             {
-                char * tmp = mozilla::Smprintf("_%03d", duplicateCounter);
+                SmprintfPointer tmp = mozilla::Smprintf("_%03d", duplicateCounter);
                 NS_ENSURE_TRUE(tmp, NS_ERROR_OUT_OF_MEMORY);
                 if (filename.Length() < kDefaultMaxFilenameLength - 4)
                 {
@@ -2038,8 +2038,7 @@ nsWebBrowserPersist::CalculateUniqueFilename(nsIURI *aURI)
                 {
                     base.Mid(tmpBase, 0, base.Length() - 4);
                 }
-                tmpBase.Append(tmp);
-                mozilla::SmprintfFree(tmp);
+                tmpBase.Append(tmp.get());
             }
             else
             {

ipc/chromium/src/base/logging.cc

@@ -8,7 +8,7 @@
 #include "prmem.h"
 #include "base/string_util.h"
 #include "nsXPCOM.h"
-#include "mozilla/Printf.h"
+#include "mozilla/Move.h"
 
 namespace mozilla {
 
@@ -44,19 +44,17 @@ Logger::~Logger()
     break;
   }
 
-  MOZ_LOG(gChromiumPRLog, prlevel, ("%s:%i: %s", mFile, mLine, mMsg ? mMsg : "<no message>"));
+  MOZ_LOG(gChromiumPRLog, prlevel, ("%s:%i: %s", mFile, mLine, mMsg ? mMsg.get() : "<no message>"));
   if (xpcomlevel != -1)
-    NS_DebugBreak(xpcomlevel, mMsg, NULL, mFile, mLine);
-
-  mozilla::SmprintfFree(mMsg);
+    NS_DebugBreak(xpcomlevel, mMsg.get(), NULL, mFile, mLine);
 }
 
 void
 Logger::printf(const char* fmt, ...)
 {
   va_list args;
   va_start(args, fmt);
-  mMsg = mozilla::VsmprintfAppend(mMsg, fmt, args);
+  mMsg = mozilla::VsmprintfAppend(mozilla::Move(mMsg), fmt, args);
   va_end(args);
 }
 

ipc/chromium/src/base/logging.h

@@ -13,6 +13,7 @@
 #include "base/basictypes.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Logging.h"
+#include "mozilla/Printf.h"
 
 #ifdef NO_CHROMIUM_LOGGING
 #include <sstream>
@@ -39,7 +40,6 @@ class Logger
     : mSeverity(severity)
     , mFile(file)
     , mLine(line)
-    , mMsg(NULL)
   { }
 
   ~Logger();
@@ -54,7 +54,7 @@ class Logger
   LogSeverity mSeverity;
   const char* mFile;
   int mLine;
-  char* mMsg;
+  SmprintfPointer mMsg;
 
   DISALLOW_EVIL_CONSTRUCTORS(Logger);
 };

ipc/glue/SharedMemoryBasic_mach.mm

@@ -31,11 +31,10 @@
 #include "mozilla/StaticMutex.h"
 
 #ifdef DEBUG
-#define LOG_ERROR(str, args...)                 \
-  PR_BEGIN_MACRO                                \
-  char *msg = mozilla::Smprintf(str, ## args);  \
-  NS_WARNING(msg);                              \
-  mozilla::SmprintfFree(msg);                   \
+#define LOG_ERROR(str, args...)                                   \
+  PR_BEGIN_MACRO                                                  \
+  mozilla::SmprintfPointer msg = mozilla::Smprintf(str, ## args); \
+  NS_WARNING(msg.get());                                          \
   PR_END_MACRO
 #else
 #define LOG_ERROR(str, args...) do { /* nothing */ } while(0)

js/src/jsprf.cpp

@@ -18,13 +18,15 @@
 
 using namespace js;
 
+typedef mozilla::SmprintfPolicyPointer<js::SystemAllocPolicy> JSSmprintfPointer;
+
 JS_PUBLIC_API(char*) JS_smprintf(const char* fmt, ...)
 {
     va_list ap;
     va_start(ap, fmt);
-    char* result = mozilla::Vsmprintf<js::SystemAllocPolicy>(fmt, ap);
+    JSSmprintfPointer result = mozilla::Vsmprintf<js::SystemAllocPolicy>(fmt, ap);
     va_end(ap);
-    return result;
+    return result.release();
 }
 
 JS_PUBLIC_API(void) JS_smprintf_free(char* mem)
@@ -36,17 +38,19 @@ JS_PUBLIC_API(char*) JS_sprintf_append(char* last, const char* fmt, ...)
 {
     va_list ap;
     va_start(ap, fmt);
-    char* result = mozilla::VsmprintfAppend<js::SystemAllocPolicy>(last, fmt, ap);
+    JSSmprintfPointer result =
+        mozilla::VsmprintfAppend<js::SystemAllocPolicy>(JSSmprintfPointer(last), fmt, ap);
     va_end(ap);
-    return result;
+    return result.release();
 }
 
 JS_PUBLIC_API(char*) JS_vsmprintf(const char* fmt, va_list ap)
 {
-    return mozilla::Vsmprintf<js::SystemAllocPolicy>(fmt, ap);
+    return mozilla::Vsmprintf<js::SystemAllocPolicy>(fmt, ap).release();
 }
 
 JS_PUBLIC_API(char*) JS_vsprintf_append(char* last, const char* fmt, va_list ap)
 {
-    return mozilla::VsmprintfAppend<js::SystemAllocPolicy>(last, fmt, ap);
+    return mozilla::VsmprintfAppend<js::SystemAllocPolicy>(JSSmprintfPointer(last),
+                                                           fmt, ap).release();
 }

mozglue/misc/Printf.h

@@ -55,6 +55,7 @@
 #include "mozilla/IntegerPrintfMacros.h"
 #include "mozilla/SizePrintfMacros.h"
 #include "mozilla/Types.h"
+#include "mozilla/UniquePtr.h"
 
 #include <stdarg.h>
 #include <string.h>
@@ -103,6 +104,26 @@ class PrintfTarget
     bool cvt_s(const char* s, int width, int prec, int flags);
 };
 
+namespace detail {
+
+template<typename AllocPolicy = mozilla::MallocAllocPolicy>
+struct AllocPolicyBasedFreePolicy
+{
+  void operator()(const void* ptr) {
+    AllocPolicy policy;
+    policy.free_(const_cast<void*>(ptr));
+  }
+};
+
+}
+
+// The type returned by Smprintf and friends.
+template<typename AllocPolicy>
+using SmprintfPolicyPointer = mozilla::UniquePtr<char, detail::AllocPolicyBasedFreePolicy<AllocPolicy>>;
+
+// The default type if no alloc policy is specified.
+typedef SmprintfPolicyPointer<mozilla::MallocAllocPolicy> SmprintfPointer;
+
 // Used in the implementation of Smprintf et al.
 template<typename AllocPolicy>
 class MOZ_STACK_CLASS SprintfState final : private mozilla::PrintfTarget, private AllocPolicy
@@ -125,8 +146,8 @@ class MOZ_STACK_CLASS SprintfState final : private mozilla::PrintfTarget, privat
         return mozilla::PrintfTarget::vprint(format, ap_list) && append("", 1);
     }
 
-    char* release() {
-        char* result = mBase;
+    SmprintfPolicyPointer<AllocPolicy> release() {
+        SmprintfPolicyPointer<AllocPolicy> result(mBase);
         mBase = nullptr;
         return result;
     }
@@ -173,7 +194,7 @@ class MOZ_STACK_CLASS SprintfState final : private mozilla::PrintfTarget, privat
 */
 template<typename AllocPolicy = mozilla::MallocAllocPolicy>
 MOZ_FORMAT_PRINTF(1, 2)
-char* Smprintf(const char* fmt, ...)
+SmprintfPolicyPointer<AllocPolicy> Smprintf(const char* fmt, ...)
 {
     SprintfState<AllocPolicy> ss(nullptr);
     va_list ap;
@@ -195,9 +216,10 @@ char* Smprintf(const char* fmt, ...)
 */
 template<typename AllocPolicy = mozilla::MallocAllocPolicy>
 MOZ_FORMAT_PRINTF(2, 3)
-char* SmprintfAppend(char* last, const char* fmt, ...)
+SmprintfPolicyPointer<AllocPolicy> SmprintfAppend(SmprintfPolicyPointer<AllocPolicy>&& last,
+                                                  const char* fmt, ...)
 {
-    SprintfState<AllocPolicy> ss(last);
+    SprintfState<AllocPolicy> ss(last.release());
     va_list ap;
     va_start(ap, fmt);
     bool r = ss.vprint(fmt, ap);
@@ -212,7 +234,7 @@ char* SmprintfAppend(char* last, const char* fmt, ...)
 ** va_list forms of the above.
 */
 template<typename AllocPolicy = mozilla::MallocAllocPolicy>
-char* Vsmprintf(const char* fmt, va_list ap)
+SmprintfPolicyPointer<AllocPolicy> Vsmprintf(const char* fmt, va_list ap)
 {
     SprintfState<AllocPolicy> ss(nullptr);
     if (!ss.vprint(fmt, ap))
@@ -221,9 +243,10 @@ char* Vsmprintf(const char* fmt, va_list ap)
 }
 
 template<typename AllocPolicy = mozilla::MallocAllocPolicy>
-char* VsmprintfAppend(char* last, const char* fmt, va_list ap)
+SmprintfPolicyPointer<AllocPolicy> VsmprintfAppend(SmprintfPolicyPointer<AllocPolicy>&& last,
+                                                   const char* fmt, va_list ap)
 {
-    SprintfState<AllocPolicy> ss(last);
+    SprintfState<AllocPolicy> ss(last.release());
     if (!ss.vprint(fmt, ap))
         return nullptr;
     return ss.release();

netwerk/cookie/nsCookieService.cpp

@@ -334,10 +334,9 @@ LogSuccess(bool aSetCookie, nsIURI *aHostURI, const nsAFlatCString &aCookieStrin
   PR_BEGIN_MACRO                                                             \
   nsresult __rv = res; /* Do not evaluate |res| more than once! */           \
   if (NS_FAILED(__rv)) {                                                     \
-    char *msg = mozilla::Smprintf("NS_ASSERT_SUCCESS(%s) failed with result 0x%" PRIX32, \
+    SmprintfPointer msg = mozilla::Smprintf("NS_ASSERT_SUCCESS(%s) failed with result 0x%" PRIX32, \
                            #res, static_cast<uint32_t>(__rv));               \
-    NS_ASSERTION(NS_SUCCEEDED(__rv), msg);                                   \
-    mozilla::SmprintfFree(msg);                                                    \
+    NS_ASSERTION(NS_SUCCEEDED(__rv), msg.get());                             \
   }                                                                          \
   PR_END_MACRO
 #else

netwerk/protocol/http/nsHttpHandler.cpp

@@ -890,12 +890,11 @@ nsHttpHandler::InitUserAgentComponents()
           ? WNT_BASE "; WOW64"
           : WNT_BASE;
 #endif
-        char *buf = mozilla::Smprintf(format,
-                               info.dwMajorVersion,
-                               info.dwMinorVersion);
+        SmprintfPointer buf = mozilla::Smprintf(format,
+                                                info.dwMajorVersion,
+                                                info.dwMinorVersion);
         if (buf) {
-            mOscpu = buf;
-            mozilla::SmprintfFree(buf);
+            mOscpu = buf.get();
         }
     }
 #elif defined (XP_MACOSX)

storage/mozStorageConnection.cpp

@@ -1006,24 +1006,22 @@ Connection::internalClose(sqlite3 *aNativeConnection)
               stmt));
 
 #ifdef DEBUG
-      char *msg = ::mozilla::Smprintf("SQL statement '%s' (%p) should have been finalized before closing the connection",
-                               ::sqlite3_sql(stmt),
-                               stmt);
-      NS_WARNING(msg);
-      ::mozilla::SmprintfFree(msg);
-      msg = nullptr;
+      {
+        SmprintfPointer msg = ::mozilla::Smprintf("SQL statement '%s' (%p) should have been finalized before closing the connection",
+                                           ::sqlite3_sql(stmt),
+                                           stmt);
+        NS_WARNING(msg.get());
+      }
 #endif // DEBUG
 
       srv = ::sqlite3_finalize(stmt);
 
 #ifdef DEBUG
       if (srv != SQLITE_OK) {
-        msg = ::mozilla::Smprintf("Could not finalize SQL statement '%s' (%p)",
-                           ::sqlite3_sql(stmt),
-                           stmt);
-        NS_WARNING(msg);
-        ::mozilla::SmprintfFree(msg);
-        msg = nullptr;
+        SmprintfPointer msg = ::mozilla::Smprintf("Could not finalize SQL statement '%s' (%p)",
+                                           ::sqlite3_sql(stmt),
+                                           stmt);
+        NS_WARNING(msg.get());
       }
 #endif // DEBUG
 
@@ -1816,12 +1814,11 @@ Connection::CreateTable(const char *aTableName,
 {
   if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
 
-  char *buf = ::mozilla::Smprintf("CREATE TABLE %s (%s)", aTableName, aTableSchema);
+  SmprintfPointer buf = ::mozilla::Smprintf("CREATE TABLE %s (%s)", aTableName, aTableSchema);
   if (!buf)
     return NS_ERROR_OUT_OF_MEMORY;
 
-  int srv = executeSql(mDBConn, buf);
-  ::mozilla::SmprintfFree(buf);
+  int srv = executeSql(mDBConn, buf.get());
 
   return convertResultCode(srv);
 }

storage/mozStorageStatement.cpp

@@ -382,7 +382,7 @@ Statement::internalFinalize(bool aDestructing)
     // Finalizing it here would be useless and segfaultish.
     //
 
-    char *msg = ::mozilla::Smprintf("SQL statement (%p) should have been finalized"
+    SmprintfPointer msg = ::mozilla::Smprintf("SQL statement (%p) should have been finalized"
       " before garbage-collection. For more details on this statement, set"
       " NSPR_LOG_MESSAGES=mozStorage:5 .",
       mDBStatement);
@@ -397,13 +397,11 @@ Statement::internalFinalize(bool aDestructing)
 #if 0
     // Deactivate the warning until we have fixed the exising culprit
     // (see bug 914070).
-    NS_WARNING(msg);
+    NS_WARNING(msg.get());
 #endif // 0
 
     // Use %s so we aren't exposing random strings to printf interpolation.
-    MOZ_LOG(gStorageLog, LogLevel::Warning, ("%s", msg));
-
-    ::mozilla::SmprintfFree(msg);
+    MOZ_LOG(gStorageLog, LogLevel::Warning, ("%s", msg.get()));
   }
 
 #endif

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -3542,7 +3542,7 @@ OOPInit()
 #if defined(XP_WIN)
   childCrashNotifyPipe =
     mozilla::Smprintf("\\\\.\\pipe\\gecko-crash-server-pipe.%i",
-               static_cast<int>(::GetCurrentProcessId()));
+               static_cast<int>(::GetCurrentProcessId())).release();
 
   const std::wstring dumpPath = gExceptionHandler->dump_path();
   crashServer = new CrashGenerationServer(
@@ -3572,7 +3572,7 @@ OOPInit()
 #elif defined(XP_MACOSX)
   childCrashNotifyPipe =
     mozilla::Smprintf("gecko-crash-server-pipe.%i",
-               static_cast<int>(getpid()));
+               static_cast<int>(getpid())).release();
   const std::string dumpPath = gExceptionHandler->dump_path();
 
   crashServer = new CrashGenerationServer(