Changeset 146682 in webkit

Timestamp:

Mar 22, 2013 4:49:52 PM (11 years ago)

Author:

mhahnenberg@apple.com

Message:

opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData
​https://bugs.webkit.org/show_bug.cgi?id=113086

Reviewed by Geoffrey Garen.

opaqueJSClassData stores cached prototypes for JSClassRefs in the C API. It doesn't make sense to
share these prototypes within a JSGlobalData across JSGlobalObjects, and in fact doing so will cause
a leak of the original JSGlobalObject that these prototypes were created in. Therefore we should move
this cache to JSGlobalObject where it belongs and where it won't cause memory leaks.

• API/JSBase.cpp: Needed to add an extern "C" so that testapi.c can use the super secret GC function.
• API/JSClassRef.cpp: We now grab the cached context data from the global object rather than the global data.

(OpaqueJSClass::contextData):

• API/JSClassRef.h: Remove this header because it's unnecessary and causes circular dependencies.
• API/tests/testapi.c: Added a new test that makes sure that using the same JSClassRef in two different contexts

doesn't cause leaks of the original global object.
(leakFinalize):
(nestedAllocateObject): This is a hack to bypass the conservative scan of the GC, which was unnecessarily marking
objects and keeping them alive, ruining the test result.
(testLeakingPrototypesAcrossContexts):
(main):

• API/tests/testapi.mm: extern "C" this so we can continue using it here.
• runtime/JSGlobalData.cpp: Remove JSClassRef related stuff.

(JSC::JSGlobalData::~JSGlobalData):

• runtime/JSGlobalData.h:

(JSGlobalData):

• runtime/JSGlobalObject.h: Add the stuff that JSGlobalData had. We add it to JSGlobalObjectRareData so that

clients who don't use the C API don't have to pay the memory cost of this extra HashMap.
(JSGlobalObject):
(JSGlobalObjectRareData):
(JSC::JSGlobalObject::opaqueJSClassData):

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSBase.cpp (modified) (1 diff)
• API/JSClassRef.cpp (modified) (1 diff)
• API/JSClassRef.h (modified) (2 diffs)
• API/tests/testapi.c (modified) (3 diffs)
• API/tests/testapi.mm (modified) (1 diff)
• ChangeLog (modified) (1 diff)
• JavaScriptCore.xcodeproj/project.pbxproj (modified) (3 diffs)
• runtime/JSGlobalData.cpp (modified) (2 diffs)
• runtime/JSGlobalData.h (modified) (2 diffs)
• runtime/JSGlobalObject.h (modified) (5 diffs)

trunk/Source/JavaScriptCore/API/JSBase.cpp

@@ -112 +112 @@
 }
 
-JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+extern "C" JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);
 
 void JSSynchronousGarbageCollectForDebugging(JSContextRef ctx)

trunk/Source/JavaScriptCore/API/JSClassRef.cpp

