Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired()

[Andreas Kling]

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)

[Andreas Kling]

I should say that the crash dump is from a valgrind run of WebKit/qt/tests/qwebframe/tst_qwebframe

[Yael]

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.

[Yael]

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,

[John Gregg]

This is also causing:  http://code.google.com/p/chromium/issues/detail?id=49323

[Yael]

*** Bug 42610 has been marked as a duplicate of this bug. ***

[Yael]

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 .

[Alexey Proskuryakov]

+    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.

[Yael]

(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.

[Yael]

Created attachment 62188 [details]
Patch

Change presenter() to m_notificationPresenter, per comment #7.

[Alexey Proskuryakov]

We usually add a comment and an assertion when introducing a null check that probably shouldn't be there.

[Yael]

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.

[Yael]

Created attachment 62195 [details]
Patch

Attached the wrong patch :-)

[Alexey Proskuryakov]

> 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.

[Yael]

(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?

[John Gregg]

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.

[John Gregg]

(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.

[Yael]

Created attachment 62209 [details]
Patch

Added NULL check and renamed to disconnectNotifications instead of DOMWindow::clearNotificationRequests.

[Darin Adler]

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.

[Yael]

Created attachment 62212 [details]
Patch

Fix spelling error (gedit should have built-in spell checker:-))
Rename disconnectNotifications to pageDestroyed, per comment #18.

[WebKit Commit Bot]

Comment on attachment 62212 [details]
Patch

Clearing flags on attachment: 62212

Committed r63847: <http://trac.webkit.org/changeset/63847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r63847 cherry-picked into qtwebkit-2.1 with commit 348c731f9f4ab48d4cc2f8aa8f51e00c1f97a850