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)
commit600db3fd5854f20e8639e53e9e5061f200a08362
tree30f03019737d952a2e1ef64d17918d6ece0c0721
parentf782ff5bb2e7ed690058467f264b93e85e02f7a4
JavaScriptCore:

        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