- Revision
- 91855
- Author
- caio.olive...@openbossa.org
- Date
- 2011-07-27 10:40:57 -0700 (Wed, 27 Jul 2011)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebKit/qt/Api/qwebframe.cpp (91854 => 91855)
--- trunk/Source/WebKit/qt/Api/qwebframe.cpp 2011-07-27 17:20:34 UTC (rev 91854)
+++ trunk/Source/WebKit/qt/Api/qwebframe.cpp 2011-07-27 17:40:57 UTC (rev 91855)
@@ -802,8 +802,10 @@
static inline void clearCoreFrame(WebCore::Frame* frame)
{
- frame->loader()->activeDocumentLoader()->writer()->begin();
- frame->loader()->activeDocumentLoader()->writer()->end();
+ WebCore::DocumentLoader* documentLoader = frame->loader()->activeDocumentLoader();
+ Q_ASSERT(documentLoader);
+ documentLoader->writer()->begin();
+ documentLoader->writer()->end();
}
static inline bool isCoreFrameClear(WebCore::Frame* frame)
Modified: trunk/Source/WebKit/qt/ChangeLog (91854 => 91855)
--- trunk/Source/WebKit/qt/ChangeLog 2011-07-27 17:20:34 UTC (rev 91854)
+++ trunk/Source/WebKit/qt/ChangeLog 2011-07-27 17:40:57 UTC (rev 91855)
@@ -1,5 +1,82 @@
2011-07-27 Caio Marcelo de Oliveira Filho <caio.olive...@openbossa.org>
+ [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.
+
+2011-07-27 Caio Marcelo de Oliveira Filho <caio.olive...@openbossa.org>
+
[Qt] Fix build in Qt 5 of QDeclarativeWebView
https://bugs.webkit.org/show_bug.cgi?id=65258
Modified: trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp (91854 => 91855)
--- trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp 2011-07-27 17:20:34 UTC (rev 91854)
+++ trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp 2011-07-27 17:40:57 UTC (rev 91855)
@@ -210,7 +210,7 @@
, m_pluginView(0)
, m_hasSentResponseToPlugin(false)
, m_hasRepresentation(false)
- , m_loadError(ResourceError())
+ , m_isOriginatingLoad(false)
{
}
@@ -229,22 +229,14 @@
return;
}
- connect(this, SIGNAL(loadStarted()),
- m_webFrame->page(), SIGNAL(loadStarted()));
- connect(this, SIGNAL(loadStarted()),
- m_webFrame, SIGNAL(loadStarted()));
connect(this, SIGNAL(loadProgress(int)),
m_webFrame->page(), SIGNAL(loadProgress(int)));
- connect(this, SIGNAL(loadFinished(bool)),
- m_webFrame->page(), SIGNAL(loadFinished(bool)));
// FIXME: The queued connection here is needed because of a problem with QNetworkAccessManager.
// See http://bugreports.qt.nokia.com/browse/QTBUG-18718
connect(this, SIGNAL(unsupportedContent(QNetworkReply*)),
m_webFrame->page(), SIGNAL(unsupportedContent(QNetworkReply*)), Qt::QueuedConnection);
- connect(this, SIGNAL(loadFinished(bool)),
- m_webFrame, SIGNAL(loadFinished(bool)));
connect(this, SIGNAL(titleChanged(QString)),
m_webFrame, SIGNAL(titleChanged(QString)));
}
@@ -454,8 +446,11 @@
m_lastRequestedUrl = m_frame->loader()->activeDocumentLoader()->requestURL();
- if (m_webFrame)
- emit m_webFrame->provisionalLoad();
+ if (!m_webFrame)
+ return;
+ emitLoadStarted();
+ postProgressEstimateChangedNotification();
+ emit m_webFrame->provisionalLoad();
}
@@ -532,12 +527,11 @@
if (dumpFrameLoaderCallbacks)
printf("%s - didFinishLoadForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
- // Clears the previous error.
- m_loadError = ResourceError();
-
if (!m_webFrame)
return;
+
m_webFrame->page()->d->updateNavigationActions();
+ emitLoadFinished(true);
}
@@ -585,12 +579,8 @@
void FrameLoaderClientQt::postProgressStartedNotification()
{
- if (m_webFrame && m_frame->page()) {
- // As a new load have started, clear the previous error.
- m_loadError = ResourceError();
- emit loadStarted();
- postProgressEstimateChangedNotification();
- }
+ if (m_webFrame && m_frame->page())
+ m_isOriginatingLoad = true;
if (m_frame->tree()->parent() || !m_webFrame)
return;
m_webFrame->page()->d->updateNavigationActions();
@@ -617,9 +607,6 @@
}
}
}
-
- if (m_webFrame && m_frame->page())
- emit loadFinished(m_loadError.isNull());
}
void FrameLoaderClientQt::setMainFrameDocumentReady(bool)
@@ -1138,38 +1125,39 @@
return false;
}
-void FrameLoaderClientQt::callErrorPageExtension(const WebCore::ResourceError& error)
+bool FrameLoaderClientQt::callErrorPageExtension(const WebCore::ResourceError& error)
{
QWebPage* page = m_webFrame->page();
- if (page->supportsExtension(QWebPage::ErrorPageExtension)) {
- QWebPage::ErrorPageExtensionOption option;
+ if (!page->supportsExtension(QWebPage::ErrorPageExtension))
+ return false;
- if (error.domain() == "QtNetwork")
- option.domain = QWebPage::QtNetwork;
- else if (error.domain() == "HTTP")
- option.domain = QWebPage::Http;
- else if (error.domain() == "WebKit")
- option.domain = QWebPage::WebKit;
- else
- return;
+ QWebPage::ErrorPageExtensionOption option;
+ if (error.domain() == "QtNetwork")
+ option.domain = QWebPage::QtNetwork;
+ else if (error.domain() == "HTTP")
+ option.domain = QWebPage::Http;
+ else if (error.domain() == "WebKit")
+ option.domain = QWebPage::WebKit;
+ else
+ return false;
- option.url = ""
- option.frame = m_webFrame;
- option.error = error.errorCode();
- option.errorString = error.localizedDescription();
+ option.url = ""
+ option.frame = m_webFrame;
+ option.error = error.errorCode();
+ option.errorString = error.localizedDescription();
- QWebPage::ErrorPageExtensionReturn output;
- if (!page->extension(QWebPage::ErrorPageExtension, &option, &output))
- return;
+ QWebPage::ErrorPageExtensionReturn output;
+ if (!page->extension(QWebPage::ErrorPageExtension, &option, &output))
+ return false;
- KURL baseUrl(output.baseUrl);
- KURL failingUrl(option.url);
+ KURL baseUrl(output.baseUrl);
+ KURL failingUrl(option.url);
- WebCore::ResourceRequest request(baseUrl);
- WTF::RefPtr<WebCore::SharedBuffer> buffer = WebCore::SharedBuffer::create(output.content.constData(), output.content.length());
- WebCore::SubstituteData substituteData(buffer, output.contentType, output.encoding, failingUrl);
- m_frame->loader()->load(request, substituteData, false);
- }
+ WebCore::ResourceRequest request(baseUrl);
+ WTF::RefPtr<WebCore::SharedBuffer> buffer = WebCore::SharedBuffer::create(output.content.constData(), output.content.length());
+ WebCore::SubstituteData substituteData(buffer, output.contentType, output.encoding, failingUrl);
+ m_frame->loader()->load(request, substituteData, false);
+ return true;
}
void FrameLoaderClientQt::dispatchDidFailProvisionalLoad(const WebCore::ResourceError& error)
@@ -1177,9 +1165,13 @@
if (dumpFrameLoaderCallbacks)
printf("%s - didFailProvisionalLoadWithError\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
- m_loadError = error;
- if (!error.isNull() && !error.isCancellation())
- callErrorPageExtension(error);
+ if (!error.isNull() && !error.isCancellation()) {
+ if (callErrorPageExtension(error))
+ return;
+ }
+
+ if (m_webFrame)
+ emitLoadFinished(false);
}
void FrameLoaderClientQt::dispatchDidFailLoad(const WebCore::ResourceError& error)
@@ -1187,9 +1179,13 @@
if (dumpFrameLoaderCallbacks)
printf("%s - didFailLoadWithError\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
- m_loadError = error;
- if (!error.isNull() && !error.isCancellation())
- callErrorPageExtension(error);
+ if (!error.isNull() && !error.isCancellation()) {
+ if (callErrorPageExtension(error))
+ return;
+ }
+
+ if (m_webFrame)
+ emitLoadFinished(false);
}
WebCore::Frame* FrameLoaderClientQt::dispatchCreatePage(const WebCore::NavigationAction&)
@@ -1688,6 +1684,26 @@
return FrameNetworkingContextQt::create(m_frame, m_webFrame, !MIMESniffingDisabled, m_webFrame->page()->networkAccessManager());
}
+void FrameLoaderClientQt::emitLoadStarted()
+{
+ QWebPage* webPage = m_webFrame->page();
+ if (m_isOriginatingLoad && webPage)
+ emit webPage->loadStarted();
+ emit m_webFrame->loadStarted();
}
+void FrameLoaderClientQt::emitLoadFinished(bool ok)
+{
+ // Signal handlers can lead to a new load, that will use the member again.
+ const bool wasOriginatingLoad = m_isOriginatingLoad;
+ m_isOriginatingLoad = false;
+
+ QWebPage* webPage = m_webFrame->page();
+ if (wasOriginatingLoad && webPage)
+ emit webPage->loadFinished(ok);
+ emit m_webFrame->loadFinished(ok);
+}
+
+}
+
#include "moc_FrameLoaderClientQt.cpp"
Modified: trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h (91854 => 91855)
--- trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h 2011-07-27 17:20:34 UTC (rev 91854)
+++ trunk/Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h 2011-07-27 17:40:57 UTC (rev 91855)
@@ -67,11 +67,10 @@
friend class ::QWebFrame;
void callPolicyFunction(FramePolicyFunction function, PolicyAction action);
- void callErrorPageExtension(const ResourceError&);
+ bool callErrorPageExtension(const ResourceError&);
+
signals:
- void loadStarted();
void loadProgress(int d);
- void loadFinished(bool);
void titleChanged(const QString& title);
void unsupportedContent(QNetworkReply*);
@@ -265,6 +264,9 @@
void onIconLoadedForPageURL(const QString&);
private:
+ void emitLoadStarted();
+ void emitLoadFinished(bool ok);
+
Frame *m_frame;
QWebFrame *m_webFrame;
ResourceResponse m_response;
@@ -279,7 +281,7 @@
bool m_hasRepresentation;
KURL m_lastRequestedUrl;
- ResourceError m_loadError;
+ bool m_isOriginatingLoad;
};
}
Modified: trunk/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp (91854 => 91855)
--- trunk/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp 2011-07-27 17:20:34 UTC (rev 91854)
+++ trunk/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp 2011-07-27 17:40:57 UTC (rev 91855)
@@ -660,6 +660,8 @@
void setUrlThenLoads_data();
void setUrlThenLoads();
void loadFinishedAfterNotFoundError();
+ void loadInSignalHandlers_data();
+ void loadInSignalHandlers();
private:
QString evalJS(const QString&s) {
@@ -3703,5 +3705,98 @@
QVERIFY(!wasLoadOk);
}
+class URLSetter : public QObject {
+ Q_OBJECT
+
+public:
+ enum Signal {
+ LoadStarted,
+ LoadFinished,
+ ProvisionalLoad
+ };
+
+ enum Type {
+ UseLoad,
+ UseSetUrl
+ };
+
+ URLSetter(QWebFrame*, Signal, Type, const QUrl&);
+
+public slots:
+ void execute();
+
+signals:
+ void finished();
+
+private:
+ QWebFrame* m_frame;
+ QUrl m_url;
+ Type m_type;
+};
+
+Q_DECLARE_METATYPE(URLSetter::Signal)
+Q_DECLARE_METATYPE(URLSetter::Type)
+
+URLSetter::URLSetter(QWebFrame* frame, Signal signal, URLSetter::Type type, const QUrl& url)
+ : m_frame(frame), m_url(url), m_type(type)
+{
+ if (signal == LoadStarted)
+ connect(m_frame, SIGNAL(loadStarted()), SLOT(execute()));
+ else if (signal == LoadFinished)
+ connect(m_frame, SIGNAL(loadFinished(bool)), SLOT(execute()));
+ else
+ connect(m_frame, SIGNAL(provisionalLoad()), SLOT(execute()));
+}
+
+void URLSetter::execute()
+{
+ // We track only the first emission.
+ m_frame->disconnect(this);
+ if (m_type == URLSetter::UseLoad)
+ m_frame->load(m_url);
+ else
+ m_frame->setUrl(m_url);
+ connect(m_frame, SIGNAL(loadFinished(bool)), SIGNAL(finished()));
+}
+
+void tst_QWebFrame::loadInSignalHandlers_data()
+{
+ QTest::addColumn<URLSetter::Type>("type");
+ QTest::addColumn<URLSetter::Signal>("signal");
+ QTest::addColumn<QUrl>("url");
+
+ const QUrl validUrl("qrc:/test2.html");
+ const QUrl invalidUrl("qrc:/invalid");
+
+ QTest::newRow("call load() in loadStarted() after valid url") << URLSetter::UseLoad << URLSetter::LoadStarted << validUrl;
+ QTest::newRow("call load() in loadStarted() after invalid url") << URLSetter::UseLoad << URLSetter::LoadStarted << invalidUrl;
+ QTest::newRow("call load() in loadFinished() after valid url") << URLSetter::UseLoad << URLSetter::LoadFinished << validUrl;
+ QTest::newRow("call load() in loadFinished() after invalid url") << URLSetter::UseLoad << URLSetter::LoadFinished << invalidUrl;
+ QTest::newRow("call load() in provisionalLoad() after valid url") << URLSetter::UseLoad << URLSetter::ProvisionalLoad << validUrl;
+ QTest::newRow("call load() in provisionalLoad() after invalid url") << URLSetter::UseLoad << URLSetter::ProvisionalLoad << invalidUrl;
+
+ QTest::newRow("call setUrl() in loadStarted() after valid url") << URLSetter::UseSetUrl << URLSetter::LoadStarted << validUrl;
+ QTest::newRow("call setUrl() in loadStarted() after invalid url") << URLSetter::UseSetUrl << URLSetter::LoadStarted << invalidUrl;
+ QTest::newRow("call setUrl() in loadFinished() after valid url") << URLSetter::UseSetUrl << URLSetter::LoadFinished << validUrl;
+ QTest::newRow("call setUrl() in loadFinished() after invalid url") << URLSetter::UseSetUrl << URLSetter::LoadFinished << invalidUrl;
+ QTest::newRow("call setUrl() in provisionalLoad() after valid url") << URLSetter::UseSetUrl << URLSetter::ProvisionalLoad << validUrl;
+ QTest::newRow("call setUrl() in provisionalLoad() after invalid url") << URLSetter::UseSetUrl << URLSetter::ProvisionalLoad << invalidUrl;
+}
+
+void tst_QWebFrame::loadInSignalHandlers()
+{
+ QFETCH(URLSetter::Type, type);
+ QFETCH(URLSetter::Signal, signal);
+ QFETCH(QUrl, url);
+
+ QWebFrame* frame = m_page->mainFrame();
+ const QUrl urlForSetter("qrc:/test1.html");
+ URLSetter setter(frame, signal, type, urlForSetter);
+
+ frame->load(url);
+ waitForSignal(&setter, SIGNAL(finished()), 200);
+ QCOMPARE(frame->url(), urlForSetter);
+}
+
QTEST_MAIN(tst_QWebFrame)
#include "tst_qwebframe.moc"
