Diff
Modified: trunk/Source/_javascript_Core/API/JSCallbackObject.cpp (181805 => 181806)
--- trunk/Source/_javascript_Core/API/JSCallbackObject.cpp 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/API/JSCallbackObject.cpp 2015-03-20 18:08:29 UTC (rev 181806)
@@ -61,15 +61,4 @@
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
Modified: trunk/Source/_javascript_Core/API/JSCallbackObject.h (181805 => 181806)
--- trunk/Source/_javascript_Core/API/JSCallbackObject.h 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/API/JSCallbackObject.h 2015-03-20 18:08:29 UTC (rev 181806)
@@ -33,7 +33,7 @@
namespace JSC {
-struct JSCallbackObjectData : WeakHandleOwner {
+struct JSCallbackObjectData {
JSCallbackObjectData(void* privateData, JSClassRef jsClass)
: privateData(privateData)
, jsClass(jsClass)
@@ -41,7 +41,7 @@
JSClassRetain(jsClass);
}
- virtual ~JSCallbackObjectData()
+ ~JSCallbackObjectData()
{
JSClassRelease(jsClass);
}
@@ -109,7 +109,6 @@
PrivatePropertyMap m_propertyMap;
};
std::unique_ptr<JSPrivatePropertyMap> m_privateProperties;
- virtual void finalize(Handle<Unknown>, void*) override;
};
@@ -125,6 +124,8 @@
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);
Modified: trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h (181805 => 181806)
--- trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h 2015-03-20 18:08:29 UTC (rev 181806)
@@ -74,6 +74,16 @@
}
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)
{
Base::finishCreation(exec->vm());
@@ -109,13 +119,6 @@
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>
Added: trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.cpp (0 => 181806)
--- trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.cpp (rev 0)
+++ trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.cpp 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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 "_javascript_Core.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;
+}
Added: trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.h (0 => 181806)
--- trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.h (rev 0)
+++ trunk/Source/_javascript_Core/API/tests/GlobalContextWithFinalizerTest.h 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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 */
Modified: trunk/Source/_javascript_Core/API/tests/testapi.c (181805 => 181806)
--- trunk/Source/_javascript_Core/API/tests/testapi.c 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/API/tests/testapi.c 2015-03-20 18:08:29 UTC (rev 181806)
@@ -41,6 +41,7 @@
#include "CompareAndSwapTest.h"
#include "CustomGlobalObjectClassTest.h"
+#include "GlobalContextWithFinalizerTest.h"
#if OS(DARWIN)
#include "ExecutionTimeLimitTest.h"
@@ -1856,6 +1857,7 @@
#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;
Modified: trunk/Source/_javascript_Core/ChangeLog (181805 => 181806)
--- trunk/Source/_javascript_Core/ChangeLog 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-03-20 18:08:29 UTC (rev 181806)
@@ -1,3 +1,52 @@
+2015-03-19 Mark Lam <mark....@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):
+ * _javascript_Core.vcxproj/testapi/testapi.vcxproj:
+ * _javascript_Core.vcxproj/testapi/testapi.vcxproj.filters:
+ * _javascript_Core.xcodeproj/project.pbxproj:
+ * heap/HeapInlines.h:
+ (JSC::Heap::allocateObjectOfType):
+ (JSC::Heap::subspaceForObjectOfType):
+ (JSC::Heap::allocatorForObjectOfType):
+
2015-03-19 Andreas Kling <akl...@apple.com>
JSCallee unnecessarily overrides a bunch of things in the method table.
Modified: trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj (181805 => 181806)
--- trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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">
Modified: trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj.filters (181805 => 181806)
--- trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj.filters 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/_javascript_Core.vcxproj/testapi/testapi.vcxproj.filters 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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>
Modified: trunk/Source/_javascript_Core/_javascript_Core.xcodeproj/project.pbxproj (181805 => 181806)
--- trunk/Source/_javascript_Core/_javascript_Core.xcodeproj/project.pbxproj 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/_javascript_Core.xcodeproj/project.pbxproj 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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 */,
Modified: trunk/Source/_javascript_Core/heap/HeapInlines.h (181805 => 181806)
--- trunk/Source/_javascript_Core/heap/HeapInlines.h 2015-03-20 17:53:31 UTC (rev 181805)
+++ trunk/Source/_javascript_Core/heap/HeapInlines.h 2015-03-20 18:08:29 UTC (rev 181806)
@@ -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 @@
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 @@
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 @@
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)
