[Chromium] Detach the frame and destroy the page immediately upon request to closeHel...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 04:55:38 +0000 (04:55 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 04:55:38 +0000 (04:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=103947

Patch by David Dorwin <ddorwin@chromium.org> on 2012-12-04
Reviewed by Kent Tamura.

In the case that the host page is being destroyed, some of the objects
the page depends on, specifically RenderViewImpl, may be destroyed by
the time close() is called asynchronously.
The frameDetached() calling code was originally copied from
WebViewImpl::close(), but WebViewImpl does not have an asynchronous
closing mechanism like the WebWidgets.

This is the fix for http://crbug.com/160650

* src/WebHelperPluginImpl.cpp:
(WebKit::WebHelperPluginImpl::init):
(WebKit::WebHelperPluginImpl::closeHelperPlugin):
(WebKit::WebHelperPluginImpl::destoryPage):
(WebKit):
(WebKit::WebHelperPluginImpl::close):.
(WebKit::WebHelperPlugin::create):
* src/WebHelperPluginImpl.h:
(WebHelperPluginImpl):
* src/WebPagePopupImpl.cpp: Made similar changes since it follows the same model. (I think the way popups are closed has prevented the race condition from occurring for popups.)
(WebKit::WebPagePopupImpl::destoryPage):
(WebKit):
(WebKit::WebPagePopupImpl::close):
(WebKit::WebPagePopupImpl::closePopup):
* src/WebPagePopupImpl.h:
(WebPagePopupImpl):

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

Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/WebHelperPluginImpl.cpp
Source/WebKit/chromium/src/WebHelperPluginImpl.h
Source/WebKit/chromium/src/WebPagePopupImpl.cpp
Source/WebKit/chromium/src/WebPagePopupImpl.h

index 7f63759..bcf6a3a 100644 (file)
@@ -1,5 +1,38 @@
 2012-12-04  David Dorwin  <ddorwin@chromium.org>
 
+        [Chromium] Detach the frame and destroy the page immediately upon request to closeHelperPlugin().
+        https://bugs.webkit.org/show_bug.cgi?id=103947
+
+        Reviewed by Kent Tamura.
+
+        In the case that the host page is being destroyed, some of the objects
+        the page depends on, specifically RenderViewImpl, may be destroyed by
+        the time close() is called asynchronously.
+        The frameDetached() calling code was originally copied from
+        WebViewImpl::close(), but WebViewImpl does not have an asynchronous
+        closing mechanism like the WebWidgets.
+
+        This is the fix for http://crbug.com/160650
+
+        * src/WebHelperPluginImpl.cpp:
+        (WebKit::WebHelperPluginImpl::init):
+        (WebKit::WebHelperPluginImpl::closeHelperPlugin):
+        (WebKit::WebHelperPluginImpl::destoryPage):
+        (WebKit):
+        (WebKit::WebHelperPluginImpl::close):.
+        (WebKit::WebHelperPlugin::create):
+        * src/WebHelperPluginImpl.h:
+        (WebHelperPluginImpl):
+        * src/WebPagePopupImpl.cpp: Made similar changes since it follows the same model. (I think the way popups are closed has prevented the race condition from occurring for popups.)
+        (WebKit::WebPagePopupImpl::destoryPage):
+        (WebKit):
+        (WebKit::WebPagePopupImpl::close):
+        (WebKit::WebPagePopupImpl::closePopup):
+        * src/WebPagePopupImpl.h:
+        (WebPagePopupImpl):
+
+2012-12-04  David Dorwin  <ddorwin@chromium.org>
+
         [Chromium] Removed obsolete local variable from WebViewImpl.cpp.
         https://bugs.webkit.org/show_bug.cgi?id=104050
 
index 0099171..c4aef3f 100644 (file)
@@ -87,6 +87,9 @@ public:
 private:
     virtual void closeWindowSoon() OVERRIDE
     {
+        // This should never be called since the only way to close the
+        // invisible page is via closeHelperPlugin().
+        ASSERT_NOT_REACHED(); 
         m_widget->closeHelperPlugin();
     }
 
@@ -120,7 +123,6 @@ bool WebHelperPluginImpl::init(WebViewImpl* webView, const String& pluginType)
     if (!initPage(webView, pluginType))
         return false;
     m_widgetClient->show(WebNavigationPolicy());
-
     setFocus(true);
 
     return true;
@@ -133,6 +135,12 @@ void WebHelperPluginImpl::closeHelperPlugin()
         m_page->mainFrame()->loader()->stopAllLoaders();
         m_page->mainFrame()->loader()->stopLoading(UnloadEventPolicyNone);
     }
+
+    // We must destroy the page now in case the host page is being destroyed, in
+    // which case some of the objects the page depends on may have been
+    // destroyed by the time this->close() is called asynchronously.
+    destoryPage();
+
     // m_widgetClient might be 0 because this widget might be already closed.
     if (m_widgetClient) {
         // closeWidgetSoon() will call this->close() later.
@@ -201,6 +209,17 @@ bool WebHelperPluginImpl::initPage(WebKit::WebViewImpl* webView, const String& p
     return true;
 }
 
+void WebHelperPluginImpl::destoryPage()
+{
+    if (!m_page)
+        return;
+
+    if (m_page->mainFrame())
+        m_page->mainFrame()->loader()->frameDetached();
+
+    m_page.clear();
+}
+
 void WebHelperPluginImpl::setCompositorSurfaceReady()
 {
 }
@@ -225,17 +244,7 @@ void WebHelperPluginImpl::setFocus(bool enable)
 
 void WebHelperPluginImpl::close()
 {
-    RefPtr<WebFrameImpl> mainFrameImpl;
-
-    if (m_page) {
-        // Initiate shutdown. This will cause a lot of notifications to be sent.
-        if (m_page->mainFrame()) {
-            mainFrameImpl = WebFrameImpl::fromFrame(m_page->mainFrame());
-            m_page->mainFrame()->loader()->frameDetached();
-        }
-        m_page.clear();
-    }
-
+    ASSERT(!m_page); // Should only be called via closePopup().
     m_widgetClient = 0;
     deref();
 }
@@ -248,10 +257,10 @@ WebHelperPlugin* WebHelperPlugin::create(WebWidgetClient* client)
         CRASH();
     // A WebHelperPluginImpl instance usually has two references.
     //  - One owned by the instance itself. It represents the visible widget.
-    //  - One owned by a WebViewImpl. It's released when the WebViewImpl ask the
-    //    WebHelperPluginImpl to close.
+    //  - One owned by the hosting element. It's released when the hosting
+    //    element asks the WebHelperPluginImpl to close.
     // We need them because the closing operation is asynchronous and the widget
-    // can be closed while the WebViewImpl is unaware of it.
+    // can be closed while the hosting element is unaware of it.
     return adoptRef(new WebHelperPluginImpl(client)).leakRef();
 }
 
index 2eb28ec..f9b5aae 100644 (file)
@@ -64,6 +64,7 @@ public:
 private:
     explicit WebHelperPluginImpl(WebWidgetClient*);
     bool initPage(WebKit::WebViewImpl*, const String& pluginType);
+    void destoryPage();
 
     // WebWidget methods:
     virtual void setCompositorSurfaceReady() OVERRIDE;
index c4b934a..61aee41 100644 (file)
@@ -225,6 +225,17 @@ bool WebPagePopupImpl::initPage()
     return true;
 }
 
