From 9db27320eda70ec4e4948ae0853dd3f2dd08912b Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Fri, 5 Feb 2016 13:27:14 +0100 Subject: Fix double LoadFinished on URL errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RenderFrameDeleted was called before DidFinishLoad, which meant m_loadingErrorFrameList was empty and wouldn't detect the page loaded was an error page. Instead this patch relies on the chromium error-page url which we already asserted. Additonally we delay emitting the loadFinished signal until the error page is also done loading, since the error-page can be considered part of the load, and we otherwise have a race condition on toPlainText. Finally we were not getting error-pages when blocking requests because we reported them as aborted them instead of blocked. Change-Id: I945eb838b7f080d4e146f18354e8986e1b88b5cd Task-number: QTBUG-50752 Reviewed-by: Michael Brüning --- src/core/network_delegate_qt.cpp | 2 +- src/core/web_contents_delegate_qt.cpp | 15 ++++++++-- src/webenginewidgets/api/qwebenginepage.cpp | 12 ++++++-- .../widgets/qwebenginepage/tst_qwebenginepage.cpp | 33 ++++++++++++++++++++++ .../qwebengineprofile/tst_qwebengineprofile.cpp | 3 +- 5 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/core/network_delegate_qt.cpp b/src/core/network_delegate_qt.cpp index 6fdabcd4c..0c0de64fd 100644 --- a/src/core/network_delegate_qt.cpp +++ b/src/core/network_delegate_qt.cpp @@ -111,7 +111,7 @@ int NetworkDelegateQt::OnBeforeURLRequest(net::URLRequest *request, const net::C QWebEngineUrlRequestInfo requestInfo(infoPrivate); interceptor->interceptRequest(requestInfo); if (requestInfo.changed()) { - int result = infoPrivate->shouldBlockRequest ? net::ERR_ABORTED : net::OK; + int result = infoPrivate->shouldBlockRequest ? net::ERR_BLOCKED_BY_CLIENT : net::OK; if (qUrl != infoPrivate->url) *newUrl = toGurl(infoPrivate->url); diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp index 81f16970d..baf61b5fc 100644 --- a/src/core/web_contents_delegate_qt.cpp +++ b/src/core/web_contents_delegate_qt.cpp @@ -191,7 +191,15 @@ void WebContentsDelegateQt::DidFailProvisionalLoad(content::RenderFrameHost* ren void WebContentsDelegateQt::DidFailLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url, int error_code, const base::string16& error_description, bool was_ignored_by_handler) { Q_UNUSED(was_ignored_by_handler); - if (m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID()) || render_frame_host->GetParent()) + if (validated_url.spec() == content::kUnreachableWebDataURL) { + m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID()); + qCritical("Loading error-page failed. This shouldn't happen."); + if (!render_frame_host->GetParent()) + m_viewClient->loadFinished(false /* success */, toQt(validated_url), true /* isErrorPage */); + return; + } + + if (render_frame_host->GetParent()) return; m_viewClient->loadFinished(false /* success */ , toQt(validated_url), false /* isErrorPage */, error_code, toQt(error_description)); @@ -200,8 +208,9 @@ void WebContentsDelegateQt::DidFailLoad(content::RenderFrameHost* render_frame_h void WebContentsDelegateQt::DidFinishLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url) { - if (m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID())) { - Q_ASSERT(validated_url.is_valid() && validated_url.spec() == content::kUnreachableWebDataURL); + Q_ASSERT(validated_url.is_valid()); + if (validated_url.spec() == content::kUnreachableWebDataURL) { + m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID()); m_viewClient->iconChanged(QUrl()); // Trigger LoadFinished signal for main frame's error page only. diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp index b1bf33067..458ddae5f 100644 --- a/src/webenginewidgets/api/qwebenginepage.cpp +++ b/src/webenginewidgets/api/qwebenginepage.cpp @@ -200,13 +200,21 @@ void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isE Q_UNUSED(errorCode); Q_UNUSED(errorDescription); - if (isErrorPage) + if (isErrorPage) { + Q_ASSERT(settings->testAttribute(QWebEngineSettings::ErrorPageEnabled)); + Q_ASSERT(success); + Q_EMIT q->loadFinished(false); return; + } isLoading = false; if (success) explicitUrl = QUrl(); - Q_EMIT q->loadFinished(success); + // Delay notifying failure until the error-page is done loading. + // Error-pages are not loaded on failures due to abort. + if (success || errorCode == -3 /* ERR_ABORTED*/ || !settings->testAttribute(QWebEngineSettings::ErrorPageEnabled)) { + Q_EMIT q->loadFinished(success); + } updateNavigationActions(); } diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp index 690cf70e4..207eb019a 100644 --- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp +++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp @@ -241,6 +241,8 @@ private Q_SLOTS: void loadInSignalHandlers(); void restoreHistory(); + void toPlainTextLoadFinishedRace_data(); + void toPlainTextLoadFinishedRace(); private: QWebEngineView* m_view; @@ -5115,5 +5117,36 @@ void tst_QWebEnginePage::restoreHistory() delete channel; } +void tst_QWebEnginePage::toPlainTextLoadFinishedRace_data() +{ + QTest::addColumn("enableErrorPage"); + QTest::newRow("disableErrorPage") << false; + QTest::newRow("enableErrorPage") << true; +} + +void tst_QWebEnginePage::toPlainTextLoadFinishedRace() +{ + QFETCH(bool, enableErrorPage); + + QWebEnginePage *page = new QWebEnginePage; + page->settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, enableErrorPage); + QSignalSpy spy(page, SIGNAL(loadFinished(bool))); + + page->load(QUrl("data:text/plain,foobarbaz")); + QTRY_VERIFY(spy.count() == 1); + QCOMPARE(toPlainTextSync(page), QString("foobarbaz")); + + page->load(QUrl("fail:unknown/scheme")); + QTRY_VERIFY(spy.count() == 2); + QString s = toPlainTextSync(page); + QVERIFY(s.contains("foobarbaz") == !enableErrorPage); + + page->load(QUrl("data:text/plain,lalala")); + QTRY_VERIFY(spy.count() == 3); + QCOMPARE(toPlainTextSync(page), QString("lalala")); + delete page; + QVERIFY(spy.count() == 3); +} + QTEST_MAIN(tst_QWebEnginePage) #include "tst_qwebenginepage.moc" diff --git a/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp b/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp index 9195a5190..7cd423356 100644 --- a/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp +++ b/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp @@ -182,9 +182,10 @@ void tst_QWebEngineProfile::urlSchemeHandlerFailRequest() QWebEngineView view; QSignalSpy loadFinishedSpy(&view, SIGNAL(loadFinished(bool))); view.setPage(new QWebEnginePage(&profile, &view)); + view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, false); view.load(QUrl(QStringLiteral("foo://bar"))); QVERIFY(loadFinishedSpy.wait()); - QVERIFY(toPlainTextSync(view.page()).isEmpty()); + QCOMPARE(toPlainTextSync(view.page()), QString()); } QTEST_MAIN(tst_QWebEngineProfile) -- cgit v1.2.3