2010-07-21 Yael Aharon <yael.aharon@nokia.com>

Yael Aharon · Yael Aharon · commit 6809dbfa131b · 2010-07-21T18:38:12.000Z
Reviewed by Darin Adler. Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired() https://bugs.webkit.org/show_bug.cgi?id=42534 Call NotificationsCenter::disconnectFrame() when the frame is disconnected from the page. Calling it from the destructor of Frame is too late and sometimes causes access violation. I was not able to reproduce this crash, so did not add new tests. This patch is based on the error reported in http://code.google.com/p/chromium/issues/detail?id=49323. * page/DOMWindow.cpp: (WebCore::DOMWindow::pageDestroyed): * page/DOMWindow.h: * page/Frame.cpp: (WebCore::Frame::pageDestroyed): Canonical link: https://commits.webkit.org/54684@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@63847 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2010-07-21  Yael Aharon  <yael.aharon@nokia.com>
+
+        Reviewed by Darin Adler.
+
+        Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired()
+        https://bugs.webkit.org/show_bug.cgi?id=42534
+
+        Call NotificationsCenter::disconnectFrame() when the frame is disconnected from the page.
+        Calling it from the destructor of Frame is too late and sometimes causes access violation.
+        I was not able to reproduce this crash, so did not add new tests.
+        This patch is based on the error reported in 
+        http://code.google.com/p/chromium/issues/detail?id=49323.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::pageDestroyed):
+        * page/DOMWindow.h:
+        * page/Frame.cpp:
+        (WebCore::Frame::pageDestroyed):
+
 2010-07-21  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Sam Weinig.
diff --git a/WebCore/notifications/NotificationCenter.cpp b/WebCore/notifications/NotificationCenter.cpp
@@ -61,6 +61,11 @@ void NotificationCenter::requestPermission(PassRefPtr<VoidCallback> callback)
 
 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);
+    if (!m_notificationPresenter)
+        return;
     m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext);
     m_notificationPresenter = 0;
 }
diff --git a/WebCore/page/DOMWindow.cpp b/WebCore/page/DOMWindow.cpp
@@ -673,6 +673,17 @@ NotificationCenter* DOMWindow::webkitNotifications() const
 }
 #endif
 
+void DOMWindow::pageDestroyed()
+{
+#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;
+#endif
+}
+
 #if ENABLE(INDEXED_DATABASE)
 IndexedDatabaseRequest* DOMWindow::indexedDB() const
 {
diff --git a/WebCore/page/DOMWindow.h b/WebCore/page/DOMWindow.h
@@ -228,6 +228,8 @@ namespace WebCore {
         NotificationCenter* webkitNotifications() const;
 #endif
 
+        void pageDestroyed();
+
 #if ENABLE(INDEXED_DATABASE)
         IndexedDatabaseRequest* indexedDB() const;
 #endif
diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
@@ -1347,6 +1347,9 @@ void Frame::pageDestroyed()
     if (Frame* parent = tree()->parent())
         parent->loader()->checkLoadComplete();
 
+    if (m_domWindow)
+        m_domWindow->pageDestroyed();
+
     // FIXME: It's unclear as to why this is called more than once, but it is,
     // so page() could be NULL.
     if (page() && page()->focusController()->focusedFrame() == this)

Original file line number	Diff line number	Diff line change
`@@ -61,6 +61,11 @@ void NotificationCenter::requestPermission(PassRefPtr<VoidCallback> callback)`
`61`	`61`
`62`	`62`	`void NotificationCenter::disconnectFrame()`
`63`	`63`	`{`
	`64`	`+ // m_notificationPresenter should never be 0. But just to be safe, we check it here.`
	`65`	`+ // Due to the mysterious bug http://code.google.com/p/chromium/issues/detail?id=49323.`
	`66`	`+ ASSERT(m_notificationPresenter);`
	`67`	`+ if (!m_notificationPresenter)`
	`68`	`+ return;`
`64`	`69`	`m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext);`
`65`	`70`	`m_notificationPresenter = 0;`
`66`	`71`	`}`