Skip to content
Permalink
Browse files
JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObj…
…ectData 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):
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj:
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters:
* JavaScriptCore.xcodeproj/project.pbxproj:
* heap/HeapInlines.h:
(JSC::Heap::allocateObjectOfType):
(JSC::Heap::subspaceForObjectOfType):
(JSC::Heap::allocatorForObjectOfType):



Canonical link: https://commits.webkit.org/160938@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181806 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Mark Lam committed Mar 20, 2015
1 parent df13713 commit 640892159534408b17b125f6d0e6a5990932bd21
@@ -61,15 +61,4 @@ Structure* JSCallbackObject<JSGlobalObject>::createStructure(VM& vm, JSGlobalObj
return Structure::create(vm, globalObject, proto, TypeInfo(GlobalObjectType, StructureFlags), info());
}

void JSCallbackObjectData::finalize(Handle<Unknown> handle, void* context)
{
JSClassRef jsClass = static_cast<JSClassRef>(context);
JSObjectRef thisRef = toRef(static_cast<JSObject*>(handle.get().asCell()));

for (; jsClass; jsClass = jsClass->parentClass)
if (JSObjectFinalizeCallback finalize = jsClass->finalize)
finalize(thisRef);
WeakSet::deallocate(WeakImpl::asWeakImpl(handle.slot()));
}

} // namespace JSC
@@ -33,15 +33,15 @@

