JavaScriptCore:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Mar 2007 04:25:49 +0000 (04:25 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Mar 2007 04:25:49 +0000 (04:25 +0000)
        Reviewed by Maciej Stachowiak.

        Fixed all known crashers exposed by run-webkit-tests --threaded. This covers:

        <rdar://problem/4565394> | http://bugs.webkit.org/show_bug.cgi?id=12585
            PAC file: after closing a window that contains macworld.com, new window
            crashes (KJS::PropertyMap::mark()) (12585)
        <rdar://problem/4571215> | http://bugs.webkit.org/show_bug.cgi?id=9211
            PAC file: Crash occurs when clicking on the navigation tabs at http://www.businessweek.com/ (9211)
        <rdar://problem/4557926>
            PAC file: Crash occurs when attempting to view image in slideshow mode
            at http://d.smugmug.com/gallery/581716 ( KJS::IfNode::execute (KJS::
            ExecState*) + 312) if you use a PAC file

        (1) Added some missing JSLocks, along with related ASSERTs.

        (2) Fully implemented support for objects that can only be garbage collected
        on the main thread. So far, only WebCore uses this. We can add it to API
        later if we learn that it's needed.

        The implementation uses a "main thread only" flag inside each object. When
        collecting on a secondary thread, the Collector does an extra pass through
        the heap to mark all flagged objects before sweeping. This solution makes
        the common case -- flag lots of objects, but never collect on a secondary
        thread -- very fast, even though the uncommon case of garbage collecting
        on a secondary thread isn't as fast as it could be. I left some notes
        about how to speed it up, if we ever care.

        For posterity, here are some things I learned about GC while investigating:

        * Each collect must either mark or delete every heap object. "Zombie"
        objects, which are neither marked nor deleted, raise these issues:

            * On the next pass, the conservative marking algorithm might mark a
            zombie, causing it to mark freed objects.

            * The client might try to use a zombie, which would seem live because
            its finalizer had not yet run.

        * A collect on the main thread is free to delete any object. Presumably,
        objects allocated on secondary threads have thread-safe finalizers.

        * A collect on a secondary thread must not delete thread-unsafe objects.

        * The mark function must be thread-safe.

        Line by line comments:

        * API/JSObjectRef.h: Added comment specifying that the finalize callback
        may run on any thread.

        * JavaScriptCore.exp: Nothing to see here.

        * bindings/npruntime.cpp:
        (_NPN_GetStringIdentifier): Added JSLock.

        * bindings/objc/objc_instance.h:
        * bindings/objc/objc_instance.mm:
        (ObjcInstance::~ObjcInstance): Use an autorelease pool. The other callers
        to CFRelease needed one, too, but they were dead code, so I removed them
        instead. (This fixes a leak seen while running run-webkit-tests --threaded,
        although I don't think it's specifically a threading issue.)

        * kjs/collector.cpp:
        (KJS::Collector::collectOnMainThreadOnly): New function. Tells the collector
        to collect a value only if it's collecting on the main thread.
        (KJS::Collector::markMainThreadOnlyObjects): New function. Scans the heap
        for "main thread only" objects and marks them.

        * kjs/date_object.cpp:
        (KJS::DateObjectImp::DateObjectImp): To make the new ASSERTs happy, allocate
        our globals on the heap, avoiding a seemingly unsafe destructor call at
        program exit time.
        * kjs/function_object.cpp:
        (FunctionPrototype::FunctionPrototype): ditto

        * kjs/interpreter.cpp:
        (KJS::Interpreter::mark): Removed boolean parameter, which was an incomplete
        and arguably hackish way to implement markMainThreadOnlyObjects() inside WebCore.
        * kjs/interpreter.h:

        * kjs/identifier.cpp:
        (KJS::identifierTable): Added some ASSERTs to check for thread safety
        problems.

        * kjs/list.cpp: Added some ASSERTs to check for thread safety problems.
        (KJS::allocateListImp):
        (KJS::List::release):
        (KJS::List::append):
        (KJS::List::empty): Make the new ASSERTs happy.

        * kjs/object.h:
        (KJS::JSObject::JSObject): "m_destructorIsThreadSafe" => "m_collectOnMainThreadOnly".
        I removed the constructor parameter because m_collectOnMainThreadOnly,
        like m_marked, is a Collector bit, so only the Collector should set or get it.

        * kjs/object_object.cpp:
        (ObjectPrototype::ObjectPrototype): Make the ASSERTs happy.
        * kjs/regexp_object.cpp:
        (RegExpPrototype::RegExpPrototype): ditto

        * kjs/ustring.cpp: Added some ASSERTs to check for thread safety problems.
        (KJS::UCharReference::ref):
        (KJS::UString::Rep::createCopying):
        (KJS::UString::Rep::create):
        (KJS::UString::Rep::destroy):
        (KJS::UString::null): Make the new ASSERTs happy.
        * kjs/ustring.h:
        (KJS::UString::Rep::ref): Added some ASSERTs to check for thread safety problems.
        (KJS::UString::Rep::deref):

        * kjs/value.h:
        (KJS::JSCell::JSCell):

JavaScriptGlue:

        Reviewed by Maciej Stachowiak.

        Fixed all known crashers exposed by run-webkit-tests --threaded while using
        a PAC file (for maximum carnage). See JavaScriptCore ChangeLog for
        more details.

        * JSBase.cpp:
        (JSBase::Release): Lock when deleting, because we may be deleting an object
        (like a JSRun) that holds thread-unsafe data.

        * JSUtils.cpp:
        (CFStringToUString): Don't lock, because our caller locks. Also, locking
        inside a function that returns thread-unsafe data by copy will only mask
        threading problems.

        * JavaScriptGlue.cpp:
        (JSRunEvaluate): Added missing JSLock.
        (JSRunCheckSyntax): Converted to JSLock.
        * JavaScriptGlue.xcodeproj/project.pbxproj:

