⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 150120 in webkit


Ignore:
Timestamp:
May 15, 2013, 9:31:13 AM (13 years ago)
Author:
jocelyn.turcotte@digia.com
Message:

[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):

Location:
trunk/Source
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r150119 r150120  
     12013-05-15  Jocelyn Turcotte  <jocelyn.turcotte@digia.com>
     2
     3        [Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
     4        https://bugs.webkit.org/show_bug.cgi?id=116035
     5
     6        Reviewed by Allan Sandfeld Jensen.
     7
     8        The previous fix only moved the crash location from WebKit down to QNetworkReplyHttpImpl
     9        which expects its QNetworkAccessManager to still be alive.
     10
     11        Fix it by watching the QNetworkReply's destroyed() signal and avoid the dangling pointer
     12        instead. The QNetworkReply doesn't need to be aborted in this case anyway.
     13
     14        * platform/network/qt/QNetworkReplyHandler.cpp:
     15        (WebCore::QNetworkReplyWrapper::QNetworkReplyWrapper):
     16        (WebCore::QNetworkReplyWrapper::release):
     17        (WebCore::QNetworkReplyWrapper::stopForwarding):
     18          Rename resetConnections to stopForwarding since not all connections are related
     19          to data forwarding to the client anymore.
     20        (WebCore::QNetworkReplyWrapper::receiveMetaData):
     21        (WebCore::QNetworkReplyWrapper::replyDestroyed):
     22        (WebCore::QNetworkReplyWrapper::didReceiveFinished):
     23        * platform/network/qt/QNetworkReplyHandler.h:
     24        (QNetworkReplyWrapper):
     25
    1262013-05-15  Darin Adler  <darin@apple.com>
    227

  • trunk/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp

    r150057 r150120  
    270270    Q_ASSERT(m_reply);
    271271
    272     // Allow the QNetworkReply to outlive its parent QNetworkAccessManager in case the later gets destroyed before our ResourceHandle is done with it.
    273     m_reply->setParent(0);
    274 
    275272    // setFinished() must be the first that we connect, so isFinished() is updated when running other slots.
    276273    connect(m_reply, SIGNAL(finished()), this, SLOT(setFinished()));
    277274    connect(m_reply, SIGNAL(finished()), this, SLOT(receiveMetaData()));
    278275    connect(m_reply, SIGNAL(readyRead()), this, SLOT(receiveMetaData()));
     276    connect(m_reply, SIGNAL(destroyed()), this, SLOT(replyDestroyed()));
    279277}
    280278
    … …  
    291289        return 0;
    292290
    293     resetConnections();
     291    m_reply->disconnect(this);
    294292    QNetworkReply* reply = m_reply;
    295293    m_reply = 0;
    … …  
    305303}
    306304
    307 void QNetworkReplyWrapper::resetConnections()
     305void QNetworkReplyWrapper::stopForwarding()
    308306{
    309307    if (m_reply) {
    310         // Disconnect all connections except the one to setFinished() slot.
     308        // Disconnect all connections that might affect the ResourceHandleClient.
    311309        m_reply->disconnect(this, SLOT(receiveMetaData()));
    312310        m_reply->disconnect(this, SLOT(didReceiveFinished()));
    … …  
    319317{
    320318    // This slot is only used to receive the first signal from the QNetworkReply object.
    321     resetConnections();
    322 
     319    stopForwarding();
    323320
    324321    WTF::String contentType = m_reply->header(QNetworkRequest::ContentTypeHeader).toString();
    … …  
    373370}
    374371
     372void QNetworkReplyWrapper::replyDestroyed()
     373{
     374    m_reply = 0;
     375    m_sniffer = nullptr;
     376}
     377
    375378void QNetworkReplyWrapper::emitMetaDataChanged()
    376379{
    … …  
    403406{
    404407    // Disconnecting will make sure that nothing will happen after emitting the finished signal.
    405     resetConnections();
     408    stopForwarding();
    406409    m_queue->push(&QNetworkReplyHandler::finish);
    407410}

  • trunk/Source/WebCore/platform/network/qt/QNetworkReplyHandler.h

    r134243 r150120  
    9595    void receiveSniffedMIMEType();
    9696    void setFinished();
    97 
    98 private:
    99     void resetConnections();
     97    void replyDestroyed();
     98
     99private:
     100    void stopForwarding();
    100101    void emitMetaDataChanged();
    101102

  • trunk/Source/WebKit/qt/ChangeLog

    r150057 r150120  
     12013-05-15  Jocelyn Turcotte  <jocelyn.turcotte@digia.com>
     2
     3        [Qt] Fix a crash under ~PingLoader when the QNAM on the page has been destroyed.
     4        https://bugs.webkit.org/show_bug.cgi?id=116035
     5
     6        Reviewed by Allan Sandfeld Jensen.
     7
     8        * tests/qwebpage/tst_qwebpage.cpp:
     9        (tst_QWebPage::networkReplyParentDidntChange): Change test to match the new expectation.
     10        (tst_QWebPage::destroyQNAMBeforeAbortDoesntCrash):
     11
    1122013-05-14  Jocelyn Turcotte  <jocelyn.turcotte@digia.com>
    213

  • trunk/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp

    r150057 r150120  
    175175
    176176    void originatingObjectInNetworkRequests();
    177     void networkReplyParentChanged();
     177    void networkReplyParentDidntChange();
     178    void destroyQNAMBeforeAbortDoesntCrash();
    178179    void testJSPrompt();
    179180    void showModalDialog();
    … …  
    28492850}
    28502851
    2851 void tst_QWebPage::networkReplyParentChanged()
     2852void tst_QWebPage::networkReplyParentDidntChange()
    28522853{
    28532854    TestNetworkManager* networkManager = new TestNetworkManager(m_page);
    … …  
    28552856    networkManager->requests.clear();
    28562857
    2857     // Trigger a load and check if pending QNetworkReplies have been reparented before returning to the event loop.
     2858    // Trigger a load and check that pending QNetworkReplies haven't been reparented before returning to the event loop.
    28582859    m_view->load(QUrl("qrc:///resources/content.html"));
    28592860
    28602861    QVERIFY(networkManager->requests.count() > 0);
    2861     QVERIFY(networkManager->findChildren<QNetworkReply*>().isEmpty());
     2862    QVERIFY(networkManager->findChildren<QNetworkReply*>().size() > 0);
     2863}
     2864
     2865void tst_QWebPage::destroyQNAMBeforeAbortDoesntCrash()
     2866{
     2867    QNetworkAccessManager* networkManager = new QNetworkAccessManager;
     2868    m_page->setNetworkAccessManager(networkManager);
     2869
     2870    m_view->load(QUrl("qrc:///resources/content.html"));
     2871    delete networkManager;
     2872    // This simulates what PingLoader does with its QNetworkReply when it times out.
     2873    // PingLoader isn't attached to a QWebPage and can be kept alive
     2874    // for 60000 seconds (~16.7 hours) to then cancel its ResourceHandle.
     2875    m_view->stop();
    28622876}
    28632877
Note: See TracChangeset for help on using the changeset viewer.