namespace JSC {

struct JSCallbackObjectData : WeakHandleOwner {
struct JSCallbackObjectData {
JSCallbackObjectData(void* privateData, JSClassRef jsClass)
: privateData(privateData)
, jsClass(jsClass)
{
JSClassRetain(jsClass);
}

virtual ~JSCallbackObjectData()
~JSCallbackObjectData()
{
JSClassRelease(jsClass);
}
@@ -109,7 +109,6 @@ struct JSCallbackObjectData : WeakHandleOwner {
PrivatePropertyMap m_propertyMap;
};
std::unique_ptr<JSPrivatePropertyMap> m_privateProperties;
virtual void finalize(Handle<Unknown>, void*) override;
};


@@ -125,6 +124,8 @@ class JSCallbackObject : public Parent {
public:
typedef Parent Base;

~JSCallbackObject();

static JSCallbackObject* create(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, JSClassRef classRef, void* data)
{
ASSERT_UNUSED(globalObject, !structure->globalObject() || structure->globalObject() == globalObject);
@@ -73,6 +73,16 @@ JSCallbackObject<Parent>::JSCallbackObject(VM& vm, JSClassRef jsClass, Structure
{
}

template <class Parent>
JSCallbackObject<Parent>::~JSCallbackObject()
{
JSObjectRef thisRef = toRef(static_cast<JSObject*>(this));
for (JSClassRef jsClass = classRef(); jsClass; jsClass = jsClass->parentClass) {
if (JSObjectFinalizeCallback finalize = jsClass->finalize)
finalize(thisRef);
}
}

template <class Parent>
void JSCallbackObject<Parent>::finishCreation(ExecState* exec)
{
@@ -109,13 +119,6 @@ void JSCallbackObject<Parent>::init(ExecState* exec)
JSObjectInitializeCallback initialize = initRoutines[i];
initialize(toRef(exec), toRef(this));
}

for (JSClassRef jsClassPtr = classRef(); jsClassPtr; jsClassPtr = jsClassPtr->parentClass) {
if (jsClassPtr->finalize) {
WeakSet::allocate(this, m_callbackObjectData.get(), classRef());
break;
}
}
}

template <class Parent>
@@ -0,0 +1,56 @@
/*
* Copyright (C) 2015 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
* THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
* BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
* THE POSSIBILITY OF SUCH DAMAGE.
*/

#include "config.h"
#include "GlobalContextWithFinalizerTest.h"

#include "JavaScriptCore.h"
#include <wtf/DataLog.h>

static bool failed = true;

static void finalize(JSObjectRef)
{
failed = false;
}

int testGlobalContextWithFinalizer()
{
JSClassDefinition def = kJSClassDefinitionEmpty;
def.className = "testClass";
def.finalize = finalize;
JSClassRef classRef = JSClassCreate(&def);

JSGlobalContextRef ref = JSGlobalContextCreateInGroup(nullptr, classRef);
JSGlobalContextRelease(ref);
JSClassRelease(classRef);

if (failed)
printf("FAIL: JSGlobalContextRef did not call its JSClassRef finalizer.\n");
else
printf("PASS: JSGlobalContextRef called its JSClassRef finalizer as expected.\n");

return failed;
}
@@ -0,0 +1,42 @@
/*
* Copyright (C) 2015 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
* THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
* BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
* THE POSSIBILITY OF SUCH DAMAGE.
*/

#ifndef GlobalContextWithFinalizerTest_h
#define GlobalContextWithFinalizerTest_h

#include "JSContextRefPrivate.h"

#ifdef __cplusplus
extern "C" {
#endif

/* Returns 1 if failures were encountered. Else, returns 0. */
int testGlobalContextWithFinalizer();

#ifdef __cplusplus
} /* extern "C" */
#endif

#endif /* GlobalContextWithFinalizerTest_h */
@@ -41,6 +41,7 @@

#include "CompareAndSwapTest.h"
#include "CustomGlobalObjectClassTest.h"
#include "GlobalContextWithFinalizerTest.h"

#if OS(DARWIN)
#include "ExecutionTimeLimitTest.h"
@@ -1856,6 +1857,7 @@ int main(int argc, char* argv[])
#if OS(DARWIN)
failed = testExecutionTimeLimit(&context) || failed;
#endif /* OS(DARWIN) */
failed = testGlobalContextWithFinalizer() || failed;

// Clear out local variables pointing at JSObjectRefs to allow their values to be collected
function = NULL;
@@ -1,3 +1,52 @@
2015-03-19 Mark Lam <mark.lam@apple.com>

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):
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj:
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters:
* JavaScriptCore.xcodeproj/project.pbxproj:
* heap/HeapInlines.h:
(JSC::Heap::allocateObjectOfType):
(JSC::Heap::subspaceForObjectOfType):
(JSC::Heap::allocatorForObjectOfType):

2015-03-19 Andreas Kling <akling@apple.com>

JSCallee unnecessarily overrides a bunch of things in the method table.
@@ -296,6 +296,8 @@
<ClInclude Include="..\..\API\tests\CompareAndSwapTest.h" />
<ClCompile Include="..\..\API\tests\CustomGlobalObjectClassTest.c" />
<ClInclude Include="..\..\API\tests\CustomGlobalObjectClassTest.h" />
<ClCompile Include="..\..\API\tests\GlobalContextWithFinalizerTest.cpp" />
<ClInclude Include="..\..\API\tests\GlobalContextWithFinalizerTest.h" />
</ItemGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
@@ -12,5 +12,7 @@
<ClInclude Include="..\..\API\tests\CompareAndSwapTest.h" />
<ClCompile Include="..\..\API\tests\CustomGlobalObjectClassTest.c" />
<ClInclude Include="..\..\API\tests\CustomGlobalObjectClassTest.h" />
<ClCompile Include="..\..\API\tests\GlobalContextWithFinalizerTest.cpp" />
<ClInclude Include="..\..\API\tests\GlobalContextWithFinalizerTest.h" />
</ItemGroup>
</Project>
@@ -1612,6 +1612,7 @@
E49DC16C12EF294E00184A1F /* SourceProviderCache.h in Headers */ = {isa = PBXBuildFile; fileRef = E49DC15112EF272200184A1F /* SourceProviderCache.h */; settings = {ATTRIBUTES = (Private, ); }; };
E49DC16D12EF295300184A1F /* SourceProviderCacheItem.h in Headers */ = {isa = PBXBuildFile; fileRef = E49DC14912EF261A00184A1F /* SourceProviderCacheItem.h */; settings = {ATTRIBUTES = (Private, ); }; };
FE0D4A061AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */; };
FE0D4A091ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */; };
FE20CE9D15F04A9500DF3430 /* LLIntCLoop.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE20CE9B15F04A9500DF3430 /* LLIntCLoop.cpp */; };
FE20CE9E15F04A9500DF3430 /* LLIntCLoop.h in Headers */ = {isa = PBXBuildFile; fileRef = FE20CE9C15F04A9500DF3430 /* LLIntCLoop.h */; settings = {ATTRIBUTES = (Private, ); }; };
FE4A331F15BD2E07006F54F3 /* VMInspector.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE4A331D15BD2E07006F54F3 /* VMInspector.cpp */; };
@@ -3360,6 +3361,8 @@
F692A8870255597D01FF60F7 /* JSCJSValue.cpp */ = {isa = PBXFileReference; fileEncoding = 30; indentWidth = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSCJSValue.cpp; sourceTree = "<group>"; tabWidth = 8; };
FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ExecutionTimeLimitTest.cpp; path = API/tests/ExecutionTimeLimitTest.cpp; sourceTree = "<group>"; };
FE0D4A051AB8DD0A002F54BF /* ExecutionTimeLimitTest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ExecutionTimeLimitTest.h; path = API/tests/ExecutionTimeLimitTest.h; sourceTree = "<group>"; };
FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = GlobalContextWithFinalizerTest.cpp; path = API/tests/GlobalContextWithFinalizerTest.cpp; sourceTree = "<group>"; };
FE0D4A081ABA2437002F54BF /* GlobalContextWithFinalizerTest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = GlobalContextWithFinalizerTest.h; path = API/tests/GlobalContextWithFinalizerTest.h; sourceTree = "<group>"; };
FE20CE9B15F04A9500DF3430 /* LLIntCLoop.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = LLIntCLoop.cpp; path = llint/LLIntCLoop.cpp; sourceTree = "<group>"; };
FE20CE9C15F04A9500DF3430 /* LLIntCLoop.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = LLIntCLoop.h; path = llint/LLIntCLoop.h; sourceTree = "<group>"; };
FE4A331D15BD2E07006F54F3 /* VMInspector.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = VMInspector.cpp; sourceTree = "<group>"; };
@@ -3747,6 +3750,8 @@
C288B2DD18A54D3E007BE40B /* DateTests.mm */,
FE0D4A041AB8DD0A002F54BF /* ExecutionTimeLimitTest.cpp */,
FE0D4A051AB8DD0A002F54BF /* ExecutionTimeLimitTest.h */,
FE0D4A071ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp */,
FE0D4A081ABA2437002F54BF /* GlobalContextWithFinalizerTest.h */,
C2181FC018A948FB0025A235 /* JSExportTests.h */,
C2181FC118A948FB0025A235 /* JSExportTests.mm */,
65570F581AA4C00A009B3C23 /* Regress141275.h */,
@@ -6787,6 +6792,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
FE0D4A091ABA2437002F54BF /* GlobalContextWithFinalizerTest.cpp in Sources */,
65570F5A1AA4C3EA009B3C23 /* Regress141275.mm in Sources */,
C29ECB031804D0ED00D2CBB4 /* CurrentThisInsideBlockGetterTest.mm in Sources */,
FEB51F6C1A97B688001F921C /* Regress141809.mm in Sources */,
@@ -29,6 +29,8 @@
#include "Heap.h"
#include "JSCell.h"
#include "Structure.h"
#include <type_traits>
#include <wtf/Assertions.h>