WebCore:

        Reviewed by Maciej Stachowiak.

        Fixed all known crashers exposed by run-webkit-tests --threaded [*]. See
        JavaScriptCore ChangeLog for more details.

        * bindings/js/kjs_binding.cpp:
        (KJS::domNodesPerDocument): Added thread safety ASSERT.
        (KJS::ScriptInterpreter::mark): Removed obsolete logic for marking unsafe
        objects when collecting on a secondary thread. The Collector takes care
        of this now.

        * bindings/js/kjs_binding.h:
        (KJS::DOMObject::DOMObject): Used new API for specifying that WebCore
        objects should be garbage collected on the main thread only.

        * bindings/js/kjs_window.cpp:
        (KJS::ScheduledAction::execute): Moved JSLock to cover implementedsCall() call,
        which, for some subclasses, ends up allocating garbage collected objects.
        (This fix was speculative. I didn't actually see a crash from this.)
        (KJS::Window::timerFired): Added JSLock around ScheduleAction destruction,
        since it destroys a KJS::List.

        * bindings/objc/WebScriptObject.mm:
        (-[WebScriptObject setException:]): Added JSLock. (This fix was speculative.
        I didn't actually see a crash from this.)

        * bridge/mac/WebCoreScriptDebugger.mm:
        (-[WebCoreScriptCallFrame evaluateWebScript:]): Added JSLock. (This fix
        was speculative. I didn't actually see a crash from this.)

        * dom/Document.cpp:
        (WebCore::Document::~Document): Added JSLock around modification to
        domNodesPerDocument(), which can be accessed concurrently during garbage
        collection.
        * dom/Node.cpp:
        (WebCore::Node::setDocument): ditto.

        [*] fast/js/toString-stack-overflow.html is an exception. --threaded mode
        crashes this test because it causes the garbage collector to run frequently,
        and this test crashes if you happen to garbage collect while it's running.
        This is a known issue with stack overflow during the mark phase. It's
        not related to threading.

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

33 files changed:
JavaScriptCore/API/JSObjectRef.h
JavaScriptCore/ChangeLog
JavaScriptCore/JavaScriptCore.exp
JavaScriptCore/bindings/npruntime.cpp
JavaScriptCore/bindings/objc/objc_instance.h
JavaScriptCore/bindings/objc/objc_instance.mm
JavaScriptCore/kjs/collector.cpp
JavaScriptCore/kjs/collector.h
JavaScriptCore/kjs/date_object.cpp
JavaScriptCore/kjs/function_object.cpp
JavaScriptCore/kjs/identifier.cpp
JavaScriptCore/kjs/interpreter.cpp
JavaScriptCore/kjs/interpreter.h
JavaScriptCore/kjs/list.cpp
JavaScriptCore/kjs/object.h
JavaScriptCore/kjs/object_object.cpp
JavaScriptCore/kjs/regexp_object.cpp
JavaScriptCore/kjs/ustring.cpp
JavaScriptCore/kjs/ustring.h
JavaScriptCore/kjs/value.h
JavaScriptGlue/ChangeLog
JavaScriptGlue/JSBase.cpp
JavaScriptGlue/JSUtils.cpp
JavaScriptGlue/JavaScriptGlue.cpp
JavaScriptGlue/JavaScriptGlue.xcodeproj/project.pbxproj
WebCore/ChangeLog
WebCore/bindings/js/kjs_binding.cpp
WebCore/bindings/js/kjs_binding.h
WebCore/bindings/js/kjs_window.cpp
WebCore/bindings/objc/WebScriptObject.mm
WebCore/bridge/mac/WebCoreScriptDebugger.mm
WebCore/dom/Document.cpp
WebCore/dom/Node.cpp

index 9abc60a987c387efd6e6cda419a4fc740832e0c9..aa87c4cc9c0ee96b49cd8628cf5f5a4d4a303e1e 100644 (file)
@@ -90,7 +90,7 @@ typedef void
 
 /*! 
 @typedef JSObjectFinalizeCallback
-@abstract The callback invoked when an object is finalized (prepared for garbage collection).
+@abstract The callback invoked when an object is finalized (prepared for garbage collection). An object may be finalized on any thread.
 @param object The JSObject being finalized.
 @discussion If you named your function Finalize, you would declare it like this:
 
index fda3c69dafe169319d85aedf82330916e8b03a78..872a21b44d4551a87e892fea1b75959c8bef9052 100644 (file)
@@ -1,3 +1,119 @@
+2007-03-06  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+        
+        Fixed all known crashers exposed by run-webkit-tests --threaded. This covers:
+
+        <rdar://problem/4565394> | http://bugs.webkit.org/show_bug.cgi?id=12585 
+            PAC file: after closing a window that contains macworld.com, new window 
+            crashes (KJS::PropertyMap::mark()) (12585)
+        <rdar://problem/4571215> | http://bugs.webkit.org/show_bug.cgi?id=9211
+            PAC file: Crash occurs when clicking on the navigation tabs at http://www.businessweek.com/ (9211)
+        <rdar://problem/4557926> 
+            PAC file: Crash occurs when attempting to view image in slideshow mode 
+            at http://d.smugmug.com/gallery/581716 ( KJS::IfNode::execute (KJS::
+            ExecState*) + 312) if you use a PAC file
+
+        (1) Added some missing JSLocks, along with related ASSERTs.
+        
+        (2) Fully implemented support for objects that can only be garbage collected
+        on the main thread. So far, only WebCore uses this. We can add it to API
+        later if we learn that it's needed. 
+        
+        The implementation uses a "main thread only" flag inside each object. When 
+        collecting on a secondary thread, the Collector does an extra pass through 
+        the heap to mark all flagged objects before sweeping. This solution makes
+        the common case -- flag lots of objects, but never collect on a secondary 
+        thread -- very fast, even though the uncommon case of garbage collecting
+        on a secondary thread isn't as fast as it could be. I left some notes 
+        about how to speed it up, if we ever care.
+        
+        For posterity, here are some things I learned about GC while investigating:
+        
+        * Each collect must either mark or delete every heap object. "Zombie" 
+        objects, which are neither marked nor deleted, raise these issues:
+
+            * On the next pass, the conservative marking algorithm might mark a 
+            zombie, causing it to mark freed objects.
+
+            * The client might try to use a zombie, which would seem live because 
+            its finalizer had not yet run.
+
+        * A collect on the main thread is free to delete any object. Presumably, 
+        objects allocated on secondary threads have thread-safe finalizers.
+
+        * A collect on a secondary thread must not delete thread-unsafe objects.
+
+        * The mark function must be thread-safe.
+        
+        Line by line comments:
+
+        * API/JSObjectRef.h: Added comment specifying that the finalize callback 
+        may run on any thread.
+
+        * JavaScriptCore.exp: Nothing to see here.
+
+        * bindings/npruntime.cpp:
+        (_NPN_GetStringIdentifier): Added JSLock.
+
+        * bindings/objc/objc_instance.h:
+        * bindings/objc/objc_instance.mm:
+        (ObjcInstance::~ObjcInstance): Use an autorelease pool. The other callers 
+        to CFRelease needed one, too, but they were dead code, so I removed them 
+        instead. (This fixes a leak seen while running run-webkit-tests --threaded,
+        although I don't think it's specifically a threading issue.) 
+        
+        * kjs/collector.cpp:
+        (KJS::Collector::collectOnMainThreadOnly): New function. Tells the collector
+        to collect a value only if it's collecting on the main thread.
+        (KJS::Collector::markMainThreadOnlyObjects): New function. Scans the heap
+        for "main thread only" objects and marks them.
+
+        * kjs/date_object.cpp: 
+        (KJS::DateObjectImp::DateObjectImp): To make the new ASSERTs happy, allocate 
+        our globals on the heap, avoiding a seemingly unsafe destructor call at 
+        program exit time.
+        * kjs/function_object.cpp:
+        (FunctionPrototype::FunctionPrototype): ditto
+
+        * kjs/interpreter.cpp:
+        (KJS::Interpreter::mark): Removed boolean parameter, which was an incomplete
+        and arguably hackish way to implement markMainThreadOnlyObjects() inside WebCore.
+        * kjs/interpreter.h:
+
+        * kjs/identifier.cpp:
+        (KJS::identifierTable): Added some ASSERTs to check for thread safety 
+        problems.
+
+        * kjs/list.cpp: Added some ASSERTs to check for thread safety problems.
+        (KJS::allocateListImp):
+        (KJS::List::release):
+        (KJS::List::append):
+        (KJS::List::empty): Make the new ASSERTs happy.
+
+        * kjs/object.h:
+        (KJS::JSObject::JSObject): "m_destructorIsThreadSafe" => "m_collectOnMainThreadOnly".
+        I removed the constructor parameter because m_collectOnMainThreadOnly,
+        like m_marked, is a Collector bit, so only the Collector should set or get it.
+
+        * kjs/object_object.cpp:
+        (ObjectPrototype::ObjectPrototype): Make the ASSERTs happy.
+        * kjs/regexp_object.cpp:
+        (RegExpPrototype::RegExpPrototype): ditto
+
+        * kjs/ustring.cpp: Added some ASSERTs to check for thread safety problems.
+        (KJS::UCharReference::ref): 
+        (KJS::UString::Rep::createCopying):
+        (KJS::UString::Rep::create):
+        (KJS::UString::Rep::destroy):
+        (KJS::UString::null): Make the new ASSERTs happy.
+        * kjs/ustring.h:
+        (KJS::UString::Rep::ref): Added some ASSERTs to check for thread safety problems.
+        (KJS::UString::Rep::deref):
+
+        * kjs/value.h:
+        (KJS::JSCell::JSCell):
+
 2007-03-06  Geoffrey Garen  <ggaren@apple.com>
 
         Reviewed by Maciej Stachowiak.
index 7376bfc507716162da38cd513dc27e37cc618078..b46651343b3a255aa3479e46057d2de514f1c1f2 100644 (file)
@@ -117,7 +117,7 @@ __ZN3KJS11Interpreter16stopTimeoutCheckEv
 __ZN3KJS11Interpreter17startTimeoutCheckEv
 __ZN3KJS11Interpreter21shouldPrintExceptionsEv
 __ZN3KJS11Interpreter24setShouldPrintExceptionsEb
-__ZN3KJS11Interpreter4markEb
+__ZN3KJS11Interpreter4markEv
 __ZN3KJS11Interpreter6s_hookE
 __ZN3KJS11Interpreter8evaluateERKNS_7UStringEiPKNS_5UCharEiPNS_7JSValueE
 __ZN3KJS11Interpreter8evaluateERKNS_7UStringEiS3_PNS_7JSValueE
@@ -166,6 +166,7 @@ __ZN3KJS6JSLock12DropAllLocksC1Ev
 __ZN3KJS6JSLock12DropAllLocksD1Ev
 __ZN3KJS6JSLock4lockEv
 __ZN3KJS6JSLock6unlockEv
+__ZN3KJS6JSLock9lockCountEv
 __ZN3KJS6Lookup9findEntryEPKNS_9HashTableERKNS_10IdentifierE
 __ZN3KJS6Parser11prettyPrintERKNS_7UStringEPiPS1_
 __ZN3KJS7CStringD1Ev
@@ -214,6 +215,7 @@ __ZN3KJS8jsStringERKNS_7UStringE
 __ZN3KJS9Collector15numInterpretersEv
 __ZN3KJS9Collector19numProtectedObjectsEv
 __ZN3KJS9Collector20rootObjectTypeCountsEv
+__ZN3KJS9Collector23collectOnMainThreadOnlyEPNS_7JSValueE
 __ZN3KJS9Collector4sizeEv
 __ZN3KJS9Collector7collectEv
 __ZN3KJS9Collector7protectEPNS_7JSValueE
@@ -273,6 +275,7 @@ __ZNK3KJS9ExecState18lexicalInterpreterEv
 __ZTVN3KJS14StringInstanceE
 __ZTVN3KJS15JSWrapperObjectE
 __ZTVN3KJS19InternalFunctionImpE
+__ZTVN3KJS6JSCellE
 __ZTVN3KJS8JSObjectE
 _kJSClassDefinitionEmpty
 _kjs_pcre_compile
index cac6de7d94a2eb678e82f75c7f95bb0425469dfe..1e0887603943af1c203e54bef0889be7120ecd4c 100644 (file)
@@ -28,6 +28,7 @@
 #include "npruntime_impl.h"
 #include "npruntime_priv.h"
 
+#include "JSLock.h"
 #include "c_utility.h"
 #include "identifier.h"
 #include <wtf/Assertions.h>
@@ -62,6 +63,8 @@ NPIdentifier _NPN_GetStringIdentifier(const NPUTF8* name)
     if (name) {
         PrivateIdentifier* identifier = 0;
         
+        KJS::JSLock lock;
+        
         identifier = getStringIdentifierMap()->get(identifierFromNPIdentifier(name).ustring().rep());
         if (identifier == 0) {
             identifier = (PrivateIdentifier*)malloc(sizeof(PrivateIdentifier));
index e97a0baf95f009b56f57e4f0f3d9ea59a25663f8..8c17f09d9afd39201d3a8dfc877232a4789be0c5 100644 (file)
@@ -43,10 +43,6 @@ public:
     
     virtual Class *getClass() const;
     
-    ObjcInstance(const ObjcInstance &other);
-
-    ObjcInstance &operator=(const ObjcInstance &other);
-    
     virtual void begin();
     virtual void end();
     
@@ -72,6 +68,9 @@ public:
     JSValue *booleanValue() const;
     
 private:
+    ObjcInstance(const ObjcInstance& other);
+    ObjcInstance& operator=(const ObjcInstance& other);
+    
     ObjectStructPtr _instance;
     mutable ObjcClass *_class;
     ObjectStructPtr _pool;
index a02846bdc0e3241f3368a0db910fee94444910aa..ddf91ced8dfc39fb7e8d5b0b4e73a3a0a9691050 100644 (file)
@@ -52,33 +52,12 @@ ObjcInstance::ObjcInstance(ObjectStructPtr instance)
 
 ObjcInstance::~ObjcInstance() 
 {
+    begin(); // -finalizeForWebScript and -dealloc/-finalize may require autorelease pools.
     if ([_instance respondsToSelector:@selector(finalizeForWebScript)])
         [_instance performSelector:@selector(finalizeForWebScript)];
     if (_instance)
         CFRelease(_instance);
-}
-
-ObjcInstance::ObjcInstance(const ObjcInstance &other) : Instance() 
-{
-    _instance = other._instance;
-    if (_instance)
-        CFRetain(_instance);
-    _class = other._class;
-    _pool = 0;
-    _beginCount = 0;
-}
-
-ObjcInstance &ObjcInstance::operator=(const ObjcInstance &other)
-{
-    ObjectStructPtr _oldInstance = _instance;
-    _instance = other._instance;
-    if (_instance)
-        CFRetain(_instance);
-    if (_oldInstance)
-        CFRelease(_oldInstance);
-    // Classes are kept around forever.
-    _class = other._class;
-    return* this;
+    end();
 }
 
 void ObjcInstance::begin()
index 01f3c192c126a80615fe12c693914d59dbd03a46..c9c912bdef7687672279d20fddc749a60e992698 100644 (file)
@@ -108,6 +108,7 @@ struct CollectorHeap {
 
 static CollectorHeap heap = {NULL, 0, 0, 0, NULL, 0, 0, 0, 0};
 
+size_t Collector::mainThreadOnlyObjectCount = 0;
 bool Collector::memoryFull = false;
 
 #ifndef NDEBUG
@@ -473,6 +474,19 @@ void Collector::unprotect(JSValue *k)
     protectedValues().remove(k->downcast());
 }
 
+void Collector::collectOnMainThreadOnly(JSValue* value)
+{
+    ASSERT(value);
+    ASSERT(JSLock::lockCount() > 0);
+
+    if (JSImmediate::isImmediate(value))
+      return;
+
+    JSCell* cell = value->downcast();
+    cell->m_collectOnMainThreadOnly = true;
+    ++mainThreadOnlyObjectCount;
+}
+
 void Collector::markProtectedObjects()
 {
   ProtectCountSet& protectedValues = KJS::protectedValues();
@@ -484,6 +498,56 @@ void Collector::markProtectedObjects()
   }
 }
 
+void Collector::markMainThreadOnlyObjects()
+{
+    ASSERT(!pthread_main_np());
+
+    // Optimization for clients that never register "main thread only" objects.
+    if (!mainThreadOnlyObjectCount)
+        return;
+
+    // FIXME: We can optimize this marking algorithm by keeping an exact set of 
+    // "main thread only" objects when the "main thread only" object count is 
+    // small. We don't want to keep an exact set all the time, because WebCore 
+    // tends to create lots of "main thread only" objects, and all that set 
+    // thrashing can be expensive.
+    
+    size_t count = 0;
+    
+    for (size_t block = 0; block < heap.usedBlocks; block++) {
+        ASSERT(count < mainThreadOnlyObjectCount);
+        
+        CollectorBlock* curBlock = heap.blocks[block];
+        size_t minimumCellsToProcess = curBlock->usedCells;
+        for (size_t i = 0; (i < minimumCellsToProcess) & (i < CELLS_PER_BLOCK); i++) {
+            CollectorCell* cell = curBlock->cells + i;
+            if (cell->u.freeCell.zeroIfFree == 0)
+                ++minimumCellsToProcess;
+            else {
+                JSCell* imp = reinterpret_cast<JSCell*>(cell);
+                if (imp->m_collectOnMainThreadOnly) {
+                    if(!imp->marked())
+                        imp->mark();
+                    if (++count == mainThreadOnlyObjectCount)
+                        return;
+                }
+            }
+        }
+    }
+
+    for (size_t cell = 0; cell < heap.usedOversizeCells; cell++) {
+        ASSERT(count < mainThreadOnlyObjectCount);
+
+        JSCell* imp = reinterpret_cast<JSCell*>(heap.oversizeCells[cell]);
+        if (imp->m_collectOnMainThreadOnly) {
+            if (!imp->marked())
+                imp->mark();
+            if (++count == mainThreadOnlyObjectCount)
+                return;
+        }
+    }
+}
+
 bool Collector::collect()
 {
   assert(JSLock::lockCount() > 0);
@@ -501,7 +565,7 @@ bool Collector::collect()
   if (Interpreter::s_hook) {
     Interpreter* scr = Interpreter::s_hook;
     do {
-      scr->mark(currentThreadIsMainThread);
+      scr->mark();
       scr = scr->next;
     } while (scr != Interpreter::s_hook);
   }
@@ -511,6 +575,8 @@ bool Collector::collect()
   markStackObjectsConservatively();
   markProtectedObjects();
   List::markProtectedLists();
+  if (!currentThreadIsMainThread)
+    markMainThreadOnlyObjects();
 
   // SWEEP: delete everything with a zero refcount (garbage) and unmark everything else
   
@@ -530,12 +596,16 @@ bool Collector::collect()
         JSCell *imp = reinterpret_cast<JSCell *>(cell);
         if (imp->m_marked) {
           imp->m_marked = false;
-        } else if (currentThreadIsMainThread || imp->m_destructorIsThreadSafe) {
+        } else {
           // special case for allocated but uninitialized object
-          // (We don't need this check earlier because nothing prior this point assumes the object has a valid vptr.)
+          // (We don't need this check earlier because nothing prior this point 
+          // assumes the object has a valid vptr.)
           if (cell->u.freeCell.zeroIfFree == 0)
             continue;
 
+          ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+          if (imp->m_collectOnMainThreadOnly)
+            --mainThreadOnlyObjectCount;
           imp->~JSCell();
           --usedCells;
           --numLiveObjects;
@@ -556,7 +626,10 @@ bool Collector::collect()
           JSCell *imp = reinterpret_cast<JSCell *>(cell);
           if (imp->m_marked) {
             imp->m_marked = false;
-          } else if (currentThreadIsMainThread || imp->m_destructorIsThreadSafe) {
+          } else {
+            ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+            if (imp->m_collectOnMainThreadOnly)
+              --mainThreadOnlyObjectCount;
             imp->~JSCell();
             --usedCells;
             --numLiveObjects;
@@ -599,7 +672,13 @@ bool Collector::collect()
   while (cell < heap.usedOversizeCells) {
     JSCell *imp = (JSCell *)heap.oversizeCells[cell];
     
-    if (!imp->m_marked && (currentThreadIsMainThread || imp->m_destructorIsThreadSafe)) {
+    if (imp->m_marked) {
+      imp->m_marked = false;
+      cell++;
+    } else {
+      ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+      if (imp->m_collectOnMainThreadOnly)
+        --mainThreadOnlyObjectCount;
       imp->~JSCell();
 #if DEBUG_COLLECTOR
       heap.oversizeCells[cell]->u.freeCell.zeroIfFree = 0;
@@ -617,9 +696,6 @@ bool Collector::collect()
         heap.numOversizeCells = heap.numOversizeCells / GROWTH_FACTOR; 
         heap.oversizeCells = (CollectorCell **)fastRealloc(heap.oversizeCells, heap.numOversizeCells * sizeof(CollectorCell *));
       }
-    } else {
-      imp->m_marked = false;
-      cell++;
     }
   }
   
index d4eee9c38b8b277f89f3301e5603234fed148e4c..43adee00dc6840e527ee796df7ba17c01849aaf7 100644 (file)
 
 namespace KJS {
 
-  /**
-   * @short Garbage collector.
-   */
   class Collector {
-    // disallow direct construction/destruction
-    Collector();
   public:
     static void* allocate(size_t s);
     static bool collect();
@@ -53,6 +48,8 @@ namespace KJS {
 
     static void protect(JSValue*);
     static void unprotect(JSValue*);
+    
+    static void collectOnMainThreadOnly(JSValue*);
 
     static size_t numInterpreters();
     static size_t numProtectedObjects();
@@ -60,15 +57,18 @@ namespace KJS {
 
     class Thread;
     static void registerThread();
-
+    
   private:
+    Collector();
 
     static void markProtectedObjects();
+    static void markMainThreadOnlyObjects();
     static void markCurrentThreadConservatively();
     static void markOtherThreadConservatively(Thread* thread);
     static void markStackObjectsConservatively();
     static void markStackObjectsConservatively(void* start, void* end);
 
+    static size_t mainThreadOnlyObjectCount;
     static bool memoryFull;
   };
 
index 8674bfc64237e933348bfe06e332c32103c042d1..3afcfbdfb885fc97a71a19488ae567dc5ca4ab7a 100644 (file)
@@ -596,15 +596,12 @@ DateObjectImp::DateObjectImp(ExecState *exec,
                              DatePrototype *dateProto)
   : InternalFunctionImp(funcProto)
 {
-  // ECMA 15.9.4.1 Date.prototype
-  putDirect(prototypePropertyName, dateProto, DontEnum|DontDelete|ReadOnly);
-
-  static const Identifier parsePropertyName("parse");
-  putDirectFunction(new DateObjectFuncImp(exec, funcProto, DateObjectFuncImp::Parse, 1, parsePropertyName), DontEnum);
-  static const Identifier UTCPropertyName("UTC");
-  putDirectFunction(new DateObjectFuncImp(exec, funcProto, DateObjectFuncImp::UTC, 7, UTCPropertyName), DontEnum);
+  static const Identifier* parsePropertyName = new Identifier("parse");
+  static const Identifier* UTCPropertyName = new Identifier("UTC");
 
-  // no. of arguments for constructor
+  putDirect(prototypePropertyName, dateProto, DontEnum|DontDelete|ReadOnly);
+  putDirectFunction(new DateObjectFuncImp(exec, funcProto, DateObjectFuncImp::Parse, 1, *parsePropertyName), DontEnum);
+  putDirectFunction(new DateObjectFuncImp(exec, funcProto, DateObjectFuncImp::UTC, 7, *UTCPropertyName), DontEnum);
   putDirect(lengthPropertyName, 7, ReadOnly|DontDelete|DontEnum);
 }
 
index c8ec2a8054562e3d08aa7a5ac6981dd54a54a945..daf270031ff36f5c11d6619e1d65cc9040acdbe5 100644 (file)
@@ -40,12 +40,13 @@ using namespace KJS;
 
 FunctionPrototype::FunctionPrototype(ExecState *exec)
 {
+  static const Identifier* applyPropertyName = new Identifier("apply");
+  static const Identifier* callPropertyName = new Identifier("call");
+
   putDirect(lengthPropertyName, jsNumber(0), DontDelete|ReadOnly|DontEnum);
   putDirectFunction(new FunctionProtoFunc(exec, this, FunctionProtoFunc::ToString, 0, toStringPropertyName), DontEnum);
-  static const Identifier applyPropertyName("apply");
-  putDirectFunction(new FunctionProtoFunc(exec, this, FunctionProtoFunc::Apply, 2, applyPropertyName), DontEnum);
-  static const Identifier callPropertyName("call");
-  putDirectFunction(new FunctionProtoFunc(exec, this, FunctionProtoFunc::Call, 1, callPropertyName), DontEnum);
+  putDirectFunction(new FunctionProtoFunc(exec, this, FunctionProtoFunc::Apply, 2, *applyPropertyName), DontEnum);
+  putDirectFunction(new FunctionProtoFunc(exec, this, FunctionProtoFunc::Call, 1, *callPropertyName), DontEnum);
 }
 
 FunctionPrototype::~FunctionPrototype()
index 01a0bdc95b5413930744fadfe467d8411a8cddc2..115760c1edfba5494ad8d200bb0fa6ef48402db5 100644 (file)
 
 #include "identifier.h"
 
+#include "JSLock.h"
+#include <new> // for placement new
+#include <string.h> // for strlen
+#include <wtf/Assertions.h>
 #include <wtf/FastMalloc.h>
 #include <wtf/HashSet.h>
-#include <string.h> // for strlen
-#include <new> // for placement new
 
 namespace WTF {
 
@@ -66,6 +68,8 @@ static IdentifierTable *table;
 
 static inline IdentifierTable& identifierTable()
 {
+    ASSERT(JSLock::lockCount() > 0);
+
     if (!table)
         table = new IdentifierTable;
     return *table;
index c11288a679b1427a0cab962c57ca2f5704a9aea5..066397cafb730f73bb63099ff53931ab3f0f4f0e 100644 (file)
@@ -537,7 +537,7 @@ JSObject *Interpreter::builtinURIErrorPrototype() const
   return m_UriErrorPrototype;
 }
 
-void Interpreter::mark(bool)
+void Interpreter::mark()
 {
     if (m_context)
         m_context->mark();
index aebd0de20f3889e269a62b79096422291c7d71ad..d608281b9dc3d52273e09a62e58ed621d7678da6 100644 (file)
@@ -286,7 +286,7 @@ namespace KJS {
      * Called during the mark phase of the garbage collector. Subclasses 
      * implementing custom mark methods must make sure to chain to this one.
      */
-    virtual void mark(bool currentThreadIsMainThread);
+    virtual void mark();
 
 #ifdef KJS_DEBUG_MEM
     /**
index f73170469fa8e89512d608eb53156571c9e0c925..e40fa86856defad96a9d721c9ffbab4d43af8cb3 100644 (file)
@@ -141,6 +141,8 @@ void List::markProtectedLists()
 
 static inline ListImp *allocateListImp()
 {
+    ASSERT(JSLock::lockCount() > 0);
+    
     // Find a free one in the pool.
     if (poolUsed < poolSize) {
         ListImp *imp = poolFreeList ? poolFreeList : &pool[0];
@@ -201,7 +203,9 @@ void List::markValues()
 }
 
 void List::release()
-{
+{   
+    ASSERT(JSLock::lockCount() > 0);
+    
     ListImp *imp = static_cast<ListImp *>(_impBase);
     
 #if DUMP_STATISTICS
@@ -221,7 +225,7 @@ void List::release()
         poolFreeList = imp;
         poolUsed--;
     } else {
-        assert(imp->state == usedOnHeap);
+        ASSERT(imp->state == usedOnHeap);
         HeapListImp *list = static_cast<HeapListImp *>(imp);
 
         // unlink from heap list
@@ -258,6 +262,8 @@ void List::clear()
 
 void List::append(JSValue *v)
 {
+    ASSERT(JSLock::lockCount() > 0);
+    
     ListImp *imp = static_cast<ListImp *>(_impBase);
 
     int i = imp->size++;
@@ -331,10 +337,10 @@ List List::copyTail() const
     return copy;
 }
 
-const List &List::empty()
+const ListList::empty()
 {
-    static List emptyList;
-    return emptyList;
+    static List* staticEmptyList = new List;
+    return *staticEmptyList;
 }
 
 List &List::operator=(const List &b)
index 13dfceb889be9df3e9f44a7fa4b090f7043731e8..0a2e49baa1dcc16ac54f34ebb726eb0c5ba86c1f 100644 (file)
@@ -106,13 +106,13 @@ namespace KJS {
      *
      * @param proto The prototype
      */
-    JSObject(JSValue* proto, bool destructorIsThreadSafe = true);
+    JSObject(JSValue* proto);
 
     /**
      * Creates a new JSObject with a prototype of jsNull()
      * (that is, the ECMAScript "null" value, not a null object pointer).
      */
-    explicit JSObject(bool destructorIsThreadSafe = true);
+    JSObject();
 
     virtual void mark();
     virtual JSType type() const;
@@ -502,16 +502,14 @@ JSObject *throwError(ExecState *, ErrorType, const UString &message);
 JSObject *throwError(ExecState *, ErrorType, const char *message);
 JSObject *throwError(ExecState *, ErrorType);
 
-inline JSObject::JSObject(JSValue* proto, bool destructorIsThreadSafe)
-    : JSCell(destructorIsThreadSafe)
-    , _proto(proto)
+inline JSObject::JSObject(JSValue* proto)
+    : _proto(proto)
 {
     assert(proto);
 }
 
-inline JSObject::JSObject(bool destructorIsThreadSafe)
-    : JSCell(destructorIsThreadSafe)
-    , _proto(jsNull())
+inline JSObject::JSObject()
+    : _proto(jsNull())
 {
 }
 
index c64e8a493bc0986882eee88b63a0d77dac98f7a5..e879ae0a29887aad48e6a13f9527511070c19192 100644 (file)
@@ -33,25 +33,26 @@ using namespace KJS;
 ObjectPrototype::ObjectPrototype(ExecState* exec, FunctionPrototype* funcProto)
   : JSObject() // [[Prototype]] is null
 {
+    static const Identifier* hasOwnPropertyPropertyName = new Identifier("hasOwnProperty");
+    static const Identifier* propertyIsEnumerablePropertyName = new Identifier("propertyIsEnumerable");
+    static const Identifier* isPrototypeOfPropertyName = new Identifier("isPrototypeOf");
+    static const Identifier* defineGetterPropertyName = new Identifier("__defineGetter__");
+    static const Identifier* defineSetterPropertyName = new Identifier("__defineSetter__");
+    static const Identifier* lookupGetterPropertyName = new Identifier("__lookupGetter__");
+    static const Identifier* lookupSetterPropertyName = new Identifier("__lookupSetter__");
+
     putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::ToString, 0, toStringPropertyName), DontEnum);
     putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::ToLocaleString, 0, toLocaleStringPropertyName), DontEnum);
     putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::ValueOf, 0, valueOfPropertyName), DontEnum);
-    static Identifier hasOwnPropertyPropertyName("hasOwnProperty");
-    static Identifier propertyIsEnumerablePropertyName("propertyIsEnumerable");
-    static Identifier isPrototypeOfPropertyName("isPrototypeOf");
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::HasOwnProperty, 1, hasOwnPropertyPropertyName), DontEnum);
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::PropertyIsEnumerable, 1, propertyIsEnumerablePropertyName), DontEnum);
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::IsPrototypeOf, 1, isPrototypeOfPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::HasOwnProperty, 1, *hasOwnPropertyPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::PropertyIsEnumerable, 1, *propertyIsEnumerablePropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::IsPrototypeOf, 1, *isPrototypeOfPropertyName), DontEnum);
 
     // Mozilla extensions
