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)
I should say that the crash dump is from a valgrind run of WebKit/qt/tests/qwebframe/tst_qwebframe
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.
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,
This is also causing: http://code.google.com/p/chromium/issues/detail?id=49323
*** Bug 42610 has been marked as a duplicate of this bug. ***
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 .
+ 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.
(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.
Created attachment 62188 [details] Patch Change presenter() to m_notificationPresenter, per comment #7.
We usually add a comment and an assertion when introducing a null check that probably shouldn't be there.
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.
Created attachment 62195 [details] Patch Attached the wrong patch :-)
> 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.
(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?
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.
(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.
Created attachment 62209 [details] Patch Added NULL check and renamed to disconnectNotifications instead of DOMWindow::clearNotificationRequests.
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.
Created attachment 62212 [details] Patch Fix spelling error (gedit should have built-in spell checker:-)) Rename disconnectNotifications to pageDestroyed, per comment #18.
Comment on attachment 62212 [details] Patch Clearing flags on attachment: 62212 Committed r63847: <http://trac.webkit.org/changeset/63847>
All reviewed patches have been landed. Closing bug.
Revision r63847 cherry-picked into qtwebkit-2.1 with commit 348c731f9f4ab48d4cc2f8aa8f51e00c1f97a850