Changeset 85233 in webkit


Ignore:
Timestamp:
Apr 28, 2011 1:46:22 PM (13 years ago)
Author:
levin@chromium.org
Message:

Add asserts to RefCounted to make sure ref/deref happens on the right thread.
https://bugs.webkit.org/show_bug.cgi?id=31639

Reviewed by Darin Adler.

Source/JavaScriptCore:

(JSC::ExecutablePool::ExecutablePool): Turned off checks for this
due to not being able to figure out what was guarding it (bug 58091).

  • parser/SourceProvider.h:

(JSC::SourceProvider::SourceProvider): Ditto.

  • runtime/RegExp.cpp:

(JSC::RegExp::RegExp): Ditto.

  • wtf/CMakeLists.txt: Added new files to the build.
  • wtf/ThreadRestrictionVerifier.h: Added.

Everything is done in the header to avoid the issue with exports
that are only useful in debug but still needing to export them.

  • wtf/RefCounted.h:

(WTF::RefCountedBase::ref): Added checks using the non thread safe verifier.
and filed bug 58171 about making it stricter.
(WTF::RefCountedBase::hasOneRef): Ditto.
(WTF::RefCountedBase::refCount): Ditto.
(WTF::RefCountedBase::setMutexForVerifier): Expose a way to change the checks to be based
on a mutex. This is in the header to avoid adding more exports from JavaScriptCore.
(WTF::RefCountedBase::deprecatedTurnOffVerifier): Temporary way to turn off verification.
Filed bug 58174 to remove this method.
(WTF::RefCountedBase::derefBase):

  • wtf/SizeLimits.cpp: Adjusted the debug size check for RefCounted.
  • wtf/text/CString.h:

(WTF::CStringBuffer::CStringBuffer): Turned off checks for this while a fix is being
done in Chromium's test_shell (bug 58093).

Source/JavaScriptGlue:

  • ForwardingHeaders/wtf/ThreadRestrictionVerifier.h: Added.

Source/WebCore:

No new functionality exposed so no new tests. (The change is basically adding
more testing.)

  • ForwardingHeaders/wtf/ThreadRestrictionVerifier.h: Added.
  • loader/icon/IconDatabase.cpp:

(WebCore::IconDatabase::defaultIcon): Set the mutex which does the guarding of the variable.
(WebCore::IconDatabase::getOrCreateIconRecord): Ditto.
(WebCore::IconDatabase::setIconDataForIconURL): Ditto.
(WebCore::IconDatabase::readFromDatabase): Ditto.

