Bug 1054425 - cache2: leak in CacheFileMetadata::WriteMetadata. r=jduell, a=sledru
authorMichal Novotny <michal.novotny@gmail.com>
Mon, 18 Aug 2014 21:11:40 +0200
changeset 208349 342c0c26e18d
parent 208348 bff13e7445c5
child 208350 54949d681a14
push id3835
push userryanvm@gmail.com
push date2014-08-20 22:04 +0000
treeherdermozilla-beta@54949d681a14 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjduell, sledru
bugs1054425
milestone32.0
Bug 1054425 - cache2: leak in CacheFileMetadata::WriteMetadata. r=jduell, a=sledru
netwerk/cache2/CacheFileIOManager.cpp
netwerk/cache2/CacheFileMetadata.cpp
--- a/netwerk/cache2/CacheFileIOManager.cpp
+++ b/netwerk/cache2/CacheFileIOManager.cpp
@@ -1936,16 +1936,21 @@ CacheFileIOManager::Write(CacheFileHandl
   LOG(("CacheFileIOManager::Write() [handle=%p, offset=%lld, count=%d, "
        "validate=%d, listener=%p]", aHandle, aOffset, aCount, aValidate,
        aCallback));
 
   nsresult rv;
   nsRefPtr<CacheFileIOManager> ioMan = gInstance;
 
   if (aHandle->IsClosed() || !ioMan) {
+    if (!aCallback) {
+      // When no callback is provided, CacheFileIOManager is responsible for
+      // releasing the buffer. We must release it even in case of failure.
+      free(const_cast<char *>(aBuf));
+    }
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   nsRefPtr<WriteEvent> ev = new WriteEvent(aHandle, aOffset, aBuf, aCount,
                                            aValidate, aCallback);
   rv = ioMan->mIOThread->Dispatch(ev, CacheIOThread::WRITE);
   NS_ENSURE_SUCCESS(rv, rv);
 
--- a/netwerk/cache2/CacheFileMetadata.cpp
+++ b/netwerk/cache2/CacheFileMetadata.cpp
@@ -255,43 +255,36 @@ CacheFileMetadata::WriteMetadata(uint32_
   CacheHash::Hash32_t hash;
   hash = CacheHash::Hash(mWriteBuf + sizeof(uint32_t),
                          p - mWriteBuf - sizeof(uint32_t));
   NetworkEndian::writeUint32(mWriteBuf, hash);
 
   NetworkEndian::writeUint32(p, aOffset);
   p += sizeof(uint32_t);
 
-  char * writeBuffer;
+  char * writeBuffer = mWriteBuf;
   if (aListener) {
     mListener = aListener;
-    writeBuffer = mWriteBuf;
   } else {
-    // We are not going to pass |this| as callback to CacheFileIOManager::Write
-    // so we must allocate a new buffer that will be released automatically when
-    // write is finished.  This is actually better than to let
-    // CacheFileMetadata::OnDataWritten do the job, since when dispatching the
-    // result from some reason fails during shutdown, we would unnecessarily leak
-    // both this object and the buffer.
-    writeBuffer = static_cast<char *>(moz_xmalloc(p - mWriteBuf));
-    memcpy(writeBuffer, mWriteBuf, p - mWriteBuf);
+    // We are not going to pass |this| as a callback so the buffer will be
+    // released by CacheFileIOManager. Just null out mWriteBuf here.
+    mWriteBuf = nullptr;
   }
 
-  rv = CacheFileIOManager::Write(mHandle, aOffset, writeBuffer, p - mWriteBuf,
+  rv = CacheFileIOManager::Write(mHandle, aOffset, writeBuffer, p - writeBuffer,
                                  true, aListener ? this : nullptr);
   if (NS_FAILED(rv)) {
     LOG(("CacheFileMetadata::WriteMetadata() - CacheFileIOManager::Write() "
          "failed synchronously. [this=%p, rv=0x%08x]", this, rv));
 
     mListener = nullptr;
-    if (writeBuffer != mWriteBuf) {
-      free(writeBuffer);
+    if (mWriteBuf) {
+      free(mWriteBuf);
+      mWriteBuf = nullptr;
     }
-    free(mWriteBuf);
-    mWriteBuf = nullptr;
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   DoMemoryReport(MemoryUsage());
 
   return NS_OK;
 }