Bug 637461 - data race in StartupCache.cpp; r=mwu
authorTaras Glek <tglek@mozilla.com>
Fri, 01 Apr 2011 01:24:16 -0400
changeset 64627 2bee01c51ed63f94ba8414d3f032b4e8b9816495
parent 64626 3ac1470a20bdac5fb03b117eafc82d93df76a1d8
child 64628 243f2713f7f9bbb8c79508e01ceffc91306221e0
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmwu
bugs637461
milestone2.2a1pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 637461 - data race in StartupCache.cpp; r=mwu
startupcache/StartupCache.cpp
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -60,17 +60,19 @@
 #include "nsITimer.h"
 #include "nsIZipWriter.h"
 #include "nsIZipReader.h"
 #include "nsWeakReference.h"
 #include "nsZipArchive.h"
 #include "mozilla/Omnijar.h"
 #include "prenv.h"
 #include "mozilla/FunctionTimer.h"
- 
+#include "nsThreadUtils.h"
+#include "nsXULAppAPI.h"
+
 #ifdef IS_BIG_ENDIAN
 #define SC_ENDIAN "big"
 #else
 #define SC_ENDIAN "little"
 #endif
 
 #if PR_BYTES_PER_WORD == 4
 #define SC_WORDSIZE "4"
@@ -121,28 +123,31 @@ StartupCache::StartupCache()
 
 StartupCache::~StartupCache() 
 {
   if (mTimer) {
     mTimer->Cancel();
   }
 
   // Generally, the in-memory table should be empty here,
-  // but in special cases (like Talos Ts tests) we
-  // could shut down before we write.
-  // This mechanism will change when IO is moved off-thread
-  // (bug 586859) or when Talos first-run is changed to allow
-  // our timer to work (bug 591471).
+  // but an early shutdown means either mTimer didn't run 
+  // or the write thread is still running.
+  WaitOnWriteThread();
   WriteToDisk();
   gStartupCache = nsnull;
 }
 
 nsresult
 StartupCache::Init() 
 {
+  if (XRE_GetProcessType() != GeckoProcessType_Default) {
+    NS_WARNING("Startup cache is only available in the chrome process");
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   nsresult rv;
   mTable.Init();
 #ifdef DEBUG
   mWriteObjectMap.Init();
 #endif
 
   // This allows to override the startup cache filename
   // which is useful from xpcshell, when there is no ProfLDS directory to keep cache in.
@@ -196,35 +201,38 @@ StartupCache::Init()
   // If it's corrupted, just remove it and start over.
   if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) {
     NS_WARNING("Failed to load startupcache file correctly, removing!");
     InvalidateCache();
   }
   return NS_OK;
 }
 
+/** 
+ * LoadArchive can be called from the main thread or while reloading cache on write thread.
+ */
 nsresult
 StartupCache::LoadArchive() 
 {
-  WaitOnWriteThread();
   PRBool exists;
   mArchive = NULL;
   nsresult rv = mFile->Exists(&exists);
   if (NS_FAILED(rv) || !exists)
     return NS_ERROR_FILE_NOT_FOUND;
   
   mArchive = new nsZipArchive();
   return mArchive->OpenArchive(mFile);
 }
 
 // NOTE: this will not find a new entry until it has been written to disk!
 // Consumer should take ownership of the resulting buffer.
 nsresult
 StartupCache::GetBuffer(const char* id, char** outbuf, PRUint32* length) 
 {
+  NS_ASSERTION(NS_IsMainThread(), "Startup cache only available on main thread");
   WaitOnWriteThread();
   if (!mStartupWriteInitiated) {
     CacheEntry* entry; 
     nsDependentCString idStr(id);
     mTable.Get(idStr, &entry);
     if (entry) {
       *outbuf = new char[entry->size];
       memcpy(*outbuf, entry->data, entry->size);
@@ -255,16 +263,17 @@ StartupCache::GetBuffer(const char* id, 
 #endif
   return NS_ERROR_NOT_AVAILABLE;
 }
 
 // Makes a copy of the buffer, client retains ownership of inbuf.
 nsresult
 StartupCache::PutBuffer(const char* id, const char* inbuf, PRUint32 len) 
 {
+  NS_ASSERTION(NS_IsMainThread(), "Startup cache only available on main thread");
   WaitOnWriteThread();
   if (StartupCache::gShutdownInitiated) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   nsAutoArrayPtr<char> data(new char[len]);
   memcpy(data, inbuf, len);
 
@@ -315,20 +324,24 @@ CacheCloseHelper(const nsACString& key, 
   rv = writer->AddEntryStream(key, holder->time, PR_TRUE, stream, PR_FALSE);
   
   if (NS_FAILED(rv)) {
     NS_WARNING("cache entry deleted but not written to disk.");
   }
   return PL_DHASH_REMOVE;
 }
 
+
+/** 
+ * WriteToDisk writes the cache out to disk. Callers of WriteToDisk need to call WaitOnWriteThread
+ * to make sure there isn't a write happening on another thread
+ */
 void
 StartupCache::WriteToDisk() 
 {
-  WaitOnWriteThread();
   nsresult rv;
   mStartupWriteInitiated = PR_TRUE;
 
   if (mTable.Count() == 0)
     return;
 
   nsCOMPtr<nsIZipWriter> zipW = do_CreateInstance("@mozilla.org/zipwriter;1");
   if (!zipW)
@@ -377,23 +390,22 @@ StartupCache::InvalidateCache()
 /*
  * WaitOnWriteThread() is called from a main thread to wait for the worker
  * thread to finish. However since the same code is used in the worker thread and
  * main thread, the worker thread can also call WaitOnWriteThread() which is a no-op.
  */
 void
 StartupCache::WaitOnWriteThread()
 {
-  PRThread* writeThread = mWriteThread;
-  if (!writeThread || writeThread == PR_GetCurrentThread())
+  NS_ASSERTION(NS_IsMainThread(), "Startup cache should only wait for io thread on main thread");
+  if (!mWriteThread || mWriteThread == PR_GetCurrentThread())
     return;
 
   NS_TIME_FUNCTION_MIN(30);
-  //NS_WARNING("Waiting on startupcache write");
-  PR_JoinThread(writeThread);
+  PR_JoinThread(mWriteThread);
   mWriteThread = NULL;
 }
 
 void 
 StartupCache::ThreadedWrite(void *aClosure)
 {
   gStartupCache->WriteToDisk();
 }