Fix a crash seen running DumpRenderTree on fast/dom/null-document-window-open-crash...
authormrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Sep 2008 07:09:37 +0000 (07:09 +0000)
committermrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Sep 2008 07:09:37 +0000 (07:09 +0000)
Reviewed by Sam Weinig.

The JS wrapper for LayoutTestController could outlive the wrapped instance, and would crash when
attempting to access the wrapped instance within layoutTestControllerObjectFinalize. We fix this by making
LayoutTestController ref-counted to ensure that it is not outlived by the JS wrapper.

* DumpRenderTree/ForwardingHeaders/wtf/RefCounted.h: Copied from JavaScriptGlue/ForwardingHeaders/wtf/RefCounted.h.
* DumpRenderTree/LayoutTestController.cpp:
(LayoutTestController::LayoutTestController):
(notifyDoneCallback): Remove code that is no longer needed now that we must always have a wrapped instance.
(layoutTestControllerObjectFinalize): Deref the wrapped object.
(LayoutTestController::makeWindowObject): Ref the wrapped object.
* DumpRenderTree/LayoutTestController.h: Make LayoutTestController RefCounted.
* DumpRenderTree/mac/DumpRenderTree.mm:
(runTest): Deref the LayoutTestController object rather than explicitly deleting it.
* DumpRenderTree/mac/LayoutTestControllerMac.mm: Remove code that is no longer needed.
* DumpRenderTree/win/DumpRenderTree.cpp:
(runTest): Deref the LayoutTestController object rather than explicitly deleting it.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@36606 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/RefCounted.h [new file with mode: 0644]
WebKitTools/DumpRenderTree/LayoutTestController.cpp
WebKitTools/DumpRenderTree/LayoutTestController.h
WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp

index f6ceee8..7f3c6f4 100644 (file)
@@ -1,3 +1,26 @@
+2008-09-17  Mark Rowe  <mrowe@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        Fix a crash seen running DumpRenderTree on fast/dom/null-document-window-open-crash.html under guard malloc.
+
+        The JS wrapper for LayoutTestController could outlive the wrapped instance, and would crash when
+        attempting to access the wrapped instance within layoutTestControllerObjectFinalize. We fix this by making
+        LayoutTestController ref-counted to ensure that it is not outlived by the JS wrapper.
+
+        * DumpRenderTree/ForwardingHeaders/wtf/RefCounted.h: Copied from JavaScriptGlue/ForwardingHeaders/wtf/RefCounted.h.
+        * DumpRenderTree/LayoutTestController.cpp:
+        (LayoutTestController::LayoutTestController):
+        (notifyDoneCallback): Remove code that is no longer needed now that we must always have a wrapped instance.
+        (layoutTestControllerObjectFinalize): Deref the wrapped object.
+        (LayoutTestController::makeWindowObject): Ref the wrapped object.
+        * DumpRenderTree/LayoutTestController.h: Make LayoutTestController RefCounted.
+        * DumpRenderTree/mac/DumpRenderTree.mm:
+        (runTest): Deref the LayoutTestController object rather than explicitly deleting it.
+        * DumpRenderTree/mac/LayoutTestControllerMac.mm: Remove code that is no longer needed.
+        * DumpRenderTree/win/DumpRenderTree.cpp:
+        (runTest): Deref the LayoutTestController object rather than explicitly deleting it.
+
 2008-09-16  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Mark Rowe.
