Changeset 181806 in webkit


Ignore:
Timestamp:
Mar 20, 2015 11:08:29 AM (9 years ago)
Author:
mark.lam@apple.com
Message:

JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called.
<​https://webkit.org/b/142846>

Reviewed by Geoffrey Garen.

Currently, JSCallbackObject<JSGlobalObject> registers weak finalizers via 2 mechanisms:

  1. JSCallbackObject<Parent>::init() registers a weak finalizer for all JSClassRef that a JSCallbackObject references.
  2. JSCallbackObject<JSGlobalObject>::create() registers a finalizer via vm.heap.addFinalizer() which destroys the JSCallbackObject.

The first finalizer is implemented as a virtual function of a JSCallbackObjectData
instance that will be destructed if the 2nd finalizer is called. Hence, if the
2nd finalizer if called first, the later invocation of the 1st finalizer will
result in a crash.

This patch fixes the issue by eliminating the finalizer registration in init().
Instead, we'll have the JSCallbackObject destructor call all the JSClassRef finalizers
if needed. This ensures that these finalizers are called before the JSCallbackObject
is destructor.

Also added assertions to a few Heap functions because JSCell::classInfo() expects
all objects that are allocated from MarkedBlock::Normal blocks to be derived from
JSDestructibleObject. These assertions will help us catch violations of this
expectation earlier.

  • API/JSCallbackObject.cpp:

(JSC::JSCallbackObjectData::finalize): Deleted.

  • API/JSCallbackObject.h:

(JSC::JSCallbackObjectData::~JSCallbackObjectData):

  • API/JSCallbackObjectFunctions.h:

(JSC::JSCallbackObject<Parent>::~JSCallbackObject):
(JSC::JSCallbackObject<Parent>::init):

  • API/tests/GlobalContextWithFinalizerTest.cpp: Added.

(finalize):
(testGlobalContextWithFinalizer):

  • API/tests/GlobalContextWithFinalizerTest.h: Added.
  • API/tests/testapi.c:

(main):

(JSC::Heap::allocateObjectOfType):
(JSC::Heap::subspaceForObjectOfType):
(JSC::Heap::allocatorForObjectOfType):

