[Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
https://bugs.webkit.org/show_bug.cgi?id=116035
Reviewed by Allan Sandfeld Jensen.
Source/WebCore:
The previous fix only moved the crash location from WebKit down to QNetworkReplyHttpImpl
which expects its QNetworkAccessManager to still be alive.
Fix it by watching the QNetworkReply's destroyed() signal and avoid the dangling pointer
instead. The QNetworkReply doesn't need to be aborted in this case anyway.
* platform/network/qt/QNetworkReplyHandler.cpp:
(WebCore::QNetworkReplyWrapper::QNetworkReplyWrapper):
(WebCore::QNetworkReplyWrapper::release):
(WebCore::QNetworkReplyWrapper::stopForwarding):
Rename resetConnections to stopForwarding since not all connections are related
to data forwarding to the client anymore.
(WebCore::QNetworkReplyWrapper::receiveMetaData):
(WebCore::QNetworkReplyWrapper::replyDestroyed):
(WebCore::QNetworkReplyWrapper::didReceiveFinished):
* platform/network/qt/QNetworkReplyHandler.h:
(QNetworkReplyWrapper):
Source/WebKit/qt:
* tests/qwebpage/tst_qwebpage.cpp:
(tst_QWebPage::networkReplyParentDidntChange): Change test to match the new expectation.
(tst_QWebPage::destroyQNAMBeforeAbortDoesntCrash):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@150120 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 53f1696..c744325 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+2013-05-15 Jocelyn Turcotte <jocelyn.turcotte@digia.com>
+
+ [Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
+ https://bugs.webkit.org/show_bug.cgi?id=116035
+
+ Reviewed by Allan Sandfeld Jensen.
+
+ The previous fix only moved the crash location from WebKit down to QNetworkReplyHttpImpl
+ which expects its QNetworkAccessManager to still be alive.
+
+ Fix it by watching the QNetworkReply's destroyed() signal and avoid the dangling pointer
+ instead. The QNetworkReply doesn't need to be aborted in this case anyway.
+
+ * platform/network/qt/QNetworkReplyHandler.cpp:
+ (WebCore::QNetworkReplyWrapper::QNetworkReplyWrapper):
+ (WebCore::QNetworkReplyWrapper::release):
+ (WebCore::QNetworkReplyWrapper::stopForwarding):
+ Rename resetConnections to stopForwarding since not all connections are related
+ to data forwarding to the client anymore.
+ (WebCore::QNetworkReplyWrapper::receiveMetaData):
+ (WebCore::QNetworkReplyWrapper::replyDestroyed):
+ (WebCore::QNetworkReplyWrapper::didReceiveFinished):
+ * platform/network/qt/QNetworkReplyHandler.h:
+ (QNetworkReplyWrapper):
+
2013-05-15 Darin Adler <darin@apple.com>
[Mac] Make Clipboard::declareAndWriteDragImage non-virtual
diff --git a/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp b/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp
index 1e45f4b..a6e0840 100644
--- a/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp
+++ b/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp
@@ -269,13 +269,11 @@
{
Q_ASSERT(m_reply);
- // Allow the QNetworkReply to outlive its parent QNetworkAccessManager in case the later gets destroyed before our ResourceHandle is done with it.
- m_reply->setParent(0);
-
// setFinished() must be the first that we connect, so isFinished() is updated when running other slots.
connect(m_reply, SIGNAL(finished()), this, SLOT(setFinished()));
connect(m_reply, SIGNAL(finished()), this, SLOT(receiveMetaData()));
connect(m_reply, SIGNAL(readyRead()), this, SLOT(receiveMetaData()));
+ connect(m_reply, SIGNAL(destroyed()), this, SLOT(replyDestroyed()));
}
QNetworkReplyWrapper::~QNetworkReplyWrapper()
@@ -290,7 +288,7 @@
if (!m_reply)
return 0;
- resetConnections();
+ m_reply->disconnect(this);
QNetworkReply* reply = m_reply;
m_reply = 0;
m_sniffer = nullptr;
@@ -304,10 +302,10 @@
receiveMetaData();
}
-void QNetworkReplyWrapper::resetConnections()
+void QNetworkReplyWrapper::stopForwarding()
{
if (m_reply) {
- // Disconnect all connections except the one to setFinished() slot.
+ // Disconnect all connections that might affect the ResourceHandleClient.
m_reply->disconnect(this, SLOT(receiveMetaData()));
m_reply->disconnect(this, SLOT(didReceiveFinished()));
m_reply->disconnect(this, SLOT(didReceiveReadyRead()));
@@ -318,8 +316,7 @@
void QNetworkReplyWrapper::receiveMetaData()
{
// This slot is only used to receive the first signal from the QNetworkReply object.
- resetConnections();
-
+ stopForwarding();
WTF::String contentType = m_reply->header(QNetworkRequest::ContentTypeHeader).toString();
m_encoding = extractCharsetFromMediaType(contentType);
@@ -372,6 +369,12 @@
m_reply->setProperty("_q_isFinished", true);
}
+void QNetworkReplyWrapper::replyDestroyed()
+{
+ m_reply = 0;
+ m_sniffer = nullptr;
+}
+
void QNetworkReplyWrapper::emitMetaDataChanged()
{
QueueLocker lock(m_queue);
@@ -402,7 +405,7 @@
void QNetworkReplyWrapper::didReceiveFinished()
{
// Disconnecting will make sure that nothing will happen after emitting the finished signal.
- resetConnections();
+ stopForwarding();
m_queue->push(&QNetworkReplyHandler::finish);
}
diff --git a/Source/WebCore/platform/network/qt/QNetworkReplyHandler.h b/Source/WebCore/platform/network/qt/QNetworkReplyHandler.h
index a689041..6bc35cc 100644
--- a/Source/WebCore/platform/network/qt/QNetworkReplyHandler.h
+++ b/Source/WebCore/platform/network/qt/QNetworkReplyHandler.h
@@ -94,9 +94,10 @@
void didReceiveReadyRead();
void receiveSniffedMIMEType();
void setFinished();
+ void replyDestroyed();
private:
- void resetConnections();
+ void stopForwarding();
void emitMetaDataChanged();
QNetworkReply* m_reply;
diff --git a/Source/WebKit/qt/ChangeLog b/Source/WebKit/qt/ChangeLog
index eb59eaf..ceaf36e 100644
--- a/Source/WebKit/qt/ChangeLog
+++ b/Source/WebKit/qt/ChangeLog
@@ -1,3 +1,14 @@
+2013-05-15 Jocelyn Turcotte <jocelyn.turcotte@digia.com>
+
+ [Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
+ https://bugs.webkit.org/show_bug.cgi?id=116035
+
+ Reviewed by Allan Sandfeld Jensen.
+
+ * tests/qwebpage/tst_qwebpage.cpp:
+ (tst_QWebPage::networkReplyParentDidntChange): Change test to match the new expectation.
+ (tst_QWebPage::destroyQNAMBeforeAbortDoesntCrash):
+
2013-05-14 Jocelyn Turcotte <jocelyn.turcotte@digia.com>
[Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
diff --git a/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp b/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
index 0e2b0a2..e430898 100644
--- a/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
+++ b/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
@@ -174,7 +174,8 @@
#endif
void originatingObjectInNetworkRequests();
- void networkReplyParentChanged();
+ void networkReplyParentDidntChange();
+ void destroyQNAMBeforeAbortDoesntCrash();
void testJSPrompt();
void showModalDialog();
void testStopScheduledPageRefresh();
@@ -2848,17 +2849,30 @@
QVERIFY(qobject_cast<QWebFrame*>(networkManager->requests.at(i).originatingObject()) == childFrames.at(i));
}
-void tst_QWebPage::networkReplyParentChanged()
+void tst_QWebPage::networkReplyParentDidntChange()
{
TestNetworkManager* networkManager = new TestNetworkManager(m_page);
m_page->setNetworkAccessManager(networkManager);
networkManager->requests.clear();
- // Trigger a load and check if pending QNetworkReplies have been reparented before returning to the event loop.
+ // Trigger a load and check that pending QNetworkReplies haven't been reparented before returning to the event loop.
m_view->load(QUrl("qrc:///resources/content.html"));
QVERIFY(networkManager->requests.count() > 0);
- QVERIFY(networkManager->findChildren<QNetworkReply*>().isEmpty());
+ QVERIFY(networkManager->findChildren<QNetworkReply*>().size() > 0);
+}
+
+void tst_QWebPage::destroyQNAMBeforeAbortDoesntCrash()
+{
+ QNetworkAccessManager* networkManager = new QNetworkAccessManager;
+ m_page->setNetworkAccessManager(networkManager);
+
+ m_view->load(QUrl("qrc:///resources/content.html"));
+ delete networkManager;
+ // This simulates what PingLoader does with its QNetworkReply when it times out.
+ // PingLoader isn't attached to a QWebPage and can be kept alive
+ // for 60000 seconds (~16.7 hours) to then cancel its ResourceHandle.
+ m_view->stop();
}
/**
