Changeset 91855 in webkit


Ignore:
Timestamp:
Jul 27, 2011 10:40:57 AM (13 years ago)
Author:
caio.oliveira@openbossa.org
Message:

[Qt] QtWebkit never finishes loading sites when they are loaded after an initial QUrl fails to load.
https://bugs.webkit.org/show_bug.cgi?id=61328

Reviewed by Andreas Kling.

Change the hooks in FrameLoaderClient we use for emitting signals. Instead of
emitting signals in the progress notification functions, we use the
dispatchDid{Start,Finish,Fail}* functions. The main reason behind this change is
that loading code is prepared to handle load() when inside those functions.

The crash was being caused by setUrl() (and load()) being called when
loadFinished(false) was emitted. The problem here is that when
postProgressFinishedNotification the FrameLoader wasn't ready for taking a load()
call again, because it was still the ProvisionalLoadState but with the
provisionalDocumentLoader already removed.

To emulate the same behavior that QtWebKit had when using
postProgressFinishedNotification, we now keep track of the frame originating the
load, and emit the signals when this frame's client is called.

The patch keeps the existing semantics for QWebPage signals, but we now emit the
QWebFrame signals everytime, not only when they are the originating frame for
loading.

  • Api/qwebframe.cpp:

(clearCoreFrame): Document our assumption that activeDocumentLoader will exist.

  • WebCoreSupport/FrameLoaderClientQt.h: Remove m_loadError, add a boolean to keep

track whether the frame is originating the load. Remove the signals from
FrameLoaderClientQt since we will emit QWebFrame and QWebPage signals directly.

  • WebCoreSupport/FrameLoaderClientQt.cpp:

(WebCore::FrameLoaderClientQt::FrameLoaderClientQt): Initialize m_isOriginatingLoad.

(WebCore::FrameLoaderClientQt::setFrame): Do not connect QWebFrame and QWebPage
signals to our signals for load/finished, signal emission will be done manually.

(WebCore::FrameLoaderClientQt::dispatchDidStartProvisionalLoad): Emit
loadStarted() signal and make the first notification of estimation change, that
Qt API tests expect to exist and notify 10%.

(WebCore::FrameLoaderClientQt::dispatchDidFinishLoad): Remove reference to
m_loadError and emit loadFinished() signal.

(WebCore::FrameLoaderClientQt::postProgressStartedNotification): Remove signal
emission and mark the originating load as true, since only the originating frame
gets this call in its client.

(WebCore::FrameLoaderClientQt::postProgressFinishedNotification): Remove signal
emission.

(WebCore::FrameLoaderClientQt::callErrorPageExtension): Return whether the call
was successful or not. This wasn't necessary before because a successful call for
error page would lead to a load(), that cleared the m_loadError.
(WebCore::FrameLoaderClientQt::dispatchDidFailProvisionalLoad): Remove reference
to m_loadError and emit finished signal indicating error if ErrorPage extension
doesn't handle it.
(WebCore::FrameLoaderClientQt::dispatchDidFailLoad): Ditto.

(WebCore::FrameLoaderClientQt::emitLoadStarted): Emit the loadStarted() signal
for the QWebFrame, and if the originating load also do for the QWebPage.

(WebCore::FrameLoaderClientQt::emitLoadFinished): Same as before but for
loadFinished(). Take care to reset the originating load flag before the signals
are emitted, since they might want to set it back again.

  • tests/qwebframe/tst_qwebframe.cpp:

(URLSetter::URLSetter): Object that sets the url using either load() or setUrl()
when a certain signal is emitted in the frame.

(URLSetter::execute):
(tst_QWebFrame::loadInSignalHandlers_data):
(tst_QWebFrame::loadInSignalHandlers): New test inspired by the bug test case. This test
crashes without this patch applied.