Location:
trunk/Source/JavaScriptCore
Files:
2 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/JSCallbackObject.cpp

    r171824 r181806  
    6262}
    6363
    64 void JSCallbackObjectData::finalize(Handle<Unknown> handle, void* context)
    65 {
    66     JSClassRef jsClass = static_cast<JSClassRef>(context);
    67     JSObjectRef thisRef = toRef(static_cast<JSObject*>(handle.get().asCell()));
    68    
    69     for (; jsClass; jsClass = jsClass->parentClass)
    70         if (JSObjectFinalizeCallback finalize = jsClass->finalize)
    71             finalize(thisRef);
    72     WeakSet::deallocate(WeakImpl::asWeakImpl(handle.slot()));
    73 }
    74    
    7564} // namespace JSC

  • trunk/Source/JavaScriptCore/API/JSCallbackObject.h

    r175651 r181806  
    3434namespace JSC {
    3535
    36 struct JSCallbackObjectData : WeakHandleOwner {
     36struct JSCallbackObjectData {
    3737    JSCallbackObjectData(void* privateData, JSClassRef jsClass)
    3838        : privateData(privateData)
    … …  
    4242    }
    4343   
    44     virtual ~JSCallbackObjectData()
     44    ~JSCallbackObjectData()
    4545    {
    4646        JSClassRelease(jsClass);
    … …  
    110110    };
    111111    std::unique_ptr<JSPrivatePropertyMap> m_privateProperties;
    112     virtual void finalize(Handle<Unknown>, void*) override;
    113112};
    114113
    … …  
    125124public:
    126125    typedef Parent Base;
     126
     127    ~JSCallbackObject();
    127128
    128129    static JSCallbackObject* create(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, JSClassRef classRef, void* data)

  • trunk/Source/JavaScriptCore/API/JSCallbackObjectFunctions.h

    r181064 r181806  
    7575
    7676template <class Parent>
     77JSCallbackObject<Parent>::~JSCallbackObject()
     78{
     79    JSObjectRef thisRef = toRef(static_cast<JSObject*>(this));
     80    for (JSClassRef jsClass = classRef(); jsClass; jsClass = jsClass->parentClass) {
     81        if (JSObjectFinalizeCallback finalize = jsClass->finalize)
     82            finalize(thisRef);
     83    }
     84}
     85   
     86template <class Parent>
    7787void JSCallbackObject<Parent>::finishCreation(ExecState* exec)
    7888{
    … …  
    109119        JSObjectInitializeCallback initialize = initRoutines[i];
    110120        initialize(toRef(exec), toRef(this));
    111     }
    112 
    113     for (JSClassRef jsClassPtr = classRef(); jsClassPtr; jsClassPtr = jsClassPtr->parentClass) {
    114         if (jsClassPtr->finalize) {
    115             WeakSet::allocate(this, m_callbackObjectData.get(), classRef());
    116             break;
    117         }
    118121    }
    119122}

  • trunk/Source/JavaScriptCore/API/tests/testapi.c

    r181670 r181806  
    4242#include "CompareAndSwapTest.h"
    4343#include "CustomGlobalObjectClassTest.h"
     44#include "GlobalContextWithFinalizerTest.h"
    4445
    4546#if OS(DARWIN)
    … …  
    18571858    failed = testExecutionTimeLimit(&context) || failed;
    18581859#endif /* OS(DARWIN) */
     1860    failed = testGlobalContextWithFinalizer() || failed;
    18591861
    18601862    // Clear out local variables pointing at JSObjectRefs to allow their values to be collected

  • trunk/Source/JavaScriptCore/ChangeLog

    r181765 r181806  
     12015-03-19  Mark Lam  <mark.lam@apple.com>
     2
     3        JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called.
     4        <https://webkit.org/b/142846>
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        Currently, JSCallbackObject<JSGlobalObject> registers weak finalizers via 2 mechanisms:
     9        1. JSCallbackObject<Parent>::init() registers a weak finalizer for all JSClassRef
     10           that a JSCallbackObject references.
     11        2. JSCallbackObject<JSGlobalObject>::create() registers a finalizer via
     12           vm.heap.addFinalizer() which destroys the JSCallbackObject.
     13
     14        The first finalizer is implemented as a virtual function of a JSCallbackObjectData
     15        instance that will be destructed if the 2nd finalizer is called.  Hence, if the
     16        2nd finalizer if called first, the later invocation of the 1st finalizer will
     17        result in a crash.
     18
     19        This patch fixes the issue by eliminating the finalizer registration in init().
     20        Instead, we'll have the JSCallbackObject destructor call all the JSClassRef finalizers
     21        if needed.  This ensures that these finalizers are called before the JSCallbackObject
     22        is destructor.
     23
     24        Also added assertions to a few Heap functions because JSCell::classInfo() expects
     25        all objects that are allocated from MarkedBlock::Normal blocks to be derived from
     26        JSDestructibleObject.  These assertions will help us catch violations of this
     27        expectation earlier.
     28
     29        * API/JSCallbackObject.cpp:
     30        (JSC::JSCallbackObjectData::finalize): Deleted.
     31        * API/JSCallbackObject.h:
     32        (JSC::JSCallbackObjectData::~JSCallbackObjectData):
     33        * API/JSCallbackObjectFunctions.h:
     34        (JSC::JSCallbackObject<Parent>::~JSCallbackObject):
     35        (JSC::JSCallbackObject<Parent>::init):
     36        * API/tests/GlobalContextWithFinalizerTest.cpp: Added.
     37        (finalize):
     38        (testGlobalContextWithFinalizer):
     39        * API/tests/GlobalContextWithFinalizerTest.h: Added.
     40        * API/tests/testapi.c:
     41        (main):
     42        * JavaScriptCore.vcxproj/testapi/testapi.vcxproj:
     43        * JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters:
     44        * JavaScriptCore.xcodeproj/project.pbxproj:
     45        * heap/HeapInlines.h:
     46        (JSC::Heap::allocateObjectOfType):
     47        (JSC::Heap::subspaceForObjectOfType):
     48        (JSC::Heap::allocatorForObjectOfType):
     49
    1502015-03-19  Andreas Kling  <akling@apple.com>
    251

  • trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/testapi/testapi.vcxproj

    r181315 r181806  
    297297    <ClCompile Include="..\..\API\tests\CustomGlobalObjectClassTest.c" />
    298298    <ClInclude Include="..\..\API\tests\CustomGlobalObjectClassTest.h" />
     299    <ClCompile Include="..\..\API\tests\GlobalContextWithFinalizerTest.cpp" />
     300    <ClInclude Include="..\..\API\tests\GlobalContextWithFinalizerTest.h" />
    299301  </ItemGroup>
    300302  <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />

  • trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters

    r181315 r181806  
    1313    <ClCompile Include="..\..\API\tests\CustomGlobalObjectClassTest.c" />
    1414    <ClInclude Include="..\..\API\tests\CustomGlobalObjectClassTest.h" />
     15    <ClCompile Include="..\..\API\tests\GlobalContextWithFinalizerTest.cpp" />
     16    <ClInclude Include="..\..\API\tests\GlobalContextWithFinalizerTest.h" />
    1517  </ItemGroup>
    1618</Project>

  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r181670 r181806  
    16131613                E49DC16D12EF295300184A1F /* SourceProviderCacheItem.h in Headers */ = {isa = PBXBuildFile; fileRef = E49DC14912EF261A00184A1F /* SourceProviderCacheItem.h */; settings = {ATTRIBUTES = (Private, ); }; };
    16141614                FE0D4A061AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */; };
     1615                FE0D4A091ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */; };
    16151616                FE20CE9D15F04A9500DF3430 /* LLIntCLoop.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE20CE9B15F04A9500DF3430 /* LLIntCLoop.cpp */; };
    16161617                FE20CE9E15F04A9500DF3430 /* LLIntCLoop.h in Headers */ = {isa = PBXBuildFile; fileRef = FE20CE9C15F04A9500DF3430 /* LLIntCLoop.h */; settings = {ATTRIBUTES = (Private, ); }; };
    … …  
    33613362                FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ExecutionTimeLimitTest.cpp; path = API/tests/ExecutionTimeLimitTest.cpp; sourceTree = "<group>"; };
    33623363                FE0D4A051AB8DD0A002F54BF /* ExecutionTimeLimitTest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ExecutionTimeLimitTest.h; path = API/tests/ExecutionTimeLimitTest.h; sourceTree = "<group>"; };
     3364                FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = GlobalContextWithFinalizerTest.cpp; path = API/tests/GlobalContextWithFinalizerTest.cpp; sourceTree = "<group>"; };
     3365                FE0D4A081ABA2437002F54BF /* GlobalContextWithFinalizerTest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = GlobalContextWithFinalizerTest.h; path = API/tests/GlobalContextWithFinalizerTest.h; sourceTree = "<group>"; };
    33633366                FE20CE9B15F04A9500DF3430 /* LLIntCLoop.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = LLIntCLoop.cpp; path = llint/LLIntCLoop.cpp; sourceTree = "<group>"; };
    33643367                FE20CE9C15F04A9500DF3430 /* LLIntCLoop.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = LLIntCLoop.h; path = llint/LLIntCLoop.h; sourceTree = "<group>"; };
    … …  
    37483751                                FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */,
    37493752                                FE0D4A051AB8DD0A002F54BF /* ExecutionTimeLimitTest.h */,
     3753                                FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */,
     3754                                FE0D4A081ABA2437002F54BF /* GlobalContextWithFinalizerTest.h */,
    37503755                                C2181FC018A948FB0025A235 /* JSExportTests.h */,
    37513756                                C2181FC118A948FB0025A235 /* JSExportTests.mm */,
    … …  
    67886793                        buildActionMask = 2147483647;
    67896794                        files = (
     6795                                FE0D4A091ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp in Sources */,
    67906796                                65570F5A1AA4C3EA009B3C23 /* Regress141275.mm in Sources */,
    67916797                                C29ECB031804D0ED00D2CBB4 /* CurrentThisInsideBlockGetterTest.mm in Sources */,

  • trunk/Source/JavaScriptCore/heap/HeapInlines.h

    r181407 r181806  
    3030#include "JSCell.h"
    3131#include "Structure.h"
     32#include <type_traits>
     33#include <wtf/Assertions.h>
    3234
    3335namespace JSC {
    … …  
    238240void* Heap::allocateObjectOfType(size_t bytes)
    239241{
     242    // JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
     243    ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));
     244
    240245    if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
    241246        return allocateWithImmortalStructureDestructor(bytes);
    … …  
    248253MarkedSpace::Subspace& Heap::subspaceForObjectOfType()
    249254{
     255    // JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
     256    ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));
     257   
    250258    if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
    251259        return subspaceForObjectsWithImmortalStructure();
    … …  
    258266MarkedAllocator& Heap::allocatorForObjectOfType(size_t bytes)
    259267{
     268    // JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
     269    ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));
     270   
    260271    if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
    261272        return allocatorForObjectWithImmortalStructureDestructor(bytes);
Note: See TracChangeset for help on using the changeset viewer.