JavaScriptCore:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2007 15:09:37 +0000 (15:09 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2007 15:09:37 +0000 (15:09 +0000)
        Reviewed by Oliver Hunt.

        Fixed <rdar://problem/4681051> Installer crashes in KJS::Collector::
        markOtherThreadConservatively(KJS::Collector::Thread*) trying to install
        iLife 06 using Rosetta on an Intel Machine

        The problem was that our thread-specific data destructor would modify the
        list of active JavaScript threads without holding the JSLock, corrupting
        the list. Corruption was especially likely if one JavaScript thread exited
        while another was starting up.

        * JavaScriptCore.exp:
        * kjs/JSLock.cpp: Don't conflate locking the JSLock with registering a
        thread, since the thread-specific data destructor needs to lock
        without registering a thread. Instead, treat thread registration as a
        part of the convenience of the JSLock object, and whittle down JSLock::lock()
        to just the bits that actually do the locking.
        (KJS::JSLock::lock):
        (KJS::JSLock::registerThread):
        * kjs/JSLock.h: Updated comments to mention the new behavior above, and
        other recent changes.
        (KJS::JSLock::JSLock):
        * kjs/collector.cpp:
        (KJS::destroyRegisteredThread): Lock here.
        (KJS::Collector::registerThread): To match, assert that we're locked here.

JavaScriptGlue:

        Reviewed by Oliver Hunt.

        Updated in light of fix for <rdar://problem/4681051> Installer crashes
        in KJS::Collector::markOtherThreadConservatively(KJS::Collector::Thread*)
        trying to install iLife 06 using Rosetta on an Intel Machine

        * JavaScriptGlue.cpp:
        (JSLockInterpreter): Ensure backwards compatibility by calling
        registerThread() when explicitly taking the JSLock. (This doesn't happen
        automatically anymore.) I doubt this actally matters, but in JavaScriptGlue
        territory, that kind of thinking will get you killed.

WebKitTools:

        Reviewed by Oliver Hunt.

        Beefed up --threaded mode in light of <rdar://problem/4681051> Installer
        crashes in KJS::Collector::markOtherThreadConservatively(KJS::Collector::Thread*)
        trying to install iLife 06 using Rosetta on an Intel Machine

        --threaded mode now runs a bunch of different JavaScript threads, randomly
        killing and respawning them. This was sufficient for reproducing the
        bug on my MacBook Pro.

        * DumpRenderTree/DumpRenderTree.m:
        (javaScriptThreads):
        (runJavaScriptThread):
        (startJavaScriptThreads):
        (stopJavaScriptThreads):
        (dumpRenderTree):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@20115 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/ChangeLog
JavaScriptCore/JavaScriptCore.exp
JavaScriptCore/kjs/JSLock.cpp
JavaScriptCore/kjs/JSLock.h
JavaScriptCore/kjs/collector.cpp
JavaScriptGlue/ChangeLog
JavaScriptGlue/JavaScriptGlue.cpp
WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/DumpRenderTree.m

index b6269d111ae266cde2d1196f938061f906b351ff..1e9a991b045e97e28593d8d29d12c4f45719bd5a 100644 (file)
@@ -1,3 +1,31 @@
+2007-03-11  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+        
+        Fixed <rdar://problem/4681051> Installer crashes in KJS::Collector::
+        markOtherThreadConservatively(KJS::Collector::Thread*) trying to install 
+        iLife 06 using Rosetta on an Intel Machine
+        
+        The problem was that our thread-specific data destructor would modify the
+        list of active JavaScript threads without holding the JSLock, corrupting
+        the list. Corruption was especially likely if one JavaScript thread exited 
+        while another was starting up.
+
+        * JavaScriptCore.exp:
+        * kjs/JSLock.cpp: Don't conflate locking the JSLock with registering a
+        thread, since the thread-specific data destructor needs to lock
+        without registering a thread. Instead, treat thread registration as a
+        part of the convenience of the JSLock object, and whittle down JSLock::lock()
+        to just the bits that actually do the locking.
+        (KJS::JSLock::lock):
+        (KJS::JSLock::registerThread):
+        * kjs/JSLock.h: Updated comments to mention the new behavior above, and
+        other recent changes.
+        (KJS::JSLock::JSLock):
+        * kjs/collector.cpp:
+        (KJS::destroyRegisteredThread): Lock here.
+        (KJS::Collector::registerThread): To match, assert that we're locked here.
+
 2007-03-10  Geoffrey Garen  <ggaren@apple.com>
 
         Reviewed by Darin Adler.
index b46651343b3a255aa3479e46057d2de514f1c1f2..bf82beba9214edfea922cbf31496f906cde19b9a 100644 (file)
@@ -164,6 +164,7 @@ __ZN3KJS6JSCell9getObjectEv
 __ZN3KJS6JSCellnwEm
 __ZN3KJS6JSLock12DropAllLocksC1Ev
 __ZN3KJS6JSLock12DropAllLocksD1Ev
+__ZN3KJS6JSLock14registerThreadEv
 __ZN3KJS6JSLock4lockEv
 __ZN3KJS6JSLock6unlockEv
 __ZN3KJS6JSLock9lockCountEv
index 48f597c3b9f8aed9ffed6c644e0ee3160fc098eb..c28450a372cdf361006fd066f46c0afccee564f6 100644 (file)
@@ -53,8 +53,6 @@ void JSLock::lock()
         pthread_setspecific(didLockJSMutex, &didLockJSMutex);
     }
     ++JSLockCount;