+void WebPagePopupImpl::destoryPage()
+{
+    if (!m_page)
+        return;
+
+    if (m_page->mainFrame())
+        m_page->mainFrame()->loader()->frameDetached();
+
+    m_page.clear();
+}
+
 WebSize WebPagePopupImpl::size()
 {
     return m_popupClient->contentSize();
@@ -313,9 +324,7 @@ void WebPagePopupImpl::setFocus(bool enable)
 void WebPagePopupImpl::close()
 {
     m_closing = true;
-    if (m_page && m_page->mainFrame())
-        m_page->mainFrame()->loader()->frameDetached();
-    m_page.clear();
+    destoryPage(); // In case closePopup() was not called.
     m_widgetClient = 0;
     deref();
 }
@@ -329,6 +338,9 @@ void WebPagePopupImpl::closePopup()
         DOMWindowPagePopup::uninstall(m_page->mainFrame()->document()->domWindow());
     }
     m_closing = true;
+
+    destoryPage();
+
     // m_widgetClient might be 0 because this widget might be already closed.
     if (m_widgetClient) {
         // closeWidgetSoon() will call this->close() later.
index 34b4794..2576707 100644 (file)
@@ -87,6 +87,7 @@ private:
 
     explicit WebPagePopupImpl(WebWidgetClient*);
     bool initPage();
+    void destoryPage();
 
     WebWidgetClient* m_widgetClient;
     WebRect m_windowRectInScreen;