diff --git a/WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/RefCounted.h b/WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/RefCounted.h
new file mode 100644 (file)
index 0000000..628a63b
--- /dev/null
@@ -0,0 +1 @@
+#include <JavaScriptCore/RefCounted.h>
index 8b1115d..f0ae8d3 100644 (file)
@@ -59,7 +59,6 @@ LayoutTestController::LayoutTestController(bool testRepaintDefault, bool testRep
     , m_waitToDump(false)
     , m_windowIsKey(true)
     , m_globalFlag(false)
-    , m_selfJSObject(0)
 {
 }
 
@@ -338,13 +337,7 @@ static JSValueRef notifyDoneCallback(JSContextRef context, JSObjectRef function,
     // Has mac & windows implementation
     // May be able to be made platform independant by using shared WorkQueue
     LayoutTestController* controller = reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
-    if (controller)
-        controller->notifyDone();
-    else {
-        const char* msg = "FAIL: Null layoutTestController in notifyDone(). Did a test call notifyDone() twice?\n";
-        fprintf(stderr, "%s", msg);
-        fprintf(stdout, "%s", msg);
-    }
+    controller->notifyDone();
     return JSValueMakeUndefined(context);
 }
 
@@ -649,8 +642,7 @@ static bool setGlobalFlagCallback(JSContextRef context, JSObjectRef thisObject,
 static void layoutTestControllerObjectFinalize(JSObjectRef object)
 {
     LayoutTestController* controller = reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(object));
-    if (controller)
-        controller->setJSObject(0);
+    controller->deref();
 }
 
 // Object Creation
@@ -658,8 +650,9 @@ static void layoutTestControllerObjectFinalize(JSObjectRef object)
 void LayoutTestController::makeWindowObject(JSContextRef context, JSObjectRef windowObject, JSValueRef* exception)
 {
     JSRetainPtr<JSStringRef> layoutTestContollerStr(Adopt, JSStringCreateWithUTF8CString("layoutTestController"));
-    setJSObject(JSObjectMake(context, getJSClass(), this));
-    JSObjectSetProperty(context, windowObject, layoutTestContollerStr.get(), jsObject(), kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, exception);
+    ref();
+    JSValueRef layoutTestContollerObject = JSObjectMake(context, getJSClass(), this);
+    JSObjectSetProperty(context, windowObject, layoutTestContollerStr.get(), layoutTestContollerObject, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, exception);
 }
 
 JSClassRef LayoutTestController::getJSClass()
index f64d9f7..5e303c8 100644 (file)
@@ -30,8 +30,9 @@
 #define LayoutTestController_h
 
 #include <JavaScriptCore/JSObjectRef.h>
+#include <wtf/RefCounted.h>
 
-class LayoutTestController {
+class LayoutTestController : public RefCounted<LayoutTestController> {
 public:
     LayoutTestController(bool testRepaintDefault, bool testRepaintSweepHorizontallyDefault);
     ~LayoutTestController();
@@ -142,10 +143,7 @@ public:
 
     bool globalFlag() const { return m_globalFlag; }
     void setGlobalFlag(bool globalFlag) { m_globalFlag = globalFlag; }
-    
-    JSObjectRef jsObject() const { return m_selfJSObject; }
-    void setJSObject(JSObjectRef obj) { m_selfJSObject = obj; }
-    
+
 private:
     bool m_dumpAsText;
     bool m_dumpAsPDF;
@@ -173,8 +171,6 @@ private:
 
     bool m_globalFlag;
 
-    JSObjectRef m_selfJSObject;
-    
     static JSClassRef getJSClass();
     static JSStaticValue* staticValues();
     static JSStaticFunction* staticFunctions();
index 8f3864e..3b5ec94 100644 (file)
@@ -1048,7 +1048,7 @@ static void runTest(const char *pathOrURL)
     ASSERT(CFArrayGetCount(openWindowsRef) == 1);
     ASSERT(CFArrayGetValueAtIndex(openWindowsRef, 0) == [[mainFrame webView] window]);
 
-    delete gLayoutTestController;
+    gLayoutTestController->deref();
     gLayoutTestController = 0;
 
     if (_shouldIgnoreWebCoreNodeLeaks)
index dea4463..3a47aeb 100644 (file)
 #import <WebKit/WebSecurityOriginPrivate.h>
 #import <WebKit/WebView.h>
 #import <WebKit/WebViewPrivate.h>
-#import <wtf/Assertions.h>
 #import <wtf/RetainPtr.h>
 
 LayoutTestController::~LayoutTestController()
 {
-    if (m_selfJSObject) {
-        ASSERT(JSObjectGetPrivate(m_selfJSObject) == this);
-        JSObjectSetPrivate(m_selfJSObject, 0);
-    }
 }
 
 void LayoutTestController::addDisallowedURL(JSStringRef url)
index f26ad21..5a36918 100644 (file)
@@ -761,7 +761,8 @@ static void runTest(const char* pathOrURL)
 
 exit:
     SysFreeString(urlBStr);
-    delete ::gLayoutTestController;
+    ::gLayoutTestController->deref();
+    ::gLayoutTestController = 0;
 
     return;
 }