Bug 42534 - Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired()
: Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimer...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P1 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-18 21:03 PDT by Andreas Kling
Modified: 2010-07-29 08:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.21 KB, patch)
2010-07-21 08:23 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (3.22 KB, patch)
2010-07-21 08:57 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (3.22 KB, patch)
2010-07-21 09:26 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (3.40 KB, patch)
2010-07-21 09:29 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2010-07-21 10:44 PDT, Yael
darin: review-
Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2010-07-21 11:08 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-07-18 21:03:29 PDT
This crash was introduced by <http://trac.webkit.org/changeset/62939> (bug 41783)

Invalid read of size 8

0x596D1AC: WebCore::NotificationCenter::disconnectFrame() (NotificationCenter.cpp:64)
0x5995C77: WebCore::DOMWindow::clear() (DOMWindow.cpp:480)
0x59956A1: WebCore::DOMWindow::disconnectFrame() (DOMWindow.cpp:399)
0x59B989A: WebCore::Frame::~Frame() (Frame.cpp:211)
0x56F6C3E: WTF::RefCounted<WebCore::Frame>::deref() (RefCounted.h:139)
0x59BC987: WebCore::Frame::lifeSupportTimerFired(WebCore::Timer<WebCore::Frame>*) (Frame.cpp:927)
0x59C2805: WebCore::Timer<WebCore::Frame>::fired() (Timer.h:98)
0x5A4C515: WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:112)
0x7BBA588: QObject::event(QEvent*) (qobject.cpp:1175)
0x6B0B78B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (qapplication.cpp:4392)
0x6B10A3C: QApplication::notify(QObject*, QEvent*) (qapplication.cpp:3794)
0x7BA707B: QCoreApplication::notifyInternal(QObject*, QEvent*) (qcoreapplication.cpp:732)
0x7BD8B61: QTimerInfoList::activateTimers() (qcoreapplication.h:215)
0x7BD5913: timerSourceDispatch(_GSource*, int (*)(void*), void*) (qeventdispatcher_glib.cpp:184)
0x97B28C1: g_main_context_dispatch (in /lib/libglib-2.0.so.0.2400.1)
0x97B6747: ??? (in /lib/libglib-2.0.so.0.2400.1)
0x97B68FB: g_main_context_iteration (in /lib/libglib-2.0.so.0.2400.1)
0x7BD5602: QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (qeventdispatcher_glib.cpp:415)
0x6BCC89D: QGuiEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (qguieventdispatcher_glib.cpp:204)
0x7BA5D51: QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (qeventloop.cpp:149)
0x7BA613B: QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (qeventloop.cpp:201)
0x04109AA: waitForSignal(QObject*, char const*, int) (util.h:48)
0x04130D6: tst_QWebFrame::symmetricUrl() (tst_qwebframe.cpp:2198)
0x042E71A: tst_QWebFrame::qt_metacall(QMetaObject::Call, int, void**) (tst_qwebframe.moc:674)
0x7BB0665: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.cpp:1575)
0x7BB1D75: QMetaObject::invokeMethod(QObject*, char const*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) (qmetaobject.cpp:1148)
0x67102A8: QTest::qInvokeTestMethod(char const*, char const*) (qobjectdefs.h:408)
0x67110C8: QTest::qInvokeTestMethods(QObject*) (qtestcase.cpp:1507)
0x67113B4: QTest::qExec(QObject*, int, char**) (qtestcase.cpp:1716)
0x040A5A5: main (tst_qwebframe.cpp:3006)

Address 0x198ad500 is 0 bytes inside a block of size 88 free'd

0x4C27E4F: operator delete(void*) (vg_replace_malloc.c:387)
0x5BF0A4B: QWebPagePrivate::~QWebPagePrivate() (qwebpage.cpp:318)
0x5BFB0D7: QWebPage::~QWebPage() (qwebpage.cpp:1891)
0x5BFC092: QWebViewPrivate::detachCurrentPage() (qwebview.cpp:372)
0x5BFC9C2: QWebViewPrivate::~QWebViewPrivate() (qwebview.cpp:60)
0x5BFCA0E: QWebView::~QWebView() (qwebview.cpp:329)
0x0409362: tst_QWebFrame::cleanup() (tst_qwebframe.cpp:728)
0x042E5AF: tst_QWebFrame::qt_metacall(QMetaObject::Call, int, void**) (tst_qwebframe.moc:656)
0x7BB0665: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.cpp:1575)
0x670E684: QTest::invokeMethod(QObject*, char const*) (qmetaobject.h:119)
0x67102E7: QTest::qInvokeTestMethod(char const*, char const*) (qtestcase.cpp:1249)
0x67110C8: QTest::qInvokeTestMethods(QObject*) (qtestcase.cpp:1507)
0x67113B4: QTest::qExec(QObject*, int, char**) (qtestcase.cpp:1716)
0x040A5A5: main (tst_qwebframe.cpp:3006)
Comment 1 Andreas Kling 2010-07-18 21:04:30 PDT
I should say that the crash dump is from a valgrind run of WebKit/qt/tests/qwebframe/tst_qwebframe
Comment 2 Yael 2010-07-19 16:29:26 PDT
Sorry about that. 
disconnectFrame() is called after the frame was detached from everything else. I need to find a better place to cancel notifications, before the frame is detached.
Comment 3 Yael 2010-07-20 05:51:30 PDT
Kling, I applied the patch from https://bugs.webkit.org/show_bug.cgi?id=41065, but the Qt runtime tests were still crashing. After disabling the 3 tests: getSetDynamicProperty(), getSetDynamicProperty_data(), and getSetStaticProperty() I get 2 unrelated XFail, but no crash.

