Changeset 55256 in webkit


Ignore:
Timestamp:
Feb 25, 2010 2:15:26 PM (14 years ago)
Author:
oliver@apple.com
Message:

2010-02-25 Oliver Hunt <oliver@apple.com>

Reviewed by Maciej Stachowiak.

Race condition in JSPropertyNameIterator and Structure destruction
https://bugs.webkit.org/show_bug.cgi?id=35398

JSPropertyNameIterator and Structure have a cyclic dependency that they
manage by clearing the appropriate reference in each other during their
destruction. However if the Structure is destroyed while the
JSPropertyNameIterator is dead but not yet finalized the Structures
WeakGCPtr will return null, and so prevent Structure from clearing
the m_cachedStructure pointer of the iterator. When the iterator is
then finalised the m_cachedStructure is invalid, and the attempt to
clear the structures back reference fails.

To fix this we simply make JSPropertyNameIterator keep the Structure
alive, using the weak pointer to break the ref cycle.

  • runtime/JSPropertyNameIterator.cpp: (JSC::JSPropertyNameIterator::~JSPropertyNameIterator): The iterator now keeps m_cachedStructure alive itself, so no longer needs to check for it being cleared
  • runtime/JSPropertyNameIterator.h: (JSC::JSPropertyNameIterator::setCachedStructure): Add an assertion to ensure correct usage (JSC::JSPropertyNameIterator::cachedStructure): Add .get()
  • runtime/Structure.cpp: (JSC::Structure::~Structure): Add an assertion that our iterator isn't already dead, and remove the now unnecessary attempt to clear the ref in the iterator
  • runtime/WeakGCPtr.h: (JSC::WeakGCPtr::hasDeadObject): An assert-only function to allow us to assert correct behaviour in the Structure destructor

2010-02-25 Oliver Hunt <oliver@apple.com>

Reviewed by Maciej Stachowiak.

Race condition in JSPropertyNameIterator and Structure destruction
https://bugs.webkit.org/show_bug.cgi?id=35398

Add test to ensure that this race condition doesn't occur.

  • fast/js/script-tests/for-in-cached.js: (cacheClearing):
Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/JavaScriptCore/ChangeLog

    r55247 r55256  
     12010-02-25  Oliver Hunt  <oliver@apple.com>
     2
     3        Reviewed by Maciej Stachowiak.
     4
     5        Race condition in JSPropertyNameIterator and Structure destruction
     6        https://bugs.webkit.org/show_bug.cgi?id=35398
     7
     8        JSPropertyNameIterator and Structure have a cyclic dependency that they
     9        manage by clearing the appropriate reference in each other during their
     10        destruction.  However if the Structure is destroyed while the
     11        JSPropertyNameIterator is dead but not yet finalized the Structures
     12        WeakGCPtr will return null, and so prevent Structure from clearing
     13        the m_cachedStructure pointer of the iterator.  When the iterator is
     14        then finalised the m_cachedStructure is invalid, and the attempt to
     15        clear the structures back reference fails.
     16
     17        To fix this we simply make JSPropertyNameIterator keep the Structure
     18        alive, using the weak pointer to break the ref cycle.
     19
     20        * runtime/JSPropertyNameIterator.cpp:
     21        (JSC::JSPropertyNameIterator::~JSPropertyNameIterator):
     22          The iterator now keeps m_cachedStructure alive itself, so no longer needs
     23          to check for it being cleared
     24        * runtime/JSPropertyNameIterator.h:
     25        (JSC::JSPropertyNameIterator::setCachedStructure):
     26          Add an assertion to ensure correct usage
     27        (JSC::JSPropertyNameIterator::cachedStructure):
     28          Add .get()
     29        * runtime/Structure.cpp:
     30        (JSC::Structure::~Structure):
     31          Add an assertion that our iterator isn't already dead, and remove
     32          the now unnecessary attempt to clear the ref in the iterator
     33        * runtime/WeakGCPtr.h:
     34        (JSC::WeakGCPtr::hasDeadObject):
     35          An assert-only function to allow us to assert correct behaviour
     36          in the Structure destructor
     37
    1382010-02-25  Jochen Eisinger  <jochen@chromium.org>
    239 
  • trunk/JavaScriptCore/runtime/JSPropertyNameIterator.cpp

    r54696 r55256  
    5050JSPropertyNameIterator::~JSPropertyNameIterator()
    5151{
    52     if (m_cachedStructure)
    53         m_cachedStructure->clearEnumerationCache(this);
     52    m_cachedStructure->clearEnumerationCache(this);
    5453}
    5554
  • trunk/JavaScriptCore/runtime/JSPropertyNameIterator.h

    r54696 r55256  
    6868        size_t size() { return m_jsStringsSize; }
    6969
    70         void setCachedStructure(Structure* structure) { m_cachedStructure = structure; }
    71         Structure* cachedStructure() { return m_cachedStructure; }
     70        void setCachedStructure(Structure* structure)
     71        {
     72            ASSERT(!m_cachedStructure);
     73            ASSERT(structure);
     74            m_cachedStructure = structure;
     75        }
     76        Structure* cachedStructure() { return m_cachedStructure.get(); }
    7277
    7378        void setCachedPrototypeChain(NonNullPassRefPtr<StructureChain> cachedPrototypeChain) { m_cachedPrototypeChain = cachedPrototypeChain; }
     
    7782        JSPropertyNameIterator(ExecState*, PropertyNameArrayData* propertyNameArrayData, size_t numCacheableSlot);
    7883
    79         Structure* m_cachedStructure;
     84        RefPtr<Structure> m_cachedStructure;
    8085        RefPtr<StructureChain> m_cachedPrototypeChain;
    8186        uint32_t m_numCacheableSlots;
  • trunk/JavaScriptCore/runtime/Structure.cpp

    r54798 r55256  
    266266
    267267    }
    268    
    269     if (m_enumerationCache)
    270         m_enumerationCache->setCachedStructure(0);
     268    ASSERT(!m_enumerationCache.hasDeadObject());
    271269
    272270    if (m_propertyTable) {
  • trunk/JavaScriptCore/runtime/WeakGCPtr.h

    r54428 r55256  
    6666    WeakGCPtr& operator=(T*);
    6767
     68#if !ASSERT_DISABLED
     69    bool hasDeadObject() const { return !!m_ptr; }
     70#endif
     71
    6872private:
    6973    void assign(T* ptr)
  • trunk/LayoutTests/ChangeLog

    r55251 r55256  
     12010-02-25  Oliver Hunt  <oliver@apple.com>
     2
     3        Reviewed by Maciej Stachowiak.
     4
     5        Race condition in JSPropertyNameIterator and Structure destruction
     6        https://bugs.webkit.org/show_bug.cgi?id=35398
     7
     8        Add test to ensure that this race condition doesn't occur.
     9
     10        * fast/js/script-tests/for-in-cached.js:
     11        (cacheClearing):
     12
    1132010-02-25  Alexey Proskuryakov  <ap@apple.com>
    214
  • trunk/LayoutTests/fast/js/script-tests/for-in-cached.js

    r50323 r55256  
    6565shouldBe("forIn5({get foo() { return 'called getter'}, set foo() { }})", "['foo', 'called getter']");
    6666
     67function cacheClearing() {
     68    for(var j=0; j < 10; j++){
     69        var o = {a:1,b:2,c:3,d:4,e:5}
     70        try {for (i in o) { delete o.a; o = null; throw "" };}finally{continue}
     71    }
     72}
     73
     74cacheClearing()
    6775
    6876var successfullyParsed = true;
Note: See TracChangeset for help on using the changeset viewer.