WebCore:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Sep 2007 02:11:08 +0000 (02:11 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Sep 2007 02:11:08 +0000 (02:11 +0000)
        Reviewed by Darin Adler.

        Speculative fix for <rdar://problem/5479443> REGRESSION: Hang due to
        infinite JS recursion on close @ engadget.com (onunload-based ad)

        If page is NULL, shouldInterruptScript now returns true, so you can't
        get stuck in a state in which a script executes forever without putting
        up a UI to ask if it should stop.

        * bindings/js/kjs_binding.cpp:
        (KJS::ScriptInterpreter::shouldInterruptScript):

WebKit:

        Reviewed by Darin Adler.

        Fixed a hang due to an infinite script running in the window's unload
        event handler, which may be the cause of <rdar://problem/5479443>
        REGRESSION: Hang due to infinite JS recursion on close @ engadget.com
        (onunload-based ad)

        * WebView/WebUIDelegatePrivate.h: Added FIXME.

        * WebView/WebView.h: Clarified headerdoc ambiguity about when delegate
        methods stop firing.

        * WebView/WebView.mm:
        (-[WebView _close]): The fix: don't nil out our delegates until after
        detaching the FrameLoader, because the act of detaching the FrameLoader
        might fire important delegate methods, like webViewShouldInterruptJavaScript:.
        Don't do other tear-down either, because the unload event handler needs
        to run in a fully constructed page.

        This change is fairly low risk because niling out our delegates is a
        very recent, never-shipped feature in WebKit, so it's unlikely that any
        apps rely on it in a crazy way.

win:

        Reviewed by Darin Adler.

        Fixed a hang due to an infinite script running in the window's unload
        event handler, which may be the cause of <rdar://problem/5479443>
        REGRESSION: Hang due to infinite JS recursion on close @ engadget.com
        (onunload-based ad)

        Added a bunch of WebKitMac's close features, and reordered others to
        match WebKitMac.

        * WebView.cpp:
        (WebView::close):
        (WebView::removeDragCaret):

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

WebCore/ChangeLog
WebCore/bindings/js/kjs_binding.cpp
WebKit/ChangeLog
WebKit/WebView/WebUIDelegatePrivate.h
WebKit/WebView/WebView.h
WebKit/WebView/WebView.mm
WebKit/win/ChangeLog
WebKit/win/WebView.cpp

index a394ef727a24d2f0ef69656621d9c4506e1f8e91..d022fbae327b8947630454eb27ff5ef348fea0fe 100644 (file)
@@ -1,3 +1,17 @@
+2007-09-17  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+
+        Speculative fix for <rdar://problem/5479443> REGRESSION: Hang due to 
+        infinite JS recursion on close @ engadget.com (onunload-based ad)
+        
+        If page is NULL, shouldInterruptScript now returns true, so you can't 
+        get stuck in a state in which a script executes forever without putting 
+        up a UI to ask if it should stop.
+
+        * bindings/js/kjs_binding.cpp:
+        (KJS::ScriptInterpreter::shouldInterruptScript):
+
 2007-09-17  Dave Hyatt  <hyatt@apple.com>
 
         Fix for bug 14743, missing glyphs on many international sites because of MLang's tiny cache.
index 3dc5810d0f1069af19f11fb7484ee78e6da75979..9c739e272687937793f047315509dc1b4d1030e7 100644 (file)
@@ -283,13 +283,20 @@ Interpreter* ScriptInterpreter::interpreterForGlobalObject(const JSValue* imp)
 
 bool ScriptInterpreter::shouldInterruptScript() const
 {
-    if (Page *page = m_frame->page())
-        return page->chrome()->shouldInterruptJavaScript();
-    
-    return false;
+    Page* page = m_frame->page();
+
+    // See <rdar://problem/5479443>. We don't think that page can ever be NULL
+    // in this case, but if it is, we've gotten into a state where we may have
+    // hung the UI, with no way to ask the client whether to cancel execution. 
+    // For now, our solution is just to cancel execution no matter what, 
+    // ensuring that we never hang. We might want to consider other solutions 
+    // if we discover problems with this one.
+    ASSERT(page);
+    if (!page)
+        return true;
+
+    return page->chrome()->shouldInterruptJavaScript();
 }
-    
-//////
 
 JSValue* jsStringOrNull(const String& s)
 {
index e9ab3ec62b7693cd63780b859c9f2c10ced2ec90..f96d8d08e7073fe9aaf25a339c075a13a6d3f9bb 100644 (file)
@@ -1,3 +1,28 @@
+2007-09-17  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+
+        Fixed a hang due to an infinite script running in the window's unload 
+        event handler, which may be the cause of <rdar://problem/5479443> 
+        REGRESSION: Hang due to infinite JS recursion on close @ engadget.com 
+        (onunload-based ad)
+
+        * WebView/WebUIDelegatePrivate.h: Added FIXME.
+        
+        * WebView/WebView.h: Clarified headerdoc ambiguity about when delegate 
+        methods stop firing.
+
+        * WebView/WebView.mm:
+        (-[WebView _close]): The fix: don't nil out our delegates until after
+        detaching the FrameLoader, because the act of detaching the FrameLoader
+        might fire important delegate methods, like webViewShouldInterruptJavaScript:.
+        Don't do other tear-down either, because the unload event handler needs 
+        to run in a fully constructed page.
+        
+        This change is fairly low risk because niling out our delegates is a 
+        very recent, never-shipped feature in WebKit, so it's unlikely that any 
+        apps rely on it in a crazy way.
+
 2007-09-15  Darin Adler  <darin@apple.com>
 
         Reviewed by John Sullivan.
index 74b6866054e353890f0ea47f8737d85c4cd83240..eb08b5676405a123ea4c1b65e13648b1dc3a7b0b 100644 (file)
@@ -79,6 +79,7 @@ enum {
 - (void)webView:(WebView *)sender dragImage:(NSImage *)anImage at:(NSPoint)viewLocation offset:(NSSize)initialOffset event:(NSEvent *)event pasteboard:(NSPasteboard *)pboard source:(id)sourceObj slideBack:(BOOL)slideFlag forView:(NSView *)view;
 - (void)webView:(WebView *)sender didDrawRect:(NSRect)rect;
 - (void)webView:(WebView *)sender didScrollDocumentInFrameView:(WebFrameView *)frameView;
+// FIXME: If we ever make this method public, it should include a WebFrame parameter.
 - (BOOL)webViewShouldInterruptJavaScript:(WebView *)sender;
 - (void)webView:(WebView *)sender willPopupMenu:(NSMenu *)menu;
 - (void)webView:(WebView *)sender contextMenuItemSelected:(NSMenuItem *)item forElement:(NSDictionary *)element;
index 30b934230cd4af3c9900e86d68bcdcf8bf2081d0..6038703b96017d5fe12532905ec7654e354e6006 100644 (file)
@@ -193,8 +193,9 @@ extern NSString *WebViewProgressFinishedNotification;
 
 /*!
     @method close
-    @abstract Cancels any pending load operations. Once the receiver is closed it will no longer
-    respond to new requests or fire any more delegate methods.
+    @abstract Closes the receiver, unloading its web page and canceling any pending loads.
+    Once the receiver has closed, it will no longer respond to requests or fire delegate methods.
+    (However, the -close method itself may fire delegate methods.)
     @discussion A garbage collected application is required to call close when the receiver is no longer needed.
     The close method will be called automatically when the window or hostWindow closes and shouldCloseWithWindow returns YES.
     A non-garbage collected application can still call close, providing a convenient way to prevent receiver
index 2410e9a126ab012efd7ebc97ed84d3264fb1c8f9..a4046fa08ec2fed38057fe1785bdeddb59be109d 100644 (file)
@@ -683,10 +683,14 @@ static bool debugWidget = true;
         return;
     _private->closed = YES;
 
-    [self _removeFromAllWebViewsSet];
+    FrameLoader* mainFrameLoader = [[self mainFrame] _frameLoader];
+    if (mainFrameLoader)
+        mainFrameLoader->detachFromParent();
 
+    [self _removeFromAllWebViewsSet];
     [self setGroupName:nil];
     [self setHostWindow:nil];
+
     [self setDownloadDelegate:nil];
     [self setEditingDelegate:nil];
     [self setFrameLoadDelegate:nil];
@@ -698,10 +702,6 @@ static bool debugWidget = true;
     // To avoid leaks, call removeDragCaret in case it wasn't called after moveDragCaretToPoint.
     [self removeDragCaret];
 
-    FrameLoader* mainFrameLoader = [[self mainFrame] _frameLoader];
-    if (mainFrameLoader)
-        mainFrameLoader->detachFromParent();
-    
     // Deleteing the WebCore::Page will clear the page cache so we call destroy on 
     // all the plug-ins in the page cache to break any retain cycles.
     // See comment in HistoryItem::releaseAllPendingPageCaches() for more information.
index 80579d673ff37e8dd566335dc3e71edf4d840deb..e1b5161f637084ff89b51e0492afdcc927fcec28 100644 (file)
@@ -1,3 +1,19 @@
+2007-09-17  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+
+        Fixed a hang due to an infinite script running in the window's unload 
+        event handler, which may be the cause of <rdar://problem/5479443> 
+        REGRESSION: Hang due to infinite JS recursion on close @ engadget.com 
+        (onunload-based ad)
+        
+        Added a bunch of WebKitMac's close features, and reordered others to
+        match WebKitMac.
+
+        * WebView.cpp:
+        (WebView::close):
+        (WebView::removeDragCaret):
+
 2007-09-17  Adam Roben  <aroben@apple.com>
 
         Fix <rdar://4979801> overflow divs don't respond to keyboard scrolling (affects RSS pages)
index 0cd5beb0406ec8ac46e47e66e2105bb705320dff..15ca626c721b242dbe0d8c42955197733e253b44 100644 (file)
@@ -193,6 +193,25 @@ void WebView::close()
 
     m_didClose = true;
 
+    Frame* frame = m_page->mainFrame();
+    if (frame)
+        frame->loader()->detachFromParent();
+
+    m_page->setGroupName(String());
+    setHostWindow(0);
+
+    setDownloadDelegate(0);
+    setEditingDelegate(0);
+    setFrameLoadDelegate(0);
+    setFrameLoadDelegatePrivate(0);
+    setPolicyDelegate(0);
+    setResourceLoadDelegate(0);
+    setUIDelegate(0);
+    setFormDelegate(0);
+
+    delete m_page;
+    m_page = 0;
+
     IWebNotificationCenter* notifyCenter = WebNotificationCenter::defaultCenterInternal();
     COMPtr<IWebPreferences> prefs;
     if (SUCCEEDED(preferences(&prefs)))
@@ -207,21 +226,6 @@ void WebView::close()
         m_preferences = 0;
     }
 
-    setHostWindow(0);
-    setFrameLoadDelegate(0);
-    setFrameLoadDelegatePrivate(0);
-    setUIDelegate(0);
-    setFormDelegate(0);
-    setPolicyDelegate(0);
-
-    Frame* frame = NULL;
-    frame = m_page->mainFrame();
-    if (frame)
-        frame->loader()->detachFromParent();
-
-    delete m_page;
-    m_page = 0;
-
     deleteBackingStore();
 }