Can you please check if you had other code changes in your environment that I should apply to mine?
thanks,
Comment 4 John Gregg 2010-07-20 10:23:04 PDT
This is also causing:  http://code.google.com/p/chromium/issues/detail?id=49323
Comment 5 Yael 2010-07-21 07:44:18 PDT
*** Bug 42610 has been marked as a duplicate of this bug. ***
Comment 6 Yael 2010-07-21 08:23:51 PDT
Created attachment 62184 [details]
Patch

Call NotificationsCenter::disconnectFrame when a frame is detached from its page. We want to cancel any outstanding premision requests at that point.
Since I could not reproduce the crash, I did not add tests at this time, but there is a new test for canceling notifications requests in https://bugs.webkit.org/show_bug.cgi?id=41413 .
Comment 7 Alexey Proskuryakov 2010-07-21 08:40:45 PDT
+    if (!presenter())
+        return;
     m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext);

Is this null check still needed with other changes?

It would probably be better to use m_notificationPresenter consistently - it's not immediately obvious if presenter() is the same thing.
Comment 8 Yael 2010-07-21 08:49:10 PDT
(In reply to comment #7)
> +    if (!presenter())
> +        return;
>      m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext);
> 
> Is this null check still needed with other changes?
> 
I think it is safe to add this check, especially since I don't have exact steps to reproduce the error.

> It would probably be better to use m_notificationPresenter consistently - it's not immediately obvious if presenter() is the same thing.

I agree, I'll update the patch.
Comment 9 Yael 2010-07-21 08:57:49 PDT
Created attachment 62188 [details]
Patch

Change presenter() to m_notificationPresenter, per comment #7.
Comment 10 Alexey Proskuryakov 2010-07-21 09:01:05 PDT
We usually add a comment and an assertion when introducing a null check that probably shouldn't be there.
Comment 11 Yael 2010-07-21 09:26:01 PDT
Created attachment 62193 [details]
Patch

Added comment about the NULL check, per comment #10.
I am reluctant to add an ASSERT here, because if m_notificationPresenter will be NULL, Chromium will continue to crash and we will continue to get crash reports. 
I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit.
Comment 12 Yael 2010-07-21 09:29:26 PDT
Created attachment 62195 [details]
Patch

Attached the wrong patch :-)
Comment 13 Alexey Proskuryakov 2010-07-21 09:46:26 PDT
> I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit.

This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users.
Comment 14 Yael 2010-07-21 09:52:11 PDT
(In reply to comment #13)
> > I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit.
> 
> This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users.

I don't know where these crash logs come from. Maybe someone from Chromium project could tell us.
However, if you look at all other functions of NotificationCenter, there is a NULL check in each one of them, so I don't understand why it is a problem to have a NULL check in this new function as well?
Comment 15 John Gregg 2010-07-21 09:53:01 PDT
I don't really like the name DOMWindow::clearNotificationRequests.  The method does more than that, since it also sets the NotificationCenter to 0.  Perhaps 'disconnectNotifications()' would be more specific.
Comment 16 John Gregg 2010-07-21 09:54:23 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit.
> > 
> > This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users.
> 
> I don't know where these crash logs come from. Maybe someone from Chromium project could tell us.
> However, if you look at all other functions of NotificationCenter, there is a NULL check in each one of them, so I don't understand why it is a problem to have a NULL check in this new function as well?

The crash reports are from release builds, not debug builds, so I think that the ASSERT would be fine.
Comment 17 Yael 2010-07-21 10:44:09 PDT
Created attachment 62209 [details]
Patch

Added NULL check and renamed to disconnectNotifications instead of DOMWindow::clearNotificationRequests.
Comment 18 Darin Adler 2010-07-21 10:47:33 PDT
Comment on attachment 62209 [details]
Patch

> +    // Due to the misterious bug http://code.google.com/p/chromium/issues/detail?id=49323.

Spelling error here: "misterious".

> +void DOMWindow::disconnectNotifications()

This function should be named after when it's called, the event the DOMWindow is learning about, rather than what it does. I think pageDestroyed is a fine name for it. It should not be notifications-specific. The body can be, but the function itself should not.

> +#if ENABLE(NOTIFICATIONS)
> +    if (m_domWindow)
> +        m_domWindow->disconnectNotifications();
> +#endif

Then this could be:

    if (m_domWindow)
        m_domWindow->pageDestroyed();

Please make those changes.
Comment 19 Yael 2010-07-21 11:08:10 PDT
Created attachment 62212 [details]
Patch

Fix spelling error (gedit should have built-in spell checker:-))
Rename disconnectNotifications to pageDestroyed, per comment #18.
Comment 20 WebKit Commit Bot 2010-07-21 11:38:26 PDT
Comment on attachment 62212 [details]
Patch

Clearing flags on attachment: 62212

Committed r63847: <http://trac.webkit.org/changeset/63847>
Comment 21 WebKit Commit Bot 2010-07-21 11:38:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Simon Hausmann 2010-07-29 08:03:35 PDT
Revision r63847 cherry-picked into qtwebkit-2.1 with commit 348c731f9f4ab48d4cc2f8aa8f51e00c1f97a850