Location:
trunk/Source/WebKit/qt
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/qt/Api/qwebframe.cpp

    r88534 r91855  
    803803static inline void clearCoreFrame(WebCore::Frame* frame)
    804804{
    805     frame->loader()->activeDocumentLoader()->writer()->begin();
    806     frame->loader()->activeDocumentLoader()->writer()->end();
     805    WebCore::DocumentLoader* documentLoader = frame->loader()->activeDocumentLoader();
     806    Q_ASSERT(documentLoader);
     807    documentLoader->writer()->begin();
     808    documentLoader->writer()->end();
    807809}
    808810
  • trunk/Source/WebKit/qt/ChangeLog

    r91845 r91855  
     12011-07-27  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
     2
     3        [Qt] QtWebkit never finishes loading sites when they are loaded after an initial QUrl fails to load.
     4        https://bugs.webkit.org/show_bug.cgi?id=61328
     5
     6        Reviewed by Andreas Kling.
     7
     8        Change the hooks in FrameLoaderClient we use for emitting signals. Instead of
     9        emitting signals in the progress notification functions, we use the
     10        dispatchDid{Start,Finish,Fail}* functions. The main reason behind this change is
     11        that loading code is prepared to handle load() when inside those functions.
     12
     13        The crash was being caused by setUrl() (and load()) being called when
     14        loadFinished(false) was emitted. The problem here is that when
     15        postProgressFinishedNotification the FrameLoader wasn't ready for taking a load()
     16        call again, because it was still the ProvisionalLoadState but with the
     17        provisionalDocumentLoader already removed.
     18
     19        To emulate the same behavior that QtWebKit had when using
     20        postProgressFinishedNotification, we now keep track of the frame originating the
     21        load, and emit the signals when this frame's client is called.
     22
     23        The patch keeps the existing semantics for QWebPage signals, but we now emit the
     24        QWebFrame signals everytime, not only when they are the originating frame for
     25        loading.
     26
     27        * Api/qwebframe.cpp:
     28        (clearCoreFrame): Document our assumption that activeDocumentLoader will exist.
     29
     30        * WebCoreSupport/FrameLoaderClientQt.h: Remove m_loadError, add a boolean to keep
     31        track whether the frame is originating the load. Remove the signals from
     32        FrameLoaderClientQt since we will emit QWebFrame and QWebPage signals directly.
     33
     34        * WebCoreSupport/FrameLoaderClientQt.cpp:
     35        (WebCore::FrameLoaderClientQt::FrameLoaderClientQt): Initialize m_isOriginatingLoad.
     36
     37        (WebCore::FrameLoaderClientQt::setFrame): Do not connect QWebFrame and QWebPage
     38        signals to our signals for load/finished, signal emission will be done manually.
     39
     40        (WebCore::FrameLoaderClientQt::dispatchDidStartProvisionalLoad): Emit
     41        loadStarted() signal and make the first notification of estimation change, that
     42        Qt API tests expect to exist and notify 10%.
     43
     44        (WebCore::FrameLoaderClientQt::dispatchDidFinishLoad): Remove reference to
     45        m_loadError and emit loadFinished() signal.
     46
     47        (WebCore::FrameLoaderClientQt::postProgressStartedNotification): Remove signal
     48        emission and mark the originating load as true, since only the originating frame
     49        gets this call in its client.
     50
     51        (WebCore::FrameLoaderClientQt::postProgressFinishedNotification): Remove signal
     52        emission.
     53
     54        (WebCore::FrameLoaderClientQt::callErrorPageExtension): Return whether the call
     55        was successful or not. This wasn't necessary before because a successful call for
     56        error page would lead to a load(), that cleared the m_loadError.
     57        (WebCore::FrameLoaderClientQt::dispatchDidFailProvisionalLoad): Remove reference
     58        to m_loadError and emit finished signal indicating error if ErrorPage extension
     59        doesn't handle it.
     60        (WebCore::FrameLoaderClientQt::dispatchDidFailLoad): Ditto.
     61
     62        (WebCore::FrameLoaderClientQt::emitLoadStarted): Emit the loadStarted() signal
     63        for the QWebFrame, and if the originating load also do for the QWebPage.
     64
     65        (WebCore::FrameLoaderClientQt::emitLoadFinished): Same as before but for
     66        loadFinished(). Take care to reset the originating load flag before the signals
     67        are emitted, since they might want to set it back again.
     68
     69        * tests/qwebframe/tst_qwebframe.cpp:
     70        (URLSetter::URLSetter): Object that sets the url using either load() or setUrl()
     71        when a certain signal is emitted in the frame.
     72
     73        (URLSetter::execute):
     74        (tst_QWebFrame::loadInSignalHandlers_data):
     75        (tst_QWebFrame::loadInSignalHandlers): New test inspired by the bug test case. This test
     76        crashes without this patch applied.
     77
    1782011-07-27  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
    279
  • trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp

    r91797 r91855  
    211211    , m_hasSentResponseToPlugin(false)
    212212    , m_hasRepresentation(false)
    213     , m_loadError(ResourceError())
     213    , m_isOriginatingLoad(false)
    214214{
    215215}
     
    230230    }
    231231
    232     connect(this, SIGNAL(loadStarted()),
    233             m_webFrame->page(), SIGNAL(loadStarted()));
    234     connect(this, SIGNAL(loadStarted()),
    235             m_webFrame, SIGNAL(loadStarted()));
    236232    connect(this, SIGNAL(loadProgress(int)),
    237233            m_webFrame->page(), SIGNAL(loadProgress(int)));
    238     connect(this, SIGNAL(loadFinished(bool)),
    239             m_webFrame->page(), SIGNAL(loadFinished(bool)));
    240234
    241235    // FIXME: The queued connection here is needed because of a problem with QNetworkAccessManager.
     
    244238            m_webFrame->page(), SIGNAL(unsupportedContent(QNetworkReply*)), Qt::QueuedConnection);
    245239
    246     connect(this, SIGNAL(loadFinished(bool)),
    247             m_webFrame, SIGNAL(loadFinished(bool)));
    248240    connect(this, SIGNAL(titleChanged(QString)),
    249241            m_webFrame, SIGNAL(titleChanged(QString)));
     
    455447    m_lastRequestedUrl = m_frame->loader()->activeDocumentLoader()->requestURL();
    456448
    457     if (m_webFrame)
    458         emit m_webFrame->provisionalLoad();
     449    if (!m_webFrame)
     450        return;
     451    emitLoadStarted();
     452    postProgressEstimateChangedNotification();
     453    emit m_webFrame->provisionalLoad();
    459454}
    460455
     
    533528        printf("%s - didFinishLoadForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
    534529
    535     // Clears the previous error.
    536     m_loadError = ResourceError();
    537 
    538530    if (!m_webFrame)
    539531        return;
     532
    540533    m_webFrame->page()->d->updateNavigationActions();
     534    emitLoadFinished(true);
    541535}
    542536
     
    586580void FrameLoaderClientQt::postProgressStartedNotification()
    587581{
    588     if (m_webFrame && m_frame->page()) {
    589         // As a new load have started, clear the previous error.
    590         m_loadError = ResourceError();
    591         emit loadStarted();
    592         postProgressEstimateChangedNotification();
    593     }
     582    if (m_webFrame && m_frame->page())
     583        m_isOriginatingLoad = true;
    594584    if (m_frame->tree()->parent() || !m_webFrame)
    595585        return;
     
    618608        }
    619609    }
    620 
    621     if (m_webFrame && m_frame->page())
    622         emit loadFinished(m_loadError.isNull());
    623610}
    624611
     
    11391126}
    11401127
    1141 void FrameLoaderClientQt::callErrorPageExtension(const WebCore::ResourceError& error)
     1128bool FrameLoaderClientQt::callErrorPageExtension(const WebCore::ResourceError& error)
    11421129{
    11431130    QWebPage* page = m_webFrame->page();
    1144     if (page->supportsExtension(QWebPage::ErrorPageExtension)) {
    1145         QWebPage::ErrorPageExtensionOption option;
    1146 
    1147         if (error.domain() == "QtNetwork")
    1148             option.domain = QWebPage::QtNetwork;
    1149         else if (error.domain() == "HTTP")
    1150             option.domain = QWebPage::Http;
    1151         else if (error.domain() == "WebKit")
    1152             option.domain = QWebPage::WebKit;
    1153         else
    1154             return;
    1155 
    1156         option.url = QUrl(error.failingURL());
    1157         option.frame = m_webFrame;
    1158         option.error = error.errorCode();
    1159         option.errorString = error.localizedDescription();
    1160 
    1161         QWebPage::ErrorPageExtensionReturn output;
    1162         if (!page->extension(QWebPage::ErrorPageExtension, &option, &output))
    1163             return;
    1164 
    1165         KURL baseUrl(output.baseUrl);
    1166         KURL failingUrl(option.url);
    1167 
    1168         WebCore::ResourceRequest request(baseUrl);
    1169         WTF::RefPtr<WebCore::SharedBuffer> buffer = WebCore::SharedBuffer::create(output.content.constData(), output.content.length());
    1170         WebCore::SubstituteData substituteData(buffer, output.contentType, output.encoding, failingUrl);
    1171         m_frame->loader()->load(request, substituteData, false);
    1172     }
     1131    if (!page->supportsExtension(QWebPage::ErrorPageExtension))
     1132        return false;
     1133
     1134    QWebPage::ErrorPageExtensionOption option;
     1135    if (error.domain() == "QtNetwork")
     1136        option.domain = QWebPage::QtNetwork;
     1137    else if (error.domain() == "HTTP")
     1138        option.domain = QWebPage::Http;
     1139    else if (error.domain() == "WebKit")
     1140        option.domain = QWebPage::WebKit;
     1141    else
     1142        return false;
     1143
     1144    option.url = QUrl(error.failingURL());
     1145    option.frame = m_webFrame;
     1146    option.error = error.errorCode();
     1147    option.errorString = error.localizedDescription();
     1148
     1149    QWebPage::ErrorPageExtensionReturn output;
     1150    if (!page->extension(QWebPage::ErrorPageExtension, &option, &output))
     1151        return false;
     1152
     1153    KURL baseUrl(output.baseUrl);
     1154    KURL failingUrl(option.url);
     1155
     1156    WebCore::ResourceRequest request(baseUrl);
     1157    WTF::RefPtr<WebCore::SharedBuffer> buffer = WebCore::SharedBuffer::create(output.content.constData(), output.content.length());
     1158    WebCore::SubstituteData substituteData(buffer, output.contentType, output.encoding, failingUrl);
     1159    m_frame->loader()->load(request, substituteData, false);
     1160    return true;
    11731161}
    11741162
     
    11781166        printf("%s - didFailProvisionalLoadWithError\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
    11791167
    1180     m_loadError = error;
    1181     if (!error.isNull() && !error.isCancellation())
    1182         callErrorPageExtension(error);
     1168    if (!error.isNull() && !error.isCancellation()) {
     1169        if (callErrorPageExtension(error))
     1170            return;
     1171    }
     1172
     1173    if (m_webFrame)
     1174        emitLoadFinished(false);
    11831175}
    11841176
     
    11881180        printf("%s - didFailLoadWithError\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
    11891181
    1190     m_loadError = error;
    1191     if (!error.isNull() && !error.isCancellation())
    1192         callErrorPageExtension(error);
     1182    if (!error.isNull() && !error.isCancellation()) {
     1183        if (callErrorPageExtension(error))
     1184            return;
     1185    }
     1186
     1187    if (m_webFrame)
     1188        emitLoadFinished(false);
    11931189}
    11941190
     
    16891685}
    16901686
     1687void FrameLoaderClientQt::emitLoadStarted()
     1688{
     1689    QWebPage* webPage = m_webFrame->page();
     1690    if (m_isOriginatingLoad && webPage)
     1691        emit webPage->loadStarted();
     1692    emit m_webFrame->loadStarted();
     1693}
     1694
     1695void FrameLoaderClientQt::emitLoadFinished(bool ok)
     1696{
     1697    // Signal handlers can lead to a new load, that will use the member again.
     1698    const bool wasOriginatingLoad = m_isOriginatingLoad;
     1699    m_isOriginatingLoad = false;
     1700
     1701    QWebPage* webPage = m_webFrame->page();
     1702    if (wasOriginatingLoad && webPage)
     1703        emit webPage->loadFinished(ok);
     1704    emit m_webFrame->loadFinished(ok);
     1705}
     1706
    16911707}
    16921708
  • trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h

    r91797 r91855  
    6868    friend class ::QWebFrame;
    6969    void callPolicyFunction(FramePolicyFunction function, PolicyAction action);
    70     void callErrorPageExtension(const ResourceError&);
     70    bool callErrorPageExtension(const ResourceError&);
     71
    7172signals:
    72     void loadStarted();
    7373    void loadProgress(int d);
    74     void loadFinished(bool);
    7574    void titleChanged(const QString& title);
    7675    void unsupportedContent(QNetworkReply*);
     
    266265
    267266private:
     267    void emitLoadStarted();
     268    void emitLoadFinished(bool ok);
     269
    268270    Frame *m_frame;
    269271    QWebFrame *m_webFrame;
     
    280282
    281283    KURL m_lastRequestedUrl;
    282     ResourceError m_loadError;
     284    bool m_isOriginatingLoad;
    283285};
    284286
  • trunk/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp

    r91669 r91855  
    661661    void setUrlThenLoads();
    662662    void loadFinishedAfterNotFoundError();
     663    void loadInSignalHandlers_data();
     664    void loadInSignalHandlers();
    663665
    664666private:
     
    37043706}
    37053707
     3708class URLSetter : public QObject {
     3709    Q_OBJECT
     3710
     3711public:
     3712    enum Signal {
     3713        LoadStarted,
     3714        LoadFinished,
     3715        ProvisionalLoad
     3716    };
     3717
     3718    enum Type {
     3719        UseLoad,
     3720        UseSetUrl
     3721    };
     3722
     3723    URLSetter(QWebFrame*, Signal, Type, const QUrl&);
     3724
     3725public slots:
     3726    void execute();
     3727
     3728signals:
     3729    void finished();
     3730
     3731private:
     3732    QWebFrame* m_frame;
     3733    QUrl m_url;
     3734    Type m_type;
     3735};
     3736
     3737Q_DECLARE_METATYPE(URLSetter::Signal)
     3738Q_DECLARE_METATYPE(URLSetter::Type)
     3739
     3740URLSetter::URLSetter(QWebFrame* frame, Signal signal, URLSetter::Type type, const QUrl& url)
     3741    : m_frame(frame), m_url(url), m_type(type)
     3742{
     3743    if (signal == LoadStarted)
     3744        connect(m_frame, SIGNAL(loadStarted()), SLOT(execute()));
     3745    else if (signal == LoadFinished)
     3746        connect(m_frame, SIGNAL(loadFinished(bool)), SLOT(execute()));
     3747    else
     3748        connect(m_frame, SIGNAL(provisionalLoad()), SLOT(execute()));
     3749}
     3750
     3751void URLSetter::execute()
     3752{
     3753    // We track only the first emission.
     3754    m_frame->disconnect(this);
     3755    if (m_type == URLSetter::UseLoad)
     3756        m_frame->load(m_url);
     3757    else
     3758        m_frame->setUrl(m_url);
     3759    connect(m_frame, SIGNAL(loadFinished(bool)), SIGNAL(finished()));
     3760}
     3761
     3762void tst_QWebFrame::loadInSignalHandlers_data()
     3763{
     3764    QTest::addColumn<URLSetter::Type>("type");
     3765    QTest::addColumn<URLSetter::Signal>("signal");
     3766    QTest::addColumn<QUrl>("url");
     3767
     3768    const QUrl validUrl("qrc:/test2.html");
     3769    const QUrl invalidUrl("qrc:/invalid");
     3770
     3771    QTest::newRow("call load() in loadStarted() after valid url") << URLSetter::UseLoad << URLSetter::LoadStarted << validUrl;
     3772    QTest::newRow("call load() in loadStarted() after invalid url") << URLSetter::UseLoad << URLSetter::LoadStarted << invalidUrl;
     3773    QTest::newRow("call load() in loadFinished() after valid url") << URLSetter::UseLoad << URLSetter::LoadFinished << validUrl;
     3774    QTest::newRow("call load() in loadFinished() after invalid url") << URLSetter::UseLoad << URLSetter::LoadFinished << invalidUrl;
     3775    QTest::newRow("call load() in provisionalLoad() after valid url") << URLSetter::UseLoad << URLSetter::ProvisionalLoad << validUrl;
     3776    QTest::newRow("call load() in provisionalLoad() after invalid url") << URLSetter::UseLoad << URLSetter::ProvisionalLoad << invalidUrl;
     3777
     3778    QTest::newRow("call setUrl() in loadStarted() after valid url") << URLSetter::UseSetUrl << URLSetter::LoadStarted << validUrl;
     3779    QTest::newRow("call setUrl() in loadStarted() after invalid url") << URLSetter::UseSetUrl << URLSetter::LoadStarted << invalidUrl;
     3780    QTest::newRow("call setUrl() in loadFinished() after valid url") << URLSetter::UseSetUrl << URLSetter::LoadFinished << validUrl;
     3781    QTest::newRow("call setUrl() in loadFinished() after invalid url") << URLSetter::UseSetUrl << URLSetter::LoadFinished << invalidUrl;
     3782    QTest::newRow("call setUrl() in provisionalLoad() after valid url") << URLSetter::UseSetUrl << URLSetter::ProvisionalLoad << validUrl;
     3783    QTest::newRow("call setUrl() in provisionalLoad() after invalid url") << URLSetter::UseSetUrl << URLSetter::ProvisionalLoad << invalidUrl;
     3784}
     3785
     3786void tst_QWebFrame::loadInSignalHandlers()
     3787{
     3788    QFETCH(URLSetter::Type, type);
     3789    QFETCH(URLSetter::Signal, signal);
     3790    QFETCH(QUrl, url);
     3791
     3792    QWebFrame* frame = m_page->mainFrame();
     3793    const QUrl urlForSetter("qrc:/test1.html");
     3794    URLSetter setter(frame, signal, type, urlForSetter);
     3795
     3796    frame->load(url);
     3797    waitForSignal(&setter, SIGNAL(finished()), 200);
     3798    QCOMPARE(frame->url(), urlForSetter);
     3799}
     3800
    37063801QTEST_MAIN(tst_QWebFrame)
    37073802#include "tst_qwebframe.moc"
Note: See TracChangeset for help on using the changeset viewer.