Location:
trunk/Source
Files:
3 added
15 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r85228 r85233  
     12011-04-07  David Levin  <levin@chromium.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        Add asserts to RefCounted to make sure ref/deref happens on the right thread.
     6        https://bugs.webkit.org/show_bug.cgi?id=31639
     7
     8        * GNUmakefile.list.am: Added new files to the build.
     9        * JavaScriptCore.gypi: Ditto.
     10        * JavaScriptCore.vcproj/WTF/WTF.vcproj: Ditto.
     11        * JavaScriptCore.xcodeproj/project.pbxproj: Ditto.
     12        * jit/ExecutableAllocator.h:
     13        (JSC::ExecutablePool::ExecutablePool): Turned off checks for this
     14        due to not being able to figure out what was guarding it (bug 58091).
     15        * parser/SourceProvider.h:
     16        (JSC::SourceProvider::SourceProvider): Ditto.
     17        * runtime/RegExp.cpp:
     18        (JSC::RegExp::RegExp): Ditto.
     19        * wtf/CMakeLists.txt: Added new files to the build.
     20        * wtf/ThreadRestrictionVerifier.h: Added.
     21        Everything is done in the header to avoid the issue with exports
     22        that are only useful in debug but still needing to export them.
     23        * wtf/RefCounted.h:
     24        (WTF::RefCountedBase::ref): Added checks using the non thread safe verifier.
     25        and filed bug 58171 about making it stricter.
     26        (WTF::RefCountedBase::hasOneRef): Ditto.
     27        (WTF::RefCountedBase::refCount): Ditto.
     28        (WTF::RefCountedBase::setMutexForVerifier): Expose a way to change the checks to be based
     29        on a mutex. This is in the header to avoid adding more exports from JavaScriptCore.
     30        (WTF::RefCountedBase::deprecatedTurnOffVerifier): Temporary way to turn off verification.
     31        Filed bug 58174 to remove this method.
     32        (WTF::RefCountedBase::derefBase):
     33        * wtf/SizeLimits.cpp: Adjusted the debug size check for RefCounted.
     34        * wtf/text/CString.h:
     35        (WTF::CStringBuffer::CStringBuffer): Turned off checks for this while a fix is being
     36        done in Chromium's test_shell (bug 58093).
     37
    1382011-04-28  Xan Lopez  <xlopez@igalia.com>
    239
  • trunk/Source/JavaScriptCore/GNUmakefile.list.am

    r85224 r85233  
    531531        Source/JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp \
    532532        Source/JavaScriptCore/wtf/ThreadIdentifierDataPthreads.h \
     533        Source/JavaScriptCore/wtf/ThreadRestrictionVerifier.h \
    533534        Source/JavaScriptCore/wtf/Threading.cpp \
    534535        Source/JavaScriptCore/wtf/Threading.h \
  • trunk/Source/JavaScriptCore/JavaScriptCore.gypi

    r84290 r85233  
    204204            'wtf/ThreadSafeRefCounted.h',
    205205            'wtf/ThreadSpecific.h',
     206            'wtf/ThreadRestrictionVerifier.h',
    206207            'wtf/Threading.h',
    207208            'wtf/ThreadingPrimitives.h',
  • trunk/Source/JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj

    r84911 r85233  
    10021002                </File>
    10031003                <File
     1004                        RelativePath="..\..\wtf\ThreadRestrictionVerifier.h"
     1005                        >
     1006                </File>
     1007                <File
    10041008                        RelativePath="..\..\wtf\ThreadSafeRefCounted.h"
    10051009                        >
  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r85189 r85233  
    4545                0B330C270F38C62300692DE3 /* TypeTraits.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0B330C260F38C62300692DE3 /* TypeTraits.cpp */; };
    4646                0B4D7E630F319AC800AD7E58 /* TypeTraits.h in Headers */ = {isa = PBXBuildFile; fileRef = 0B4D7E620F319AC800AD7E58 /* TypeTraits.h */; settings = {ATTRIBUTES = (Private, ); }; };
     47                0BAC94A01338728400CF135B /* ThreadRestrictionVerifier.h in Headers */ = {isa = PBXBuildFile; fileRef = 0BAC949E1338728400CF135B /* ThreadRestrictionVerifier.h */; settings = {ATTRIBUTES = (Private, ); }; };
    4748                0BDFFAE00FC6192900D69EF4 /* CrossThreadRefCounted.h in Headers */ = {isa = PBXBuildFile; fileRef = 0BDFFAD40FC6171000D69EF4 /* CrossThreadRefCounted.h */; settings = {ATTRIBUTES = (Private, ); }; };
    4849                0BDFFAE10FC6193100D69EF4 /* OwnFastMallocPtr.h in Headers */ = {isa = PBXBuildFile; fileRef = 0BDFFAD10FC616EC00D69EF4 /* OwnFastMallocPtr.h */; settings = {ATTRIBUTES = (Private, ); }; };
     
    706707                0B330C260F38C62300692DE3 /* TypeTraits.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = TypeTraits.cpp; sourceTree = "<group>"; };
    707708                0B4D7E620F319AC800AD7E58 /* TypeTraits.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TypeTraits.h; sourceTree = "<group>"; };
     709                0BAC949E1338728400CF135B /* ThreadRestrictionVerifier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ThreadRestrictionVerifier.h; sourceTree = "<group>"; };
    708710                0BDFFAD10FC616EC00D69EF4 /* OwnFastMallocPtr.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OwnFastMallocPtr.h; sourceTree = "<group>"; };
    709711                0BDFFAD40FC6171000D69EF4 /* CrossThreadRefCounted.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CrossThreadRefCounted.h; sourceTree = "<group>"; };
     
    16251627                                A1D764511354448B00C5C7C0 /* Alignment.h */,
    16261628                                A7C40C07130B057D00D002A1 /* BlockStack.h */,
    1627                                 A7C40C08130B057D00D002A1 /* SentinelLinkedList.h */,
    1628                                 A7C40C09130B057D00D002A1 /* SinglyLinkedList.h */,
    16291629                                5135FAD512D26856003C083B /* Decoder.h */,
    16301630                                5135FAD612D26856003C083B /* Encoder.h */,
     
    17241724                                76FB9F1012E851960051A2EB /* SHA1.cpp */,
    17251725                                76FB9F0E12E851860051A2EB /* SHA1.h */,
     1726                                A7C40C08130B057D00D002A1 /* SentinelLinkedList.h */,
     1727                                A7C40C09130B057D00D002A1 /* SinglyLinkedList.h */,
    17261728                                0BF28A2811A33DC300638F84 /* SizeLimits.cpp */,
    17271729                                86D87DA512BC4B14008E73A1 /* StackBounds.cpp */,
     
    17451747                                BC5F7BBD11823B590052C02C /* ThreadSafeRefCounted.h */,
    17461748                                E1B7C8BD0DA3A3360074B0DC /* ThreadSpecific.h */,
     1749                                0BAC949E1338728400CF135B /* ThreadRestrictionVerifier.h */,
    17471750                                0B330C260F38C62300692DE3 /* TypeTraits.cpp */,
    17481751                                0B4D7E620F319AC800AD7E58 /* TypeTraits.h */,
     
    25282531                                BC18C4700E16F5CD00B34460 /* Threading.h in Headers */,
    25292532                                BC5F7BBF11823B590052C02C /* ThreadingPrimitives.h in Headers */,
     2533                                0BAC94A01338728400CF135B /* ThreadRestrictionVerifier.h in Headers */,
    25302534                                BC5F7BC011823B590052C02C /* ThreadSafeRefCounted.h in Headers */,
    25312535                                BC18C4710E16F5CD00B34460 /* ThreadSpecific.h in Headers */,
  • trunk/Source/JavaScriptCore/jit/ExecutableAllocator.h

    r79773 r85233  
    351351        CRASH(); // Failed to allocate
    352352    m_end = m_freePtr + allocSize;
     353    deprecatedTurnOffVerifier();
    353354}
    354355
  • trunk/Source/JavaScriptCore/parser/SourceProvider.h

    r76611 r85233  
    4747            , m_cacheOwned(!cache)
    4848        {
     49            deprecatedTurnOffVerifier();
    4950        }
    5051        virtual ~SourceProvider()
  • trunk/Source/JavaScriptCore/runtime/RegExp.cpp

    r80667 r85233  
    8686{
    8787    m_state = compile(globalData);
     88    deprecatedTurnOffVerifier();
    8889}
    8990
  • trunk/Source/JavaScriptCore/wtf/CMakeLists.txt

    r84911 r85233  
    4343    MessageQueue.h
    4444    NonCopyingSort.h
     45    ThreadRestrictionVerifier.h
    4546    Noncopyable.h
    4647    NotFound.h
  • trunk/Source/JavaScriptCore/wtf/RefCounted.h

    r80969 r85233  
    2424#include "Assertions.h"
    2525#include "FastAllocBase.h"
     26#include "ThreadRestrictionVerifier.h"
    2627#include "Noncopyable.h"
     28#include "OwnPtr.h"
     29#include "UnusedParam.h"
    2730
    2831namespace WTF {
     
    3538    void ref()
    3639    {
     40#ifndef NDEBUG
     41        // Start thread verification as soon as the ref count gets to 2. This
     42        // heuristic reflects the fact that items are often created on one thread
     43        // and then given to another thread to be used.
     44        // FIXME: Make this restriction tigher. Especially as we move to more
     45        // common methods for sharing items across threads like CrossThreadCopier.h
     46        // We should be able to add a "detachFromThread" method to make this explicit.
     47        if (m_refCount == 1)
     48            m_verifier.setShared(true);
     49#endif
     50        // If this assert fires, it either indicates a thread safety issue or
     51        // that the verification needs to change. See ThreadRestrictionVerifier for
     52        // the different modes.
     53        ASSERT(m_verifier.isSafeToUse());
    3754        ASSERT(!m_deletionHasBegun);
    3855        ASSERT(!m_adoptionIsRequired);
     
    4259    bool hasOneRef() const
    4360    {
     61        ASSERT(m_verifier.isSafeToUse());
    4462        ASSERT(!m_deletionHasBegun);
    4563        return m_refCount == 1;
     
    4866    int refCount() const
    4967    {
     68        ASSERT(m_verifier.isSafeToUse());
    5069        return m_refCount;
     70    }
     71
     72    void setMutexForVerifier(Mutex&);
     73
     74    // Turns off verification. Use of this method is discouraged (instead extend
     75    // ThreadRestrictionVerifier to verify your case).
     76    // FIXME: remove this method.
     77    void deprecatedTurnOffVerifier()
     78    {
     79#ifndef NDEBUG
     80        m_verifier.turnOffVerification();
     81#endif
    5182    }
    5283
     
    85116    bool derefBase()
    86117    {
     118        ASSERT(m_verifier.isSafeToUse());
    87119        ASSERT(!m_deletionHasBegun);
    88120        ASSERT(!m_adoptionIsRequired);
     
    97129
    98130        --m_refCount;
     131#ifndef NDEBUG
     132        // Stop thread verification when the ref goes to 1 because it
     133        // is safe to be passed to another thread at this point.
     134        if (m_refCount == 1)
     135            m_verifier.setShared(false);
     136#endif
    99137        return false;
    100138    }
     
    118156    bool m_deletionHasBegun;
    119157    bool m_adoptionIsRequired;
     158    ThreadRestrictionVerifier m_verifier;
    120159#endif
    121160};
     
    165204};
    166205
     206#ifndef NDEBUG
     207inline void RefCountedBase::setMutexForVerifier(Mutex&) { }
     208#else
     209inline void RefCountedBase::setMutexForVerifier(Mutex& mutex)
     210{
     211    m_verifier.setMutexMode(mutex);
     212}
     213#endif
     214
    167215} // namespace WTF
    168216
  • trunk/Source/JavaScriptCore/wtf/SizeLimits.cpp

    r62734 r85233  
    3737#include <wtf/RefCounted.h>
    3838#include <wtf/RefPtr.h>
     39#include <wtf/ThreadRestrictionVerifier.h>
    3940#include <wtf/Vector.h>
    4041
     
    4243
    4344#ifndef NDEBUG
    44 struct StructWithIntAndTwoBools { int a; bool b; bool c; };
    45 static const size_t refCountedExtraDebugSize = sizeof(StructWithIntAndTwoBools) - sizeof(int);
     45struct SameSizeAsRefCounted {
     46    int a;
     47    bool b;
     48    bool c;
     49    ThreadRestrictionVerifier d;
     50    // The debug version may get bigger.
     51};
    4652#else
    47 static const size_t refCountedExtraDebugSize = 0;
     53struct SameSizeAsRefCounted {
     54    int a;
     55    // Don't add anything here because this should stay small.
     56};
    4857#endif
    4958
    5059COMPILE_ASSERT(sizeof(OwnPtr<int>) == sizeof(int*), OwnPtr_should_stay_small);
    5160COMPILE_ASSERT(sizeof(PassRefPtr<RefCounted<int> >) == sizeof(int*), PassRefPtr_should_stay_small);
    52 COMPILE_ASSERT(sizeof(RefCounted<int>) == sizeof(int) + refCountedExtraDebugSize, RefCounted_should_stay_small);
    53 COMPILE_ASSERT(sizeof(RefCountedCustomAllocated<int>) == sizeof(int) + refCountedExtraDebugSize, RefCountedCustomAllocated_should_stay_small);
     61COMPILE_ASSERT(sizeof(RefCounted<int>) == sizeof(SameSizeAsRefCounted), RefCounted_should_stay_small);
     62COMPILE_ASSERT(sizeof(RefCountedCustomAllocated<int>) == sizeof(SameSizeAsRefCounted), RefCountedCustomAllocated_should_stay_small);
    5463COMPILE_ASSERT(sizeof(RefPtr<RefCounted<int> >) == sizeof(int*), RefPtr_should_stay_small);
    5564COMPILE_ASSERT(sizeof(Vector<int>) == 3 * sizeof(int*), Vector_should_stay_small);
  • trunk/Source/JavaScriptCore/wtf/text/CString.h

    r69277 r85233  
    4242
    4343    static PassRefPtr<CStringBuffer> create(size_t length) { return adoptRef(new CStringBuffer(length)); }
    44     CStringBuffer(size_t length) : m_vector(length) { }
     44    CStringBuffer(size_t length) : m_vector(length) { deprecatedTurnOffVerifier(); }
    4545    char* mutableData() { return m_vector.data(); }
    4646
  • trunk/Source/JavaScriptGlue/ChangeLog

    r85003 r85233  
     12011-04-04  David Levin  <levin@chromium.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        Add asserts to RefCounted to make sure ref/deref happens on the right thread.
     6        https://bugs.webkit.org/show_bug.cgi?id=31639
     7
     8        * ForwardingHeaders/wtf/ThreadRestrictionVerifier.h: Added.
     9
    1102011-04-26  Dan Bernstein  <mitz@apple.com>
    211
  • trunk/Source/WebCore/ChangeLog

    r85220 r85233  
     12011-04-07  David Levin  <levin@chromium.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        Add asserts to RefCounted to make sure ref/deref happens on the right thread.
     6        https://bugs.webkit.org/show_bug.cgi?id=31639
     7
     8        No new functionality exposed so no new tests. (The change is basically adding
     9        more testing.)
     10
     11        * ForwardingHeaders/wtf/ThreadRestrictionVerifier.h: Added.
     12        * loader/icon/IconDatabase.cpp:
     13        (WebCore::IconDatabase::defaultIcon): Set the mutex which does the guarding of the variable.
     14        (WebCore::IconDatabase::getOrCreateIconRecord): Ditto.
     15        (WebCore::IconDatabase::setIconDataForIconURL): Ditto.
     16        (WebCore::IconDatabase::readFromDatabase): Ditto.
     17
    1182011-04-28  Kenneth Russell  <kbr@google.com>
    219
  • trunk/Source/WebCore/loader/icon/IconDatabase.cpp

    r85056 r85233  
    388388    if (!m_defaultIconRecord) {
    389389        m_defaultIconRecord = IconRecord::create("urlIcon");
     390        m_defaultIconRecord->setMutexForVerifier(m_urlAndIconLock);
    390391        loadDefaultIconRecord(m_defaultIconRecord.get());
    391392    }
     
    518519   
    519520    RefPtr<SharedBuffer> data = dataOriginal ? dataOriginal->copy() : PassRefPtr<SharedBuffer>(0);
     521    if (data)
     522        data->setMutexForVerifier(m_urlAndIconLock);
    520523    String iconURL = iconURLOriginal.crossThreadString();
    521524   
     
    879882
    880883    RefPtr<IconRecord> newIcon = IconRecord::create(iconURL);
     884    newIcon->setMutexForVerifier(m_urlAndIconLock);
    881885    m_iconURLToRecordMap.set(iconURL, newIcon.get());
    882886
     
    14801484        didAnyWork = true;
    14811485        RefPtr<SharedBuffer> imageData = getImageDataForIconURLFromSQLDatabase(icons[i]->iconURL());
     1486        imageData->setMutexForVerifier(m_urlAndIconLock);
    14821487
    14831488        // Verify this icon still wants to be read from disk
Note: See TracChangeset for help on using the changeset viewer.