-    static const Identifier defineGetterPropertyName("__defineGetter__");
-    static const Identifier defineSetterPropertyName("__defineSetter__");
-    static const Identifier lookupGetterPropertyName("__lookupGetter__");
-    static const Identifier lookupSetterPropertyName("__lookupSetter__");
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::DefineGetter, 2, defineGetterPropertyName), DontEnum);
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::DefineSetter, 2, defineSetterPropertyName), DontEnum);
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::LookupGetter, 1, lookupGetterPropertyName), DontEnum);
-    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::LookupSetter, 1, lookupSetterPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::DefineGetter, 2, *defineGetterPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::DefineSetter, 2, *defineSetterPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::LookupGetter, 1, *lookupGetterPropertyName), DontEnum);
+    putDirectFunction(new ObjectProtoFunc(exec, funcProto, ObjectProtoFunc::LookupSetter, 1, *lookupSetterPropertyName), DontEnum);
 }
 
 
index e55777ec136822d69db7016c6db249d0d226e446..23d3638224917de4a6e86e150fec1376c7ddbc1f 100644 (file)
@@ -48,12 +48,11 @@ RegExpPrototype::RegExpPrototype(ExecState *exec,
                                        FunctionPrototype *funcProto)
   : JSObject(objProto)
 {
-  // The constructor will be added later in RegExpObject's constructor (?)
+  static const Identifier* execPropertyName = new Identifier("exec");
+  static const Identifier* testPropertyName = new Identifier("test");
 
-  static const Identifier execPropertyName("exec");
-  static const Identifier testPropertyName("test");
-  putDirectFunction(new RegExpProtoFunc(exec, funcProto, RegExpProtoFunc::Exec, 0, execPropertyName), DontEnum);
-  putDirectFunction(new RegExpProtoFunc(exec, funcProto, RegExpProtoFunc::Test, 0, testPropertyName), DontEnum);
+  putDirectFunction(new RegExpProtoFunc(exec, funcProto, RegExpProtoFunc::Exec, 0, *execPropertyName), DontEnum);
+  putDirectFunction(new RegExpProtoFunc(exec, funcProto, RegExpProtoFunc::Test, 0, *testPropertyName), DontEnum);
   putDirectFunction(new RegExpProtoFunc(exec, funcProto, RegExpProtoFunc::ToString, 0, toStringPropertyName), DontEnum);
 }
 