namespace JSC {

@@ -237,6 +239,9 @@ inline void* Heap::allocateWithoutDestructor(size_t bytes)
template<typename ClassType>
void* Heap::allocateObjectOfType(size_t bytes)
{
// JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));

if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
return allocateWithImmortalStructureDestructor(bytes);
if (ClassType::needsDestruction)
@@ -247,6 +252,9 @@ void* Heap::allocateObjectOfType(size_t bytes)
template<typename ClassType>
MarkedSpace::Subspace& Heap::subspaceForObjectOfType()
{
// JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));

if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
return subspaceForObjectsWithImmortalStructure();
if (ClassType::needsDestruction)
@@ -257,6 +265,9 @@ MarkedSpace::Subspace& Heap::subspaceForObjectOfType()
template<typename ClassType>
MarkedAllocator& Heap::allocatorForObjectOfType(size_t bytes)
{
// JSCell::classInfo() expects objects allocated with normal destructor to derive from JSDestructibleObject.
ASSERT((!ClassType::needsDestruction || ClassType::hasImmortalStructure || std::is_convertible<ClassType, JSDestructibleObject>::value));

if (ClassType::needsDestruction && ClassType::hasImmortalStructure)
return allocatorForObjectWithImmortalStructureDestructor(bytes);
if (ClassType::needsDestruction)

0 comments on commit 6408921

Please sign in to comment.