WebCore:
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2008 05:19:04 +0000 (05:19 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2008 05:19:04 +0000 (05:19 +0000)
        Reviewed by Darin

        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released

        Test: platform/mac/plugins/webScriptObject-exception-deadlock.html

        * bindings/objc/WebScriptObject.mm:
        (-[WebScriptObject valueForKey:]): The line `resultObj = [super valueForKey:key];    // defaults to throwing an exception`
          says it all - it throws an exception.  This method also happens to hold the JSLock.  Problematically, when the exeception
          is thrown and the method exited, the JSLock is never released.  Fix that without otherwise changing behavior by holding the
          JSLock in two individual scopes - Right before the exception and right after.

WebKitTools:

        Changes by Geoff Garen, Reviewed by Darin

        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released

        DRT changes for test: platform/mac/plugins/webScriptObject-exception-deadlock.html

        [WebScriptObject valueForKey:] might throw an exception, and previously might have "leaked" the global JSLock
        This test calls valueForKey, then runs some arbitrary Javascript on a 2ndary thread.  If the lock has leaked,
        this series of method calls will deadlock.  If things are good, it will complete successfully.

        * DumpRenderTree/mac/ObjCController.m:
        (runJavaScriptThread):
        (+[ObjCController isSelectorExcludedFromWebScript:]):
        (+[ObjCController webScriptNameForSelector:]):
        (-[ObjCController testValueForKey]):

LayoutTests:

        Reviewed by Darin

        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released

        * platform/mac-tiger/Skipped: Removed 2 hanging tests that now don't hang
        * platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt: Added.
        * platform/mac/plugins/webScriptObject-exception-deadlock.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/platform/mac-tiger/Skipped
LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/objc/WebScriptObject.mm
WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/mac/ObjCController.m

index cab4b4f15d5fd138237bb8e41382e4d5f4b1695c..3c8ec7e4707c7f6d5d92418c8bdbd16f92ab5686 100644 (file)
@@ -1,3 +1,13 @@
+2008-02-18  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released
+
+        * platform/mac-tiger/Skipped: Removed 2 hanging tests that now don't hang
+        * platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt: Added.
+        * platform/mac/plugins/webScriptObject-exception-deadlock.html: Added.
+
 2008-02-18  Darin Adler  <darin@apple.com>
 
         Reviewed by Sam.
index dde518cbdd2063107edf2d9bd86bcc6dfcf4210e..206de05fff66930c521bd569101db57d5a3d4cba 100644 (file)
@@ -4,9 +4,5 @@ fast/cookies/local-file-can-set-cookies.html
 # For this test to work on Tiger we'd need a new Foundation
 http/tests/xmlhttprequest/small-chunks-response-text.html
 
-# Storage tests failing only on Tiger - <rdar://problem/5747529>
-storage/quota-tracking.html
-storage/success-callback.html
-
 # On Tiger, WebKit does not override the MIME type returned by NSHTTPURLResponse
 http/tests/loading/text-content-type-with-binary-extension.html
diff --git a/LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt b/LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt
new file mode 100644 (file)
index 0000000..78e121e
--- /dev/null
@@ -0,0 +1 @@
+This test checks for an exception in [WebScriptObject valueForKey:(NSString *)key] That method is liable to throw an exception which caused the JSLock to never be released, leading to a possible deadlock. If this test doesn't hang, it passed.
diff --git a/LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock.html b/LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock.html
new file mode 100644 (file)
index 0000000..c715d31
--- /dev/null
@@ -0,0 +1,16 @@
+<html>
+<script>
+
+if (window.layoutTestController) {
+    layoutTestController.dumpAsText();
+    objCController.storeWebScriptObject(window);
+    objCController.testValueForKey();
+}
+
+</script>
+<body>
+This test checks for an exception in [WebScriptObject valueForKey:(NSString *)key]
+That method is liable to throw an exception which caused the JSLock to never be released, leading to a possible deadlock.
+If this test doesn't hang, it passed.
+</body>
+</html>
index 3a1dfe983b5d427e37ef6c9b36f93b6d4354d36d..a5178ac4616204c94f069d78b63740ea54054dd5 100644 (file)
@@ -1,3 +1,17 @@
+2008-02-18  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released
+
+        Test: platform/mac/plugins/webScriptObject-exception-deadlock.html
+
+        * bindings/objc/WebScriptObject.mm:
+        (-[WebScriptObject valueForKey:]): The line `resultObj = [super valueForKey:key];    // defaults to throwing an exception` 
+          says it all - it throws an exception.  This method also happens to hold the JSLock.  Problematically, when the exeception
+          is thrown and the method exited, the JSLock is never released.  Fix that without otherwise changing behavior by holding the 
+          JSLock in two individual scopes - Right before the exception and right after.  
+
 2008-02-18  Darin Adler  <darin@apple.com>
 
         Reviewed by Sam.
index f231d29d4adea9b12863a988dbd623bef91aa13f..4f1168bef31179e2febbe5b54245f6280a8888c3 100644 (file)
@@ -382,19 +382,28 @@ static void getListFromNSArray(ExecState *exec, NSArray *array, RootObject* root
     ExecState* exec = [self _rootObject]->globalObject()->globalExec();
     ASSERT(!exec->hadException());
 
-    JSLock lock;
-    JSValue *result = [self _imp]->get(exec, String(key));
-    
-    if (exec->hadException()) {
-        LOG_EXCEPTION(exec);
-        result = jsUndefined();
-        exec->clearException();
-    }
+    id resultObj;
+    {
+        // Need to scope this lock to ensure that we release the lock before calling
+        // [super valueForKey:key] which might throw an exception and bypass the JSLock destructor,
+        // leaving the lock permanently held
+        JSLock lock;
+        
+        JSValue *result = [self _imp]->get(exec, String(key));
+        
+        if (exec->hadException()) {
+            LOG_EXCEPTION(exec);
+            result = jsUndefined();
+            exec->clearException();
+        }
 
-    id resultObj = [WebScriptObject _convertValueToObjcValue:result originRootObject:[self _originRootObject] rootObject:[self _rootObject]];
+        resultObj = [WebScriptObject _convertValueToObjcValue:result originRootObject:[self _originRootObject] rootObject:[self _rootObject]];
+    }
+    
     if ([resultObj isKindOfClass:[WebUndefined class]])
         resultObj = [super valueForKey:key];    // defaults to throwing an exception
 
+    JSLock lock;
     _didExecute(self);
     
     return resultObj;
index 5395cc898814d5461115d8f7e35a393678114050..8e5998e15c9c58c1128659c8bd8141410fa592e0 100644 (file)
@@ -1,3 +1,21 @@
+2008-02-18  Brady Eidson  <beidson@apple.com>
+
+        Changes by Geoff Garen, Reviewed by Darin
+
+        Fix for <rdar://5747529> - ObjC Exception can cause JSLock to never be released
+
+        DRT changes for test: platform/mac/plugins/webScriptObject-exception-deadlock.html
+
+        [WebScriptObject valueForKey:] might throw an exception, and previously might have "leaked" the global JSLock
+        This test calls valueForKey, then runs some arbitrary Javascript on a 2ndary thread.  If the lock has leaked,
+        this series of method calls will deadlock.  If things are good, it will complete successfully.
+
+        * DumpRenderTree/mac/ObjCController.m:
+        (runJavaScriptThread):
+        (+[ObjCController isSelectorExcludedFromWebScript:]):
+        (+[ObjCController webScriptNameForSelector:]):
+        (-[ObjCController testValueForKey]):
+
 2008-02-18  Matt Lilek  <webkit@mattlilek.com>
 
         Reviewed by Adam.
index 1b9abb7b1ecd9f706b80f1756d7788d66fd31df5..9ca9299de2aaf2db3a37bba2f5678458d26eb5bf 100644 (file)
 
 #import "ObjCController.h"
 
+#import <JavaScriptCore/JavaScriptCore.h>
 #import <WebKit/DOMAbstractView.h>
 #import <WebKit/WebScriptObject.h>
 #import <WebKit/WebView.h>
+#import <pthread.h>
 #import <wtf/Assertions.h>
 
+static void* runJavaScriptThread(void* arg)
+{
+    JSGlobalContextRef ctx = JSGlobalContextCreate(0);
+    JSStringRef scriptRef = JSStringCreateWithUTF8CString("'Hello World!'");
+
+    JSValueRef exception = 0;
+    JSEvaluateScript(ctx, scriptRef, 0, 0, 0, &exception);
+    ASSERT(!exception);
+
+    JSGlobalContextRelease(ctx);
+    JSStringRelease(scriptRef);
+    
+    return 0;
+}
+
 @implementation ObjCController
 
 + (BOOL)isSelectorExcludedFromWebScript:(SEL)aSelector
@@ -46,6 +63,7 @@
             || aSelector == @selector(testWrapperRoundTripping:)
             || aSelector == @selector(accessStoredWebScriptObject)
             || aSelector == @selector(storeWebScriptObject:)
+            || aSelector == @selector(testValueForKey)
         )
         return NO;
     return YES;
@@ -67,6 +85,8 @@
         return @"testWrapperRoundTripping";
     if (aSelector == @selector(storeWebScriptObject:))
         return @"storeWebScriptObject";
+    if (aSelector == @selector(testValueForKey))
+        return @"testValueForKey";
 
     return nil;
 }
     return num;
 }
 
+- (void)testValueForKey
+{
+    ASSERT(storedWebScriptObject);
+    
+    @try {
+        [storedWebScriptObject valueForKey:@"ThisKeyDoesNotExist"];
+    } @catch (NSException *e) {
+    }
+
+    pthread_t pthread;
+    pthread_create(&pthread, 0, &runJavaScriptThread, 0);
+    pthread_join(pthread, 0);
+}
+
 - (BOOL)testWrapperRoundTripping:(WebScriptObject *)webScriptObject
 {
     JSObjectRef jsObject = [webScriptObject JSObject];