-
-    Collector::registerThread();
 }
 
 void JSLock::unlock()
@@ -69,6 +67,11 @@ void JSLock::unlock()
     }
 }
 
+void JSLock::registerThread()
+{
+    Collector::registerThread();
+}
+
 JSLock::DropAllLocks::DropAllLocks()
     : m_lockCount(0)
 {
@@ -100,6 +103,10 @@ void JSLock::unlock()
 {
 }
 
+void JSLock::registerThread()
+{
+}
+
 JSLock::DropAllLocks::DropAllLocks()
 {
 }
index 4a4a5d844bda4357fd9c8033761a1787ddb3d51a..9ddf6f18e2d1b362402624e312a1322ee3650620 100644 (file)
@@ -30,11 +30,12 @@ namespace KJS {
 
     // To make it safe to use JavaScript on multiple threads, it is
     // important to lock before doing anything that allocates a
-    // garbage-collected object or which may affect other shared state
-    // such as the protect count hash table. The simplest way to do
-    // this is by having a local JSLock object for the scope
-    // where the lock must be held. The lock is recursive so nesting
-    // is ok.
+    // JavaScript data structure or that interacts with shared state
+    // such as the protect count hash table. The simplest way to lock
+    // is to create a local JSLock object in the scope where the lock 
+    // must be held. The lock is recursive so nesting is ok. The JSLock 
+    // object also acts as a convenience short-hand for running important
+    // initialization routines.
 
     // To avoid deadlock, sometimes it is necessary to temporarily
     // release the lock. Since it is recursive you actually have to
@@ -47,9 +48,10 @@ namespace KJS {
 
     class JSLock : Noncopyable {
     public:
-        JSLock() 
+        JSLock()
         {
             lock();
+            registerThread();
         }
 
         ~JSLock() 
@@ -61,6 +63,8 @@ namespace KJS {
         static void unlock();
         static int lockCount();
 
+        static void registerThread();
+
         class DropAllLocks : Noncopyable {
         public:
             DropAllLocks();
index 4dfbce041c803d86c02b8377eb4566e4336141d8..eac0c82a3aecbe16d32a7a7452703a180c066758 100644 (file)
@@ -236,19 +236,27 @@ static void destroyRegisteredThread(void *data)
 {
   Collector::Thread *thread = (Collector::Thread *)data;
 
+  // Can't use JSLock convenience object here because we don't want to re-register
+  // an exiting thread.
+  JSLock::lock();
+  
   if (registeredThreads == thread) {
     registeredThreads = registeredThreads->next;
   } else {
     Collector::Thread *last = registeredThreads;
-    for (Collector::Thread *t = registeredThreads->next; t != NULL; t = t->next) {
+    Collector::Thread *t;
+    for (t = registeredThreads->next; t != NULL; t = t->next) {
       if (t == thread) {
         last->next = t->next;
           break;
       }
       last = t;
     }
+    ASSERT(t); // If t is NULL, we never found ourselves in the list.
   }
 
+  JSLock::unlock();
+
   delete thread;
 }
 
@@ -259,6 +267,8 @@ static void initializeRegisteredThreadKey()
 
 void Collector::registerThread()
 {
+  ASSERT(JSLock::lockCount() > 0);
+  
   pthread_once(&registeredThreadKeyOnce, initializeRegisteredThreadKey);
 
   if (!pthread_getspecific(registeredThreadKey)) {
index 202251a41f1d42cd441a74429c484da6a6ab2c05..649ffb6c0df70ea500d389eb93c8db3865fc02e1 100644 (file)
@@ -1,3 +1,17 @@
+2007-03-12  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+        
+        Updated in light of fix for <rdar://problem/4681051> Installer crashes 
+        in KJS::Collector::markOtherThreadConservatively(KJS::Collector::Thread*) 
+        trying to install iLife 06 using Rosetta on an Intel Machine
+        
+        * JavaScriptGlue.cpp:
+        (JSLockInterpreter): Ensure backwards compatibility by calling 
+        registerThread() when explicitly taking the JSLock. (This doesn't happen 
+        automatically anymore.) I doubt this actally matters, but in JavaScriptGlue
+        territory, that kind of thinking will get you killed.
+
 2007-03-06  Geoffrey Garen  <ggaren@apple.com>
 
         Reviewed by Maciej Stachowiak.
index e97dff6f76def332e08a6edece3c226c2659f6d7..456291b1f7325cb9792da54f0bcf36041d8d3908 100644 (file)
@@ -641,6 +641,7 @@ CFMutableArrayRef JSCreateJSArrayFromCFArray(CFArrayRef array)
 void JSLockInterpreter()
 {
     JSLock::lock();
+    JSLock::registerThread();
 }
 
 
index e72acce4826e6bc5bbce0a4853034ba91637c5ab..ae8dccfe988fffe1baf677c505afa587173c07ff 100644 (file)
@@ -1,3 +1,22 @@
+2007-03-11  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+        
+        Beefed up --threaded mode in light of <rdar://problem/4681051> Installer 
+        crashes in KJS::Collector::markOtherThreadConservatively(KJS::Collector::Thread*) 
+        trying to install iLife 06 using Rosetta on an Intel Machine
+        
+        --threaded mode now runs a bunch of different JavaScript threads, randomly
+        killing and respawning them. This was sufficient for reproducing the
+        bug on my MacBook Pro.
+
+        * DumpRenderTree/DumpRenderTree.m:
+        (javaScriptThreads):
+        (runJavaScriptThread):
+        (startJavaScriptThreads):
+        (stopJavaScriptThreads):
+        (dumpRenderTree):
+
 2007-03-11  Krzysztof Kowalczyk  <kkowalczyk@gmail.com>
 
         Reviewed by Brady Eidson.
index 8d5b58bc6e1bb5bf110b1420b83a2ca7d34e513b..308d4785b1a744a4190d971a9c18235687e0aedf 100644 (file)
@@ -157,13 +157,27 @@ static BOOL workQueueFrozen;
 const unsigned maxViewHeight = 600;
 const unsigned maxViewWidth = 800;
 
-// Loops forever, running a script
+static pthread_mutex_t javaScriptThreadsMutex = PTHREAD_MUTEX_INITIALIZER;
+static BOOL javaScriptThreadsShouldTerminate;
+
+static const int javaScriptThreadsCount = 4;
+static CFMutableDictionaryRef javaScriptThreads()
+{
+    assert(pthread_mutex_trylock(&javaScriptThreadsMutex) == EBUSY);
+    static CFMutableDictionaryRef staticJavaScriptThreads;
+    if (!staticJavaScriptThreads)
+        staticJavaScriptThreads = CFDictionaryCreateMutable(kCFAllocatorDefault, 0, NULL, NULL);
+    return staticJavaScriptThreads;
+}
+
+// Loops forever, running a script and randomly respawning, until 
+// javaScriptThreadsShouldTerminate becomes true.
 void* runJavaScriptThread(void* arg)
 {
-    char* script =
+    const char* const script =
     " \
     var array = []; \
-    for (var i = 0; i < 1000; i++) { \
+    for (var i = 0; i < 10; i++) { \
         array.push(String(i)); \
     } \
     ";
@@ -181,23 +195,61 @@ void* runJavaScriptThread(void* arg)
         
         JSGarbageCollect(ctx);
 
-        pthread_testcancel(); // Allow thread termination
+        pthread_mutex_lock(&javaScriptThreadsMutex);
+
+        // Check for cancellation.
+        if (javaScriptThreadsShouldTerminate) {
+            pthread_mutex_unlock(&javaScriptThreadsMutex);
+            return 0;
+        }
+
+        // Respawn probabilistically.
+        if (random() % 5 == 0) {
+            pthread_t pthread;
+            pthread_create(&pthread, NULL, &runJavaScriptThread, NULL);
+            pthread_detach(pthread);
+
+            CFDictionaryRemoveValue(javaScriptThreads(), pthread_self());
+            CFDictionaryAddValue(javaScriptThreads(), pthread, NULL);
+
+            pthread_mutex_unlock(&javaScriptThreadsMutex);
+            return 0;
+        }
+
+        pthread_mutex_unlock(&javaScriptThreadsMutex);
     }
 }
 
-static pthread_t javaScriptThread;
-
-static void startJavaScriptThread(void)
+static void startJavaScriptThreads(void)
 {
-    assert(!javaScriptThread);
-    pthread_create(&javaScriptThread, NULL, runJavaScriptThread, NULL);
+    pthread_mutex_lock(&javaScriptThreadsMutex);
+
+    for (int i = 0; i < javaScriptThreadsCount; i++) {
+        pthread_t pthread;
+        pthread_create(&pthread, NULL, &runJavaScriptThread, NULL);
+        pthread_detach(pthread);
+        CFDictionaryAddValue(javaScriptThreads(), pthread, NULL);
+    }
+
+    pthread_mutex_unlock(&javaScriptThreadsMutex);
 }
 
-static void stopJavaScriptThread(void)
+static void stopJavaScriptThreads(void)
 {
-    assert(javaScriptThread);
-    pthread_cancel(javaScriptThread);
-    javaScriptThread = NULL;
+    pthread_mutex_lock(&javaScriptThreadsMutex);
+
+    javaScriptThreadsShouldTerminate = YES;
+
+    const pthread_t pthreads[javaScriptThreadsCount];
+    assert(CFDictionaryGetCount(javaScriptThreads()) == javaScriptThreadsCount);
+    CFDictionaryGetKeysAndValues(javaScriptThreads(), (const void**)pthreads, NULL);
+
+    pthread_mutex_unlock(&javaScriptThreadsMutex);
+
+    for (int i = 0; i < javaScriptThreadsCount; i++) {
+        pthread_t pthread = pthreads[i];
+        pthread_join(pthread, NULL);
+    }
 }
 
 static BOOL shouldIgnoreWebCoreNodeLeaks(CFStringRef URLString)
@@ -427,7 +479,7 @@ void dumpRenderTree(int argc, const char *argv[])
     [webView cacheDisplayInRect:[webView bounds] toBitmapImageRep:imageRep];
 
     if (threaded)
-        startJavaScriptThread();
+        startJavaScriptThreads();
     
     if (argc == optind+1 && strcmp(argv[optind], "-") == 0) {
         char filenameBuffer[2048];
@@ -449,7 +501,7 @@ void dumpRenderTree(int argc, const char *argv[])
     }
 
     if (threaded)
-        stopJavaScriptThread();
+        stopJavaScriptThreads();
     
     [workQueue release];