index 3bce8285d560c244d364fc30c19789ba58d91bfa..cbceee3b66c3729fd0d004bff034f2de75e7dae9 100644 (file)
 #include "config.h"
 #include "ustring.h"
 
+#include "JSLock.h"
+#include "dtoa.h"
+#include "identifier.h"
+#include "operations.h"
 #include <assert.h>
-#include <stdlib.h>
-#include <stdio.h>
 #include <ctype.h>
+#include <float.h>
+#include <math.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <wtf/Vector.h>
+
 #if HAVE(STRING_H)
 #include <string.h>
 #endif
 #include <strings.h>
 #endif
 
-#include "dtoa.h"
-#include "identifier.h"
-#include "operations.h"
-#include <float.h>
-#include <math.h>
-#include <wtf/Vector.h>
-
 using std::max;
 using std::min;
 
@@ -154,6 +155,8 @@ UCharReference& UCharReference::operator=(UChar c)
 
 UChar& UCharReference::ref() const
 {
+  ASSERT(JSLock::lockCount() > 0);
+
   if (offset < str->rep()->len)
     return *(str->rep()->data() + offset);
   else {
@@ -164,6 +167,8 @@ UChar& UCharReference::ref() const
 
 PassRefPtr<UString::Rep> UString::Rep::createCopying(const UChar *d, int l)
 {
+  ASSERT(JSLock::lockCount() > 0);
+
   int sizeInBytes = l * sizeof(UChar);
   UChar *copyD = static_cast<UChar *>(fastMalloc(sizeInBytes));
   memcpy(copyD, d, sizeInBytes);
@@ -173,6 +178,8 @@ PassRefPtr<UString::Rep> UString::Rep::createCopying(const UChar *d, int l)
 
 PassRefPtr<UString::Rep> UString::Rep::create(UChar *d, int l)
 {
+  ASSERT(JSLock::lockCount() > 0);
+
   Rep *r = new Rep;
   r->offset = 0;
   r->len = l;
@@ -192,7 +199,8 @@ PassRefPtr<UString::Rep> UString::Rep::create(UChar *d, int l)
 
 PassRefPtr<UString::Rep> UString::Rep::create(PassRefPtr<Rep> base, int offset, int length)
 {
-  assert(base);
+  ASSERT(JSLock::lockCount() > 0);
+  ASSERT(base);
 
   int baseOffset = base->offset;
 
@@ -222,6 +230,8 @@ PassRefPtr<UString::Rep> UString::Rep::create(PassRefPtr<Rep> base, int offset,
 
 void UString::Rep::destroy()
 {
+  ASSERT(JSLock::lockCount() > 0);
+
   if (isIdentifier)
     Identifier::remove(this);
   if (baseString) {
@@ -466,10 +476,10 @@ UString::UString(const UString &a, const UString &b)
   }
 }
 
-const UString &UString::null()
+const UStringUString::null()
 {
-  static UString n;
-  return n;
+  static UString* n = new UString;
+  return *n;
 }
 
 UString UString::from(int i)
index 75c8fa756295cdb364d4e364ef53a8f510e5fe62..faddd8ad1a314117a2e108ae7c51b980a0aa0603 100644 (file)
 #ifndef _KJS_USTRING_H_
 #define _KJS_USTRING_H_
 
+#include "JSLock.h"
+#include <stdint.h>
+#include <wtf/Assertions.h>
 #include <wtf/FastMalloc.h>
-#include <wtf/RefPtr.h>
 #include <wtf/PassRefPtr.h>
-
-#include <stdint.h>
+#include <wtf/RefPtr.h>
 
 /* On ARM some versions of GCC don't pack structures by default so sizeof(UChar)
    will end up being != 2 which causes crashes since the code depends on that. */
@@ -199,8 +200,8 @@ namespace KJS {
       static unsigned computeHash(const UChar *, int length);
       static unsigned computeHash(const char *);
 
-      Rep* ref() { ++rc; return this; }
-      void deref() { if (--rc == 0) destroy(); }
+      Rep* ref() { ASSERT(JSLock::lockCount() > 0); ++rc; return this; }
+      void deref() { ASSERT(JSLock::lockCount() > 0); if (--rc == 0) destroy(); }
 
       // unshared data
       int offset;
index 0c8a95e01be0c997df0ed84c58b7b2c6ec95e965..8b1d4160efc3cef1f919901cd6e910c74f436bcd 100644 (file)
@@ -122,7 +122,7 @@ class JSCell : public JSValue {
     friend class JSObject;
     friend class GetterSetterImp;
 private:
-    explicit JSCell(bool destructorIsThreadSafe = true);
+    JSCell();
     virtual ~JSCell();
 public:
     // Querying the type.
@@ -156,7 +156,7 @@ public:
     bool marked() const;
 
 private:
-    bool m_destructorIsThreadSafe : 1;
+    bool m_collectOnMainThreadOnly : 1;
     bool m_marked : 1;
 };
 
@@ -203,8 +203,8 @@ inline JSValue::~JSValue()
 {
 }
 
-inline JSCell::JSCell(bool destructorIsThreadSafe)
-    : m_destructorIsThreadSafe(destructorIsThreadSafe)
+inline JSCell::JSCell()
+    : m_collectOnMainThreadOnly(false)
     , m_marked(false)
 {
 }
index bd6d06ffc3b3c4c7854e1682ecf1923a4c556916..202251a41f1d42cd441a74429c484da6a6ab2c05 100644 (file)
@@ -1,3 +1,25 @@
+2007-03-06  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Fixed all known crashers exposed by run-webkit-tests --threaded while using
+        a PAC file (for maximum carnage). See JavaScriptCore ChangeLog for 
+        more details.
+
+        * JSBase.cpp:
+        (JSBase::Release): Lock when deleting, because we may be deleting an object
+        (like a JSRun) that holds thread-unsafe data.
+
+        * JSUtils.cpp:
+        (CFStringToUString): Don't lock, because our caller locks. Also, locking
+        inside a function that returns thread-unsafe data by copy will only mask
+        threading problems.
+
+        * JavaScriptGlue.cpp:
+        (JSRunEvaluate): Added missing JSLock.
+        (JSRunCheckSyntax): Converted to JSLock.
+        * JavaScriptGlue.xcodeproj/project.pbxproj:
+
 2007-02-22  Geoffrey Garen  <ggaren@apple.com>
 
         Reviewed by Darin Adler.
index b899e6f8c34c591da43994ff400f75b81beb9d1f..0a622c0025816aa2682adcb8564eeecc5e04da66 100644 (file)
@@ -46,6 +46,7 @@ void JSBase::Release()
 {
     if (--fRetainCount == 0)
     {
+        JSLock lock;
         delete this;
     }
 }
index 842686eba77cd7e2412f9ef89baeffffcafbd355..53af5055e63e6761ad98462522500bb9b2307852 100644 (file)
@@ -51,8 +51,6 @@ static CFTypeRef KJSValueToCFTypeInternal(JSValue *inValue, ExecState *exec, Obj
 
 UString CFStringToUString(CFStringRef inCFString)
 {
-    JSLock lock;
-
     UString result;
     if (inCFString) {
         CFIndex len = CFStringGetLength(inCFString);
index ba4177fcee45646ca58f5473bb475f7d2dbabc20..e97dff6f76def332e08a6edece3c226c2659f6d7 100644 (file)
@@ -287,6 +287,7 @@ JSObjectRef JSRunEvaluate(JSRunRef ref)
     JSRun* ptr = (JSRun*)ref;
     if (ptr)
     {
+        JSLock lock;
         Completion completion = ptr->Evaluate();
         if (completion.isValueCompletion())
         {
@@ -320,9 +321,8 @@ bool JSRunCheckSyntax(JSRunRef ref)
     JSRun* ptr = (JSRun*)ref;
     if (ptr)
     {
-            JSLockInterpreter();
+            JSLock lock;
             result = ptr->CheckSyntax();
-            JSUnlockInterpreter();
     }
     return result;
 }
index 7e4239709c78aa9f9af8b5f6e3da0b2e3ef5953a..c70ee83ad5aea4d9f8227150f29e6b42f602bc06 100644 (file)
                0867D690FE84028FC02AAC07 /* Project object */ = {
                        isa = PBXProject;
                        buildConfigurationList = 14AC662B08CE7791006915A8 /* Build configuration list for PBXProject "JavaScriptGlue" */;
-                       compatibilityVersion = "Xcode 2.4";
                        hasScannedForEncodings = 1;
                        mainGroup = 0867D691FE84028FC02AAC07 /* JavaScriptGlue */;
                        productRefGroup = 034768DFFF38A50411DB9C8B /* Products */;
                        projectDirPath = "";
                        projectRoot = "";
-                       shouldCheckCompatibility = 1;
                        targets = (
                                DD66F3B908F73ED700C75FD7 /* JavaScriptGlue */,
                                1422E87609DE3BE800749B87 /* testjsglue */,
index 42365fadf57a3b879eb0bb7592497e5983e6589b..4996704f9fd61e9c09baffb1bb14f3edc3ab603a 100644 (file)
@@ -1,3 +1,48 @@
+2007-03-06  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Fixed all known crashers exposed by run-webkit-tests --threaded [*]. See 
+        JavaScriptCore ChangeLog for more details.
+
+        * bindings/js/kjs_binding.cpp:
+        (KJS::domNodesPerDocument): Added thread safety ASSERT.
+        (KJS::ScriptInterpreter::mark): Removed obsolete logic for marking unsafe
+        objects when collecting on a secondary thread. The Collector takes care
+        of this now.
+
+        * bindings/js/kjs_binding.h:
+        (KJS::DOMObject::DOMObject): Used new API for specifying that WebCore
+        objects should be garbage collected on the main thread only.
+
+        * bindings/js/kjs_window.cpp:
+        (KJS::ScheduledAction::execute): Moved JSLock to cover implementedsCall() call,
+        which, for some subclasses, ends up allocating garbage collected objects.
+        (This fix was speculative. I didn't actually see a crash from this.)
+        (KJS::Window::timerFired): Added JSLock around ScheduleAction destruction,
+        since it destroys a KJS::List.
+
+        * bindings/objc/WebScriptObject.mm:
+        (-[WebScriptObject setException:]): Added JSLock. (This fix was speculative. 
+        I didn't actually see a crash from this.)
+
+        * bridge/mac/WebCoreScriptDebugger.mm:
+        (-[WebCoreScriptCallFrame evaluateWebScript:]): Added JSLock. (This fix 
+        was speculative. I didn't actually see a crash from this.)
+
+        * dom/Document.cpp:
+        (WebCore::Document::~Document): Added JSLock around modification to 
+        domNodesPerDocument(), which can be accessed concurrently during garbage 
+        collection.
+        * dom/Node.cpp:
+        (WebCore::Node::setDocument): ditto.
+        
+        [*] fast/js/toString-stack-overflow.html is an exception. --threaded mode
+        crashes this test because it causes the garbage collector to run frequently,
+        and this test crashes if you happen to garbage collect while it's running.
+        This is a known issue with stack overflow during the mark phase. It's
+        not related to threading.
+
 2007-03-06  Mark Rowe  <mrowe@apple.com>
 
         Reviewed by Sam Weinig.
index 34601d2f888ce8701b6a9f990dc1cafbf6a7e473..bfc540aaa10e4dced42e43b919cd0d74a4d44916 100644 (file)
@@ -119,6 +119,11 @@ static DOMObjectMap& domObjects()
 
 static NodePerDocMap& domNodesPerDocument()
 {
+    // domNodesPerDocument() callers must synchronize using the JSLock because 
+    // domNodesPerDocument() is called during garbage collection, which can happen
+    // on a secondary thread.
+    ASSERT(JSLock::lockCount());
+
     static NodePerDocMap staticDOMNodesPerDocument;
     return staticDOMNodesPerDocument;
 }
@@ -216,22 +221,6 @@ void ScriptInterpreter::markDOMNodesForDocument(Document* doc)
     }
 }
 
-void ScriptInterpreter::mark(bool currentThreadIsMainThread)
-{
-    if (!currentThreadIsMainThread) {
-        // On alternate threads, DOMObjects remain in the cache because they're not collected.
-        // So, they need an opportunity to mark their children.
-        DOMObjectMap::iterator objectEnd = domObjects().end();
-        for (DOMObjectMap::iterator objectIt = domObjects().begin(); objectIt != objectEnd; ++objectIt) {
-            DOMObject* object = objectIt->second;
-            if (!object->marked())
-                object->mark();
-        }
-    }
-
-    Interpreter::mark(currentThreadIsMainThread);
-}
-
 ExecState* ScriptInterpreter::globalExec()
 {
     // we need to make sure that any script execution happening in this
index 842e39e7ebf7173725ef9e32d343eed1ae6401e2..659441d2d1507ad37e945f5681ea1cc253a07bfa 100644 (file)
@@ -46,9 +46,12 @@ namespace KJS {
      */
     class DOMObject : public JSObject {
     protected:
-        // DOMObject Destruction is not thread-safe because JS DOM objects 
-        // wrap unsafe WebCore DOM data structures
-        DOMObject() : JSObject(false) {}
+        DOMObject()
+        {
+            // DOMObject destruction is not thread-safe because DOMObjects wrap 
+            // unsafe WebCore DOM data structures.
+            Collector::collectOnMainThreadOnly(this);
+        }
 #ifndef NDEBUG
         virtual ~DOMObject();
 #endif
@@ -92,7 +95,6 @@ namespace KJS {
          */
         bool wasRunByUserGesture() const;
 
-        virtual void mark(bool currentThreadIsMainThread);
         virtual ExecState* globalExec();
 
         WebCore::Event* getCurrentEvent() const { return m_currentEvent; }
index 5720e0b1ad9614920d076f11548700fe17e47a02..6860545ca3d7407765e342238f43a9698fc4b8f5 100644 (file)
@@ -83,7 +83,11 @@ class DOMWindowTimer : public TimerBase {
 public:
     DOMWindowTimer(int timeoutId, int nestingLevel, Window* o, ScheduledAction* a)
         : m_timeoutId(timeoutId), m_nestingLevel(nestingLevel), m_object(o), m_action(a) { }
-    virtual ~DOMWindowTimer() { delete m_action; }
+    virtual ~DOMWindowTimer() 
+    { 
+        JSLock lock;
+        delete m_action; 
+    }
 
     int timeoutId() const { return m_timeoutId; }
     
@@ -1848,10 +1852,10 @@ void ScheduledAction::execute(Window* window)
     interpreter->setProcessingTimerCallback(true);
 
     if (JSValue* func = m_func.get()) {
+        JSLock lock;
         if (func->isObject() && static_cast<JSObject*>(func)->implementsCall()) {
             ExecState* exec = interpreter->globalExec();
             ASSERT(window == interpreter->globalObject());
-            JSLock lock;
             interpreter->startTimeoutCheck();
             static_cast<JSObject*>(func)->call(exec, window, m_args);
             interpreter->stopTimeoutCheck();
@@ -1990,6 +1994,8 @@ void Window::timerFired(DOMWindowTimer* timer)
     m_timeouts.remove(timer->timeoutId());
     delete timer;
     action->execute(this);
+    
+    JSLock lock;
     delete action;
 }
 
index 31465d8754bcf8fffa3a5771c730bcef6cec2424..4c970d0f7c0e2c9e8c83fe0c65a36d9d661bbd52 100644 (file)
@@ -417,6 +417,8 @@ static List listFromNSArray(ExecState *exec, NSArray *array)
     if (![self _rootObject])
         return;
 
+    JSLock lock;
+    
     if ([self _rootObject]->interpreter()->context()) {
         ExecState *exec = [self _rootObject]->interpreter()->context()->execState();
 
index 601d42afb312995280fb07aa454e9bf5922141c8..f41c7eee5aaf016502a92beff205bc8b2a7ac439 100644 (file)
@@ -336,6 +336,8 @@ class WebCoreScriptDebuggerImp : public KJS::Debugger {
 
 - (id)evaluateWebScript:(NSString *)script
 {
+    JSLock lock;
+
     UString code = String(script);
 
     ExecState   *state   = _state;
@@ -359,7 +361,6 @@ class WebCoreScriptDebuggerImp : public KJS::Debugger {
     // evaluate
     JSValue *result;
     if (eval) {
-        JSLock lock;
         List args;
         args.append(jsString(code));
         result = eval->call(state, NULL, args);
index d1bc187c3d250bb84f6e89814d0b2ce0c01ce5af..faf510069c9687f5cbae19e3385e3a269968cf10 100644 (file)
@@ -392,7 +392,10 @@ Document::~Document()
 #endif
 
     XMLHttpRequest::detachRequests(this);
-    KJS::ScriptInterpreter::forgetAllDOMNodesForDocument(this);
+    {
+        KJS::JSLock lock;
+        KJS::ScriptInterpreter::forgetAllDOMNodesForDocument(this);
+    }
 
     if (m_docChanged && changedDocuments)
         changedDocuments->remove(this);
index 83e41e32ecbc65ecc5da6922956f8ddcb0d3cee2..d1f919c07cece56c3f840c15ccd4da31c97e82b9 100644 (file)
@@ -169,7 +169,10 @@ void Node::setDocument(Document* doc)
     if (inDocument() || m_document == doc)
         return;
 
-    KJS::ScriptInterpreter::updateDOMNodeDocument(this, m_document.get(), doc);
+    {
+        KJS::JSLock lock;
+        KJS::ScriptInterpreter::updateDOMNodeDocument(this, m_document.get(), doc);
+    }
     m_document = doc;
 }