@@ -152 +152 @@
 OpaqueJSClassContextData& OpaqueJSClass::contextData(ExecState* exec)
 {
-    OwnPtr<OpaqueJSClassContextData>& contextData = exec->globalData().opaqueJSClassData.add(this, nullptr).iterator->value;
+    OwnPtr<OpaqueJSClassContextData>& contextData = exec->lexicalGlobalObject()->opaqueJSClassData().add(this, nullptr).iterator->value;
     if (!contextData)
         contextData = adoptPtr(new OpaqueJSClassContextData(exec->globalData(), this));

trunk/Source/JavaScriptCore/API/JSClassRef.h

@@ -27 +27 @@
 #define JSClassRef_h
 
-#include "JSObjectRef.h"
+#include <JavaScriptCore/JSObjectRef.h>
 
 #include "Weak.h"
-#include "JSObject.h"
 #include "Protect.h"
 #include <wtf/HashMap.h>
@@ -88 +87 @@
     static PassRefPtr<OpaqueJSClass> create(const JSClassDefinition*);
     static PassRefPtr<OpaqueJSClass> createNoAutomaticPrototype(const JSClassDefinition*);
-    ~OpaqueJSClass();
+    JS_EXPORT_PRIVATE ~OpaqueJSClass();
     
     String className();

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

@@ -56 +56 @@
 #endif
 
+extern void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+
 static JSGlobalContextRef context;
 int failed;
@@ -131 +133 @@
     free(cfBuffer);
     JSStringRelease(valueAsString);
+}
+
+static int leakedObject = 1;
+
+static void leakFinalize(JSObjectRef object)
+{
+    (void)object;
+    leakedObject = 0;
+}
+
+// This is a hack to avoid the C++ stack keeping the original JSObject alive.
+static void nestedAllocateObject(JSContextRef context, JSClassRef class, unsigned n)
+{
+    if (!n) {
+        JSObjectRef object = JSObjectMake(context, class, 0);
+        JSObjectRef globalObject = JSContextGetGlobalObject(context);
+        JSStringRef propertyName = JSStringCreateWithUTF8CString("value");
+        JSObjectSetProperty(context, globalObject, propertyName, object, kJSPropertyAttributeNone, 0);
+        JSStringRelease(propertyName);
+        return;
+    }
+    nestedAllocateObject(context, class, n - 1);
+}
+
+static void testLeakingPrototypesAcrossContexts()
+{
+    JSClassDefinition leakDefinition = kJSClassDefinitionEmpty;
+    leakDefinition.finalize = leakFinalize;
+    JSClassRef leakClass = JSClassCreate(&leakDefinition);
+
+    JSContextGroupRef group = JSContextGroupCreate();
+
+    {
+        JSGlobalContextRef context1 = JSGlobalContextCreateInGroup(group, NULL);
+        nestedAllocateObject(context1, leakClass, 10);
+        JSGlobalContextRelease(context1);
+    }
+
+    {
+        JSGlobalContextRef context2 = JSGlobalContextCreateInGroup(group, NULL);
+        JSObjectRef object2 = JSObjectMake(context2, leakClass, 0);
+        JSValueProtect(context2, object2);
+        JSSynchronousGarbageCollectForDebugging(context2);
+        if (leakedObject) {
+            printf("FAIL: Failed to finalize the original object after the first GC.\n");
+            failed = 1;
+        } else
+            printf("PASS: Finalized the original object as expected.\n");
+        JSValueUnprotect(context2, object2);
+        JSGlobalContextRelease(context2);
+    }
+
+    JSContextGroupRelease(group);
+
+    JSClassRelease(leakClass);
 }
 
@@ -1686 +1743 @@
     }
 
+    testLeakingPrototypesAcrossContexts();
+
     // Clear out local variables pointing at JSObjectRefs to allow their values to be collected
     function = NULL;

trunk/Source/JavaScriptCore/API/tests/testapi.mm

@@ -26 +26 @@
 #import <JavaScriptCore/JavaScriptCore.h>
 
-void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+extern "C" void JSSynchronousGarbageCollectForDebugging(JSContextRef);
 
 extern "C" bool _Block_has_signature(id);

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2013-03-22  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData
+        https://bugs.webkit.org/show_bug.cgi?id=113086
+
+        Reviewed by Geoffrey Garen.
+
+        opaqueJSClassData stores cached prototypes for JSClassRefs in the C API. It doesn't make sense to 
+        share these prototypes within a JSGlobalData across JSGlobalObjects, and in fact doing so will cause 
+        a leak of the original JSGlobalObject that these prototypes were created in. Therefore we should move 
+        this cache to JSGlobalObject where it belongs and where it won't cause memory leaks.
+
+        * API/JSBase.cpp: Needed to add an extern "C" so that testapi.c can use the super secret GC function.
+        * API/JSClassRef.cpp: We now grab the cached context data from the global object rather than the global data.
+        (OpaqueJSClass::contextData):
+        * API/JSClassRef.h: Remove this header because it's unnecessary and causes circular dependencies.
+        * API/tests/testapi.c: Added a new test that makes sure that using the same JSClassRef in two different contexts
+        doesn't cause leaks of the original global object.
+        (leakFinalize):
+        (nestedAllocateObject): This is a hack to bypass the conservative scan of the GC, which was unnecessarily marking
+        objects and keeping them alive, ruining the test result.
+        (testLeakingPrototypesAcrossContexts):
+        (main):
+        * API/tests/testapi.mm: extern "C" this so we can continue using it here.
+        * runtime/JSGlobalData.cpp: Remove JSClassRef related stuff.
+        (JSC::JSGlobalData::~JSGlobalData):
+        * runtime/JSGlobalData.h:
+        (JSGlobalData):
+        * runtime/JSGlobalObject.h: Add the stuff that JSGlobalData had. We add it to JSGlobalObjectRareData so that 
+        clients who don't use the C API don't have to pay the memory cost of this extra HashMap.
+        (JSGlobalObject):
+        (JSGlobalObjectRareData):
+        (JSC::JSGlobalObject::opaqueJSClassData):
+
 2013-03-19  Martin Robinson  <mrobinson@igalia.com>
 

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

@@ -743 +743 @@
                 BC18C41B0E16F5CD00B34460 /* JSCallbackObject.h in Headers */ = {isa = PBXBuildFile; fileRef = 14ABDF5D0A437FEF00ECCA01 /* JSCallbackObject.h */; };
                 BC18C41C0E16F5CD00B34460 /* JSCallbackObjectFunctions.h in Headers */ = {isa = PBXBuildFile; fileRef = A8E894310CD0602400367179 /* JSCallbackObjectFunctions.h */; };
