Changeset 30395 in webkit

Timestamp:

Feb 18, 2008 9:19:04 PM (16 years ago)

Author:

beidson@apple.com

Message:

WebCore:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac-tiger/Skipped (modified) (1 diff)
• LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock-expected.txt (added)
• LayoutTests/platform/mac/plugins/webScriptObject-exception-deadlock.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bindings/objc/WebScriptObject.mm (modified) (1 diff)
• WebKitTools/ChangeLog (modified) (1 diff)
• WebKitTools/DumpRenderTree/mac/ObjCController.m (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/mac-tiger/Skipped

@@ -5 +5 @@
 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

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/bindings/objc/WebScriptObject.mm

@@ -383 +383 @@
     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 = [WebScriptObject _convertValueToObjcValue:result originRootObject:[self _originRootObject] rootObject:[self _rootObject]];
+    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();
+        }
+
+        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);
     

trunk/WebKitTools/ChangeLog

@@ +1 @@
+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>
 

trunk/WebKitTools/DumpRenderTree/mac/ObjCController.m

@@ -29 +29 @@
 #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
@@ -47 +64 @@
             || aSelector == @selector(accessStoredWebScriptObject)
             || aSelector == @selector(storeWebScriptObject:)
+            || aSelector == @selector(testValueForKey)
         )
         return NO;
@@ -68 +86 @@
     if (aSelector == @selector(storeWebScriptObject:))
         return @"storeWebScriptObject";
+    if (aSelector == @selector(testValueForKey))
+        return @"testValueForKey";
 
     return nil;
@@ -116 +136 @@
 }
 
+- (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
 {