Crash when the page with a calendar picker is scrolled
authortkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2012 03:15:08 +0000 (03:15 +0000)
committertkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2012 03:15:08 +0000 (03:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=84287

Reviewed by Hajime Morita.

Use the same ownership model as WebPopupMenuImpl's.

* src/WebPagePopupImpl.cpp:
(WebKit::WebPagePopupImpl::close):
Clear m_widgetClient to avoid furthur access to it. deref instead of delete.
(WebKit::WebPagePopupImpl::closePopup):
Do not call closeWidgetSoon() if close() was already called.
(WebKit::WebPagePopup::create):
Add a reference. Add explanation of the ownership.
* src/WebPagePopupImpl.h:
(WebPagePopupImpl): Make this RefCounted.  Make the destuctor public.
* src/WebViewImpl.cpp:
(WebKit::WebViewImpl::WebViewImpl): No need to clear m_pagePopup explicitly.
(WebKit::WebViewImpl::openPagePopup): Need to use .get() because m_pagePopup is a RefPtr.
(WebKit::WebViewImpl::closePagePopup): ditto.
(WebKit::WebViewImpl::hidePopups): ditto.
* src/WebViewImpl.h: Make m_pagePopup a RefPtr.

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

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

index 529b2ee..2178cd5 100644 (file)
@@ -1,3 +1,28 @@
+2012-04-23  Kent Tamura  <tkent@chromium.org>
+
+        Crash when the page with a calendar picker is scrolled
+        https://bugs.webkit.org/show_bug.cgi?id=84287
+
+        Reviewed by Hajime Morita.
+
+        Use the same ownership model as WebPopupMenuImpl's.
+
+        * src/WebPagePopupImpl.cpp:
+        (WebKit::WebPagePopupImpl::close):
+        Clear m_widgetClient to avoid furthur access to it. deref instead of delete.
+        (WebKit::WebPagePopupImpl::closePopup):
+        Do not call closeWidgetSoon() if close() was already called.
+        (WebKit::WebPagePopup::create):
+        Add a reference. Add explanation of the ownership.
+        * src/WebPagePopupImpl.h:
+        (WebPagePopupImpl): Make this RefCounted.  Make the destuctor public.
+        * src/WebViewImpl.cpp:
+        (WebKit::WebViewImpl::WebViewImpl): No need to clear m_pagePopup explicitly.
+        (WebKit::WebViewImpl::openPagePopup): Need to use .get() because m_pagePopup is a RefPtr.
+        (WebKit::WebViewImpl::closePagePopup): ditto.
+        (WebKit::WebViewImpl::hidePopups): ditto.
+        * src/WebViewImpl.h: Make m_pagePopup a RefPtr.
+
 2012-04-18  James Robinson  <jamesr@chromium.org>
 
         [chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type
index 9fcaf4d..1ad417b 100644 (file)
@@ -282,7 +282,8 @@ void WebPagePopupImpl::setFocus(bool enable)
 void WebPagePopupImpl::close()
 {
     m_page.clear();
-    delete this;
+    m_widgetClient = 0;
+    deref();
 }
 
 void WebPagePopupImpl::closePopup()
@@ -292,8 +293,11 @@ void WebPagePopupImpl::closePopup()
         m_page->mainFrame()->loader()->stopAllLoaders();
         m_page->mainFrame()->loader()->stopLoading(UnloadEventPolicyNone);
     }
-    // closeWidgetSoon() will call this->close() later.
-    m_widgetClient->closeWidgetSoon();
+    // m_widgetClient might be 0 because this widget might be already closed.
+    if (m_widgetClient) {
+        // closeWidgetSoon() will call this->close() later.
+        m_widgetClient->closeWidgetSoon();
+    }
 
     m_popupClient->didClosePopup();
 }
@@ -307,7 +311,13 @@ WebPagePopup* WebPagePopup::create(WebWidgetClient* client)
 #if ENABLE(PAGE_POPUP)
     if (!client)
         CRASH();
-    return new WebPagePopupImpl(client);
+    // A WebPagePopupImpl 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
+    //    WebPagePopupImpl to close.
+    // We need them because the closing operation is asynchronous and the widget
+    // can be closed while the WebViewImpl is unaware of it.
+    return adoptRef(new WebPagePopupImpl(client)).leakRef();
 #else
     UNUSED_PARAM(client);
     return 0;
index 6f69928..2e1c8c3 100644 (file)
@@ -37,6 +37,7 @@
 #include "PageWidgetDelegate.h"
 #include "WebPagePopup.h"
 #include <wtf/OwnPtr.h>
+#include <wtf/RefCounted.h>
 
 namespace WebCore {
 class Page;
@@ -51,11 +52,13 @@ class WebViewImpl;
 
 class WebPagePopupImpl : public WebPagePopup,
                          public PageWidgetEventHandler,
-                         public WebCore::PagePopup {
+                         public WebCore::PagePopup,
+                         public RefCounted<WebPagePopupImpl> {
     WTF_MAKE_NONCOPYABLE(WebPagePopupImpl);
     WTF_MAKE_FAST_ALLOCATED;
 
 public:
+    virtual ~WebPagePopupImpl();
     bool init(WebViewImpl*, WebCore::PagePopupClient*, const WebCore::IntRect& originBoundsInRootView);
     bool handleKeyEvent(const WebCore::PlatformKeyboardEvent&);
     void closePopup();
@@ -82,7 +85,6 @@ private:
 #endif
 
     explicit WebPagePopupImpl(WebWidgetClient*);
-    virtual ~WebPagePopupImpl();
     bool initPage();
 
     WebWidgetClient* m_widgetClient;
index 3a0196e..45e9fbd 100644 (file)
@@ -372,9 +372,6 @@ WebViewImpl::WebViewImpl(WebViewClient* client)
     , m_dragOperation(WebDragOperationNone)
     , m_autofillPopupShowing(false)
     , m_autofillPopup(0)
-#if ENABLE(PAGE_POPUP)
-    , m_pagePopup(0)
-#endif
     , m_isTransparent(false)
     , m_tabsToLinks(false)
     , m_dragScrollTimer(adoptPtr(new DragScrollTimer))
@@ -1215,15 +1212,15 @@ PagePopup* WebViewImpl::openPagePopup(PagePopupClient* client, const IntRect& or
 
     if (Frame* frame = focusedWebCoreFrame())
         frame->selection()->setCaretVisible(false);
-    return m_pagePopup;
+    return m_pagePopup.get();
 }
 
 void WebViewImpl::closePagePopup(PagePopup* popup)
 {
     ASSERT(popup);
     WebPagePopupImpl* popupImpl = static_cast<WebPagePopupImpl*>(popup);
-    ASSERT(m_pagePopup == popupImpl);
-    if (m_pagePopup != popupImpl)
+    ASSERT(m_pagePopup.get() == popupImpl);
+    if (m_pagePopup.get() != popupImpl)
         return;
     m_pagePopup->closePopup();
     m_pagePopup = 0;
@@ -2797,7 +2794,7 @@ void WebViewImpl::hidePopups()
     hideAutofillPopup();
 #if ENABLE(PAGE_POPUP)
     if (m_pagePopup)
-        closePagePopup(m_pagePopup);
+        closePagePopup(m_pagePopup.get());
 #endif
 }
 
index dfb91ec..9383ba2 100644 (file)
@@ -711,7 +711,7 @@ private:
 
 #if ENABLE(PAGE_POPUP)
     // The popup associated with an input element.
-    WebPagePopupImpl* m_pagePopup;
+    RefPtr<WebPagePopupImpl> m_pagePopup;
 #endif
 
     OwnPtr<WebDevToolsAgentPrivate> m_devToolsAgent;