Bug 42534 - Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired()
: Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimer...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P1 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-07-18 21:03 PST by
Modified: 2010-07-29 08:03 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-18 21:03:29 PST
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 From 2010-07-18 21:04:30 PST -------
I should say that the crash dump is from a valgrind run of WebKit/qt/tests/qwebframe/tst_qwebframe
------- Comment #2 From 2010-07-19 16:29:26 PST -------
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 From 2010-07-20 05:51:30 PST -------
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 From 2010-07-20 10:23:04 PST -------
This is also causing:  http://code.google.com/p/chromium/issues/detail?id=49323
------- Comment #5 From 2010-07-21 07:44:18 PST -------
*** Bug 42610 has been marked as a duplicate of this bug. ***
------- Comment #6 From 2010-07-21 08:23:51 PST -------
Created an attachment (id=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 From 2010-07-21 08:40:45 PST -------
+    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 From 2010-07-21 08:49:10 PST -------
(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 From 2010-07-21 08:57:49 PST -------
Created an attachment (id=62188) [details]
Patch

Change presenter() to m_notificationPresenter, per comment #7.
------- Comment #10 From 2010-07-21 09:01:05 PST -------
We usually add a comment and an assertion when introducing a null check that probably shouldn't be there.
------- Comment #11 From 2010-07-21 09:26:01 PST -------
Created an attachment (id=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 From 2010-07-21 09:29:26 PST -------
Created an attachment (id=62195) [details]
Patch

Attached the wrong patch :-)
------- Comment #13 From 2010-07-21 09:46:26 PST -------
> 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 From 2010-07-21 09:52:11 PST -------
(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 From 2010-07-21 09:53:01 PST -------
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 From 2010-07-21 09:54:23 PST -------
(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 From 2010-07-21 10:44:09 PST -------
Created an attachment (id=62209) [details]
Patch

Added NULL check and renamed to disconnectNotifications instead of DOMWindow::clearNotificationRequests.
------- Comment #18 From 2010-07-21 10:47:33 PST -------
(From update of attachment 62209 [details])
> +    // 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 From 2010-07-21 11:08:10 PST -------
Created an attachment (id=62212) [details]
Patch

Fix spelling error (gedit should have built-in spell checker:-))
Rename disconnectNotifications to pageDestroyed, per comment #18.
------- Comment #20 From 2010-07-21 11:38:26 PST -------
(From update of attachment 62212 [details])
Clearing flags on attachment: 62212

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