-                BC18C41D0E16F5CD00B34460 /* JSClassRef.h in Headers */ = {isa = PBXBuildFile; fileRef = 1440FCE10A51E46B0005F061 /* JSClassRef.h */; };
+                BC18C41D0E16F5CD00B34460 /* JSClassRef.h in Headers */ = {isa = PBXBuildFile; fileRef = 1440FCE10A51E46B0005F061 /* JSClassRef.h */; settings = {ATTRIBUTES = (Private, ); }; };
                 BC18C41E0E16F5CD00B34460 /* JSContextRef.h in Headers */ = {isa = PBXBuildFile; fileRef = 14BD5A2A0A3E91F600BAF59C /* JSContextRef.h */; settings = {ATTRIBUTES = (Public, ); }; };
                 BC18C41F0E16F5CD00B34460 /* JSFunction.h in Headers */ = {isa = PBXBuildFile; fileRef = F692A85F0255597D01FF60F7 /* JSFunction.h */; settings = {ATTRIBUTES = (Private, ); }; };
@@ -2957 +2957 @@
                                 0F63945515D07057006A597C /* ArrayProfile.h in Headers */,
                                 BC18C3E70E16F5CD00B34460 /* ArrayPrototype.h in Headers */,
+                                BC18C41D0E16F5CD00B34460 /* JSClassRef.h in Headers */,
                                 86E3C61D167BABEE006D760A /* JSVirtualMachineInternal.h in Headers */,
                                 BC18C5240E16FC8A00B34460 /* ArrayPrototype.lut.h in Headers */,
@@ -3155 +3156 @@
                                 BC18C42B0E16F5CD00B34460 /* JSCJSValue.h in Headers */,
                                 865A30F1135007E100CDB49E /* JSCJSValueInlines.h in Headers */,
-                                BC18C41D0E16F5CD00B34460 /* JSClassRef.h in Headers */,
                                 86E3C613167BABD7006D760A /* JSContext.h in Headers */,
                                 BC18C41E0E16F5CD00B34460 /* JSContextRef.h in Headers */,

trunk/Source/JavaScriptCore/runtime/JSGlobalData.cpp

@@ -45 +45 @@
 #include "JSAPIValueWrapper.h"
 #include "JSArray.h"
-#include "JSClassRef.h"
 #include "JSFunction.h"
 #include "JSLock.h"
@@ -307 +306 @@
     fastDelete(const_cast<HashTable*>(stringConstructorTable));
 
-    opaqueJSClassData.clear();
-
     delete emptyList;
 

trunk/Source/JavaScriptCore/runtime/JSGlobalData.h

@@ -63 +63 @@
 #endif
 
-struct OpaqueJSClass;
-struct OpaqueJSClassContextData;
-
 namespace JSC {
 
@@ -370 +367 @@
 #endif
 
-        HashMap<OpaqueJSClass*, OwnPtr<OpaqueJSClassContextData> > opaqueJSClassData;
-
         JSGlobalObject* dynamicGlobalObject;
 

trunk/Source/JavaScriptCore/runtime/JSGlobalObject.h

@@ -25 +25 @@
 #include "ArrayAllocationProfile.h"
 #include "JSArray.h"
+#include "JSClassRef.h"
 #include "JSGlobalData.h"
 #include "JSSegmentedVariableObject.h"
@@ -38 +39 @@
 #include <wtf/OwnPtr.h>
 #include <wtf/RandomNumber.h>
+
+struct OpaqueJSClass;
+struct OpaqueJSClassContextData;
 
 namespace JSC {
@@ -87 +91 @@
 private:
     typedef HashSet<RefPtr<OpaqueJSWeakObjectMap> > WeakMapSet;
+    typedef HashMap<OpaqueJSClass*, OwnPtr<OpaqueJSClassContextData> > OpaqueJSClassDataMap;
 
     struct JSGlobalObjectRareData {
@@ -96 +101 @@
         WeakMapSet weakMaps;
         unsigned profileGroup;
+        
+        OpaqueJSClassDataMap opaqueJSClassData;
     };
 
@@ -396 +403 @@
     }
 
+    OpaqueJSClassDataMap& opaqueJSClassData()
+    {
+        createRareDataIfNeeded();
+        return m_rareData->opaqueJSClassData;
+    }
+
     double weakRandomNumber() { return m_weakRandom.get(); }
     unsigned weakRandomInteger() { return m_weakRandom.getUint32(); }