[webkit-changes] [97674] branches/chromium/874/Source/WebCore

[dimich]

Title: [97674] branches/chromium/874/Source/WebCore

Revision

97674

Author

dim...@chromium.org

Date

2011-10-17 16:51:09 -0700 (Mon, 17 Oct 2011)

Log Message

Merge 97667 - Re-landing: window.webkitNotifications uses deallocated NotificationPresenter after live Iframe transfer.
https://bugs.webkit.org/show_bug.cgi?id=70147

Reviewed by David Levin.

I only found a way to test this manually, since Chromium TestShell uses static instance
of NotificationPresenter instead of per-page one so the issue does not reproduce.
Adding manual test that works in full build of Chromium.

* manual-tests/iframe_notifications/iframe-reparenting-close-window-child.html: Added.
* manual-tests/iframe_notifications/iframe-reparenting-close-window-iframe.html: Added.
* manual-tests/iframe_notifications/iframe-reparenting-close-window.html: Added.
* notifications/NotificationCenter.cpp:
(WebCore::NotificationCenter::disconnectFrame):
* page/DOMWindow.cpp:
(WebCore::DOMWindow::resetNotifications):
* page/DOMWindow.h:
* page/Frame.cpp:
(WebCore::Frame::transferChildFrameToNewDocument): reset webkitNotifications object.

TBR=dim...@chromium.org
Review URL: http://codereview.chromium.org/8333006

Modified Paths

• branches/chromium/874/Source/WebCore/notifications/NotificationCenter.cpp
• branches/chromium/874/Source/WebCore/page/DOMWindow.cpp
• branches/chromium/874/Source/WebCore/page/DOMWindow.h
• branches/chromium/874/Source/WebCore/page/Frame.cpp

Diff

Modified: branches/chromium/874/Source/WebCore/notifications/NotificationCenter.cpp (97673 => 97674)

--- branches/chromium/874/Source/WebCore/notifications/NotificationCenter.cpp	2011-10-17 23:49:25 UTC (rev 97673)
+++ branches/chromium/874/Source/WebCore/notifications/NotificationCenter.cpp	2011-10-17 23:51:09 UTC (rev 97674)
@@ -61,9 +61,8 @@
 
 void NotificationCenter::disconnectFrame()
 {
-    // m_notificationPresenter should never be 0. But just to be safe, we check it here.
-    // Due to the mysterious bug http://code.google.com/p/chromium/issues/detail?id=49323.
-    ASSERT(m_notificationPresenter);
+    // Can be 0 if iframe was transferred into another page. In this case
+    // this method is invoked more then once.
     if (!m_notificationPresenter)
         return;
     m_notificationPresenter->cancelRequestsForPermission(scriptExecutionContext());

Modified: branches/chromium/874/Source/WebCore/page/DOMWindow.cpp (97673 => 97674)

--- branches/chromium/874/Source/WebCore/page/DOMWindow.cpp	2011-10-17 23:49:25 UTC (rev 97673)
+++ branches/chromium/874/Source/WebCore/page/DOMWindow.cpp	2011-10-17 23:51:09 UTC (rev 97674)
@@ -548,9 +548,7 @@
 #endif
 
 #if ENABLE(NOTIFICATIONS)
-    if (m_notifications)
-        m_notifications->disconnectFrame();
-    m_notifications = 0;
+    resetNotifications();
 #endif
 
 #if ENABLE(INDEXED_DATABASE)
@@ -758,6 +756,14 @@
       
     return m_notifications.get();
 }
+
+void DOMWindow::resetNotifications()
+{
+    if (!m_notifications)
+        return;
+    m_notifications->disconnectFrame();
+    m_notifications = 0;
+}
 #endif
 
 void DOMWindow::pageDestroyed()
@@ -766,9 +772,7 @@
 #if ENABLE(NOTIFICATIONS)
     // Clearing Notifications requests involves accessing the client so it must be done
     // before the frame is detached.
-    if (m_notifications)
-        m_notifications->disconnectFrame();
-    m_notifications = 0;
+    resetNotifications();
 #endif
 }
 

Modified: branches/chromium/874/Source/WebCore/page/DOMWindow.h (97673 => 97674)

--- branches/chromium/874/Source/WebCore/page/DOMWindow.h	2011-10-17 23:49:25 UTC (rev 97673)
+++ branches/chromium/874/Source/WebCore/page/DOMWindow.h	2011-10-17 23:51:09 UTC (rev 97674)
@@ -383,6 +383,9 @@
 
 #if ENABLE(NOTIFICATIONS)
         NotificationCenter* webkitNotifications() const;
+        // Renders webkitNotifications object safely inoperable, disconnects
+        // if from embedder-provided NotificationPresenter.
+        void resetNotifications();
 #endif
 
 #if ENABLE(QUOTA)

Modified: branches/chromium/874/Source/WebCore/page/Frame.cpp (97673 => 97674)

--- branches/chromium/874/Source/WebCore/page/Frame.cpp	2011-10-17 23:49:25 UTC (rev 97673)
+++ branches/chromium/874/Source/WebCore/page/Frame.cpp	2011-10-17 23:51:09 UTC (rev 97674)
@@ -747,8 +747,12 @@
         // when the Geolocation's iframe is reparented.
         // See https://bugs.webkit.org/show_bug.cgi?id=55577
         // and https://bugs.webkit.org/show_bug.cgi?id=52877
-        if (m_domWindow)
+        if (m_domWindow) {
             m_domWindow->resetGeolocation();
+#if ENABLE(NOTIFICATIONS)
+            m_domWindow->resetNotifications();
+#endif
+        }
 
 #if ENABLE(MEDIA_STREAM)
         if (m_mediaStreamFrameController)

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes