From c43c3c3ae46735e3d23e0ed70c31143d2556a579 Mon Sep 17 00:00:00 2001 From: Peter Varga Date: Mon, 25 Jan 2021 14:59:55 +0100 Subject: Fix loadFinished signal if page has content but server sends HTTP error For triggering an error page 3 conditions should be fulfilled: - main frame navigation - the page's document is empty - the HTTP status code indicates an error This fix adds check for the empty document and sends loadFinished signal without expecting an error page if the document is not empty. Fixes: QTBUG-90517 Change-Id: I6463d75fb5e682932feca64b0f059f9aa475795c Reviewed-by: Kirill Burtsev --- src/core/web_contents_adapter.cpp | 9 +- src/core/web_contents_adapter_client.h | 3 +- src/core/web_contents_delegate_qt.cpp | 28 +++++- src/core/web_contents_delegate_qt.h | 6 +- src/webengine/api/qquickwebengineview.cpp | 4 +- src/webengine/api/qquickwebengineview_p_p.h | 3 +- src/webenginewidgets/api/qwebenginepage.cpp | 8 +- src/webenginewidgets/api/qwebenginepage_p.h | 3 +- tests/auto/widgets/loadsignals/tst_loadsignals.cpp | 106 +++++++++++++++++++++ .../widgets/qwebenginepage/tst_qwebenginepage.cpp | 47 --------- 10 files changed, 153 insertions(+), 64 deletions(-) diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp index a5473372d..ee0c1fb3c 100644 --- a/src/core/web_contents_adapter.cpp +++ b/src/core/web_contents_adapter.cpp @@ -713,9 +713,10 @@ void WebContentsAdapter::load(const QWebEngineHttpRequest &request) // chromium accepts LOAD_TYPE_HTTP_POST only for the HTTP and HTTPS protocols if (!params.url.SchemeIsHTTPOrHTTPS()) { m_adapterClient->loadFinished(false, request.url(), false, - net::ERR_DISALLOWED_URL_SCHEME, - QCoreApplication::translate("WebContentsAdapter", - "HTTP-POST data can only be sent over HTTP(S) protocol")); + net::ERR_DISALLOWED_URL_SCHEME, + QCoreApplication::translate("WebContentsAdapter", + "HTTP-POST data can only be sent over HTTP(S) protocol"), + false); return; } params.post_data = network::ResourceRequestBody::CreateFromBytes( @@ -771,7 +772,7 @@ void WebContentsAdapter::setContent(const QByteArray &data, const QString &mimeT GURL dataUrlToLoad(urlString); if (dataUrlToLoad.spec().size() > url::kMaxURLChars) { - m_adapterClient->loadFinished(false, baseUrl, false, net::ERR_ABORTED); + m_adapterClient->loadFinished(false, baseUrl, false, net::ERR_ABORTED, QString(), false); return; } content::NavigationController::LoadURLParams params((dataUrlToLoad)); diff --git a/src/core/web_contents_adapter_client.h b/src/core/web_contents_adapter_client.h index 04df99f0e..267266d81 100644 --- a/src/core/web_contents_adapter_client.h +++ b/src/core/web_contents_adapter_client.h @@ -466,7 +466,8 @@ public: virtual void loadStarted(const QUrl &provisionalUrl, bool isErrorPage = false) = 0; virtual void loadCommitted() = 0; virtual void loadVisuallyCommitted() = 0; - virtual void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) = 0; + virtual void loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, + const QString &errorDescription, bool triggersErrorPage) = 0; virtual void focusContainer() = 0; virtual void unhandledKeyEvent(QKeyEvent *event) = 0; virtual QSharedPointer diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp index e594f648e..1e92a46f8 100644 --- a/src/core/web_contents_delegate_qt.cpp +++ b/src/core/web_contents_delegate_qt.cpp @@ -341,6 +341,7 @@ void WebContentsDelegateQt::RenderViewHostChanged(content::RenderViewHost *, con void WebContentsDelegateQt::EmitLoadStarted(const QUrl &url, bool isErrorPage) { + m_isDocumentEmpty = true; m_viewClient->loadStarted(url, isErrorPage); m_viewClient->updateNavigationActions(); @@ -383,9 +384,10 @@ void WebContentsDelegateQt::DidStartNavigation(content::NavigationHandle *naviga EmitLoadStarted(toQt(navigation_handle->GetURL())); } -void WebContentsDelegateQt::EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, const QString &errorDescription) +void WebContentsDelegateQt::EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, const QString &errorDescription, bool triggersErrorPage) { Q_ASSERT(!isErrorPage || webEngineSettings()->testAttribute(WebEngineSettings::ErrorPageEnabled)); + Q_ASSERT((triggersErrorPage && webEngineSettings()->testAttribute(WebEngineSettings::ErrorPageEnabled)) || !triggersErrorPage); // When error page enabled we don't need to send the error page load finished signal if (m_loadProgressMap[url] == 100) @@ -396,7 +398,7 @@ void WebContentsDelegateQt::EmitLoadFinished(bool success, const QUrl &url, bool m_isNavigationCommitted = false; m_viewClient->loadProgressChanged(100); - m_viewClient->loadFinished(success, url, isErrorPage, errorCode, errorDescription); + m_viewClient->loadFinished(success, url, isErrorPage, errorCode, errorDescription, triggersErrorPage); m_viewClient->updateNavigationActions(); } @@ -489,7 +491,11 @@ void WebContentsDelegateQt::DidStopLoading() void WebContentsDelegateQt::didFailLoad(const QUrl &url, int errorCode, const QString &errorDescription) { m_viewClient->iconChanged(QUrl()); - EmitLoadFinished(false /* success */ , url, false /* isErrorPage */, errorCode, errorDescription); + bool errorPageEnabled = webEngineSettings()->testAttribute(WebEngineSettings::ErrorPageEnabled); + // Delay notifying failure until the error-page is done loading. + // Error-pages are not loaded on failures due to abort. + bool aborted = (errorCode == -3 /* ERR_ABORTED*/ ); + EmitLoadFinished(false /* success */ , url, false /* isErrorPage */, errorCode, errorDescription, errorPageEnabled && !aborted); } void WebContentsDelegateQt::DidFailLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url, int error_code) @@ -542,7 +548,9 @@ void WebContentsDelegateQt::DidFinishLoad(content::RenderFrameHost* render_frame content::NavigationEntry *entry = web_contents()->GetController().GetActiveEntry(); int http_statuscode = entry ? entry->GetHttpStatusCode() : 0; - EmitLoadFinished(http_statuscode < 400, toQt(validated_url), false /* isErrorPage */, http_statuscode); + bool errorPageEnabled = webEngineSettings()->testAttribute(WebEngineSettings::ErrorPageEnabled); + bool triggersErrorPage = errorPageEnabled && (http_statuscode >= 400) && m_isDocumentEmpty; + EmitLoadFinished(http_statuscode < 400, toQt(validated_url), false /* isErrorPage */, http_statuscode, QString(), triggersErrorPage); } void WebContentsDelegateQt::DidUpdateFaviconURL(content::RenderFrameHost *render_frame_host, const std::vector &candidates) @@ -856,6 +864,18 @@ bool WebContentsDelegateQt::ShouldNavigateOnBackForwardMouseButtons() #endif } +void WebContentsDelegateQt::ResourceLoadComplete(content::RenderFrameHost* render_frame_host, + const content::GlobalRequestID& request_id, + const blink::mojom::ResourceLoadInfo& resource_load_info) +{ + Q_UNUSED(render_frame_host); + Q_UNUSED(request_id); + + if (resource_load_info.request_destination == network::mojom::RequestDestination::kDocument) { + m_isDocumentEmpty = (resource_load_info.raw_body_bytes == 0); + } +} + FaviconManager *WebContentsDelegateQt::faviconManager() { return m_faviconManager.data(); diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h index cd6d901e4..5a3dff6e9 100644 --- a/src/core/web_contents_delegate_qt.h +++ b/src/core/web_contents_delegate_qt.h @@ -175,6 +175,9 @@ public: void DidFirstVisuallyNonEmptyPaint() override; void ActivateContents(content::WebContents* contents) override; bool ShouldNavigateOnBackForwardMouseButtons() override; + void ResourceLoadComplete(content::RenderFrameHost* render_frame_host, + const content::GlobalRequestID& request_id, + const blink::mojom::ResourceLoadInfo& resource_load_info) override; void didFailLoad(const QUrl &url, int errorCode, const QString &errorDescription); void overrideWebPreferences(content::WebContents *, blink::web_pref::WebPreferences*); @@ -214,7 +217,7 @@ private: const QUrl &url, bool user_gesture); void EmitLoadStarted(const QUrl &url, bool isErrorPage = false); - void EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()); + void EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString(), bool triggersErrorPage = false); void EmitLoadCommitted(); LoadingState determineLoadingState(content::WebContents *contents); @@ -242,6 +245,7 @@ private: QMap m_loadProgressMap; QUrl m_lastLoadedUrl; bool m_isNavigationCommitted = false; + bool m_isDocumentEmpty = true; base::WeakPtrFactory m_weakPtrFactory { this }; }; diff --git a/src/webengine/api/qquickwebengineview.cpp b/src/webengine/api/qquickwebengineview.cpp index db112e7bc..195833fe7 100644 --- a/src/webengine/api/qquickwebengineview.cpp +++ b/src/webengine/api/qquickwebengineview.cpp @@ -495,9 +495,11 @@ Q_STATIC_ASSERT(static_cast(WebEngineError::NoErrorDomain) == static_cast(WebEngineError::CertificateErrorDomain) == static_cast(QQuickWebEngineView::CertificateErrorDomain)); Q_STATIC_ASSERT(static_cast(WebEngineError::DnsErrorDomain) == static_cast(QQuickWebEngineView::DnsErrorDomain)); -void QQuickWebEngineViewPrivate::loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, const QString &errorDescription) +void QQuickWebEngineViewPrivate::loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, + const QString &errorDescription, bool triggersErrorPage) { Q_Q(QQuickWebEngineView); + Q_UNUSED(triggersErrorPage); if (isErrorPage) { #if QT_CONFIG(webengine_testsupport) diff --git a/src/webengine/api/qquickwebengineview_p_p.h b/src/webengine/api/qquickwebengineview_p_p.h index 68d65410a..ebe55c345 100644 --- a/src/webengine/api/qquickwebengineview_p_p.h +++ b/src/webengine/api/qquickwebengineview_p_p.h @@ -116,7 +116,8 @@ public: void loadStarted(const QUrl &provisionalUrl, bool isErrorPage = false) override; void loadCommitted() override; void loadVisuallyCommitted() override; - void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) override; + void loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, + const QString &errorDescription, bool triggersErrorPage) override; void focusContainer() override; void unhandledKeyEvent(QKeyEvent *event) override; QSharedPointer diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp index f6e4e9f9b..57aa413b9 100644 --- a/src/webenginewidgets/api/qwebenginepage.cpp +++ b/src/webenginewidgets/api/qwebenginepage.cpp @@ -282,7 +282,8 @@ void QWebEnginePagePrivate::loadStarted(const QUrl &provisionalUrl, bool isError QTimer::singleShot(0, q, &QWebEnginePage::loadStarted); } -void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, const QString &errorDescription) +void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, + const QString &errorDescription, bool triggersErrorPage) { Q_Q(QWebEnginePage); Q_UNUSED(url); @@ -297,9 +298,8 @@ void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isE } isLoading = false; - // 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_ASSERT((success && !triggersErrorPage) || !success); + if (!triggersErrorPage) { QTimer::singleShot(0, q, [q, success](){ emit q->loadFinished(success); }); diff --git a/src/webenginewidgets/api/qwebenginepage_p.h b/src/webenginewidgets/api/qwebenginepage_p.h index 3ddf4b3d6..82ce99503 100644 --- a/src/webenginewidgets/api/qwebenginepage_p.h +++ b/src/webenginewidgets/api/qwebenginepage_p.h @@ -107,7 +107,8 @@ public: void loadStarted(const QUrl &provisionalUrl, bool isErrorPage = false) override; void loadCommitted() override { } void loadVisuallyCommitted() override { } - void loadFinished(bool success, const QUrl &url, bool isErrorPage = false, int errorCode = 0, const QString &errorDescription = QString()) override; + void loadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, + const QString &errorDescription, bool triggersErrorPage) override; void focusContainer() override; void unhandledKeyEvent(QKeyEvent *event) override; QSharedPointer diff --git a/tests/auto/widgets/loadsignals/tst_loadsignals.cpp b/tests/auto/widgets/loadsignals/tst_loadsignals.cpp index b4170587d..2dfe341d0 100644 --- a/tests/auto/widgets/loadsignals/tst_loadsignals.cpp +++ b/tests/auto/widgets/loadsignals/tst_loadsignals.cpp @@ -53,6 +53,10 @@ private Q_SLOTS: void loadAfterInPageNavigation_qtbug66869(); void fileDownloadDoesNotTriggerLoadSignals_qtbug66661(); void numberOfStartedAndFinishedSignalsIsSame(); + void loadFinishedAfterNotFoundError_data(); + void loadFinishedAfterNotFoundError(); + void errorPageTriggered_data(); + void errorPageTriggered(); private: QWebEngineProfile profile; @@ -72,6 +76,11 @@ void tst_LoadSignals::initTestCase() void tst_LoadSignals::init() { + // Reset content + loadFinishedSpy.clear(); + view.load(QUrl("about:blank")); + QTRY_COMPARE(loadFinishedSpy.count(), 1); + loadStartedSpy.clear(); loadProgressSpy.clear(); loadFinishedSpy.clear(); @@ -283,5 +292,102 @@ void tst_LoadSignals::numberOfStartedAndFinishedSignalsIsSame() { QVERIFY(loadFinishedSpy[3][0].toBool()); } +void tst_LoadSignals::loadFinishedAfterNotFoundError_data() +{ + QTest::addColumn("rfcInvalid"); + QTest::addColumn("withServer"); + QTest::addRow("rfc_invalid") << true << false; + QTest::addRow("non_existent") << false << false; + QTest::addRow("server_404") << false << true; +} + +void tst_LoadSignals::loadFinishedAfterNotFoundError() +{ + QFETCH(bool, withServer); + QFETCH(bool, rfcInvalid); + + QScopedPointer server; + if (withServer) { + server.reset(new HttpServer); + QVERIFY(server->start()); + } + + view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, false); + auto url = server + ? server->url("/not-found-page.html") + : QUrl(rfcInvalid ? "http://some.invalid" : "http://non.existent/url"); + view.load(url); + QTRY_COMPARE_WITH_TIMEOUT(loadFinishedSpy.count(), 1, 20000); + QVERIFY(!loadFinishedSpy.at(0).at(0).toBool()); + QCOMPARE(toPlainTextSync(view.page()), QString()); + QCOMPARE(loadFinishedSpy.count(), 1); + + view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, true); + url = server + ? server->url("/another-missing-one.html") + : QUrl(rfcInvalid ? "http://some.other.invalid" : "http://another.non.existent/url"); + view.load(url); + QTRY_COMPARE_WITH_TIMEOUT(loadFinishedSpy.count(), 2, 20000); + QVERIFY(!loadFinishedSpy.at(1).at(0).toBool()); + + QEXPECT_FAIL("", "No more loads (like separate load for error pages) are expected", Continue); + QTRY_COMPARE_WITH_TIMEOUT(loadFinishedSpy.count(), 3, 1000); +} + +void tst_LoadSignals::errorPageTriggered_data() +{ + QTest::addColumn("urlPath"); + QTest::addColumn("loadSucceed"); + QTest::addColumn("triggersErrorPage"); + QTest::newRow("/content/200") << QStringLiteral("/content/200") << true << false; + QTest::newRow("/empty/200") << QStringLiteral("/content/200") << true << false; + QTest::newRow("/content/404") << QStringLiteral("/content/404") << false << false; + QTest::newRow("/empty/404") << QStringLiteral("/empty/404") << false << true; +} + +void tst_LoadSignals::errorPageTriggered() +{ + HttpServer server; + connect(&server, &HttpServer::newRequest, [] (HttpReqRep *rr) { + QList parts = rr->requestPath().split('/'); + if (parts.length() != 3) { + // For example, /favicon.ico + rr->sendResponse(404); + return; + } + bool isDocumentEmpty = (parts[1] == "empty"); + int httpStatusCode = parts[2].toInt(); + + rr->setResponseHeader(QByteArrayLiteral("content-type"), QByteArrayLiteral("text/html")); + if (!isDocumentEmpty) { + rr->setResponseBody(QByteArrayLiteral("")); + } + rr->sendResponse(httpStatusCode); + }); + QVERIFY(server.start()); + + QFETCH(QString, urlPath); + QFETCH(bool, loadSucceed); + QFETCH(bool, triggersErrorPage); + + view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, true); + view.load(server.url(urlPath)); + QTRY_COMPARE(loadFinishedSpy.size(), 1); + QCOMPARE(loadFinishedSpy[0][0].toBool(), loadSucceed); + if (triggersErrorPage) + QVERIFY(toPlainTextSync(view.page()).contains("HTTP ERROR 404")); + else + QVERIFY(toPlainTextSync(view.page()).isEmpty()); + loadFinishedSpy.clear(); + + view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, false); + view.load(server.url(urlPath)); + QTRY_COMPARE(loadFinishedSpy.size(), 1); + QCOMPARE(loadFinishedSpy[0][0].toBool(), loadSucceed); + QVERIFY(toPlainTextSync(view.page()).isEmpty()); + loadFinishedSpy.clear(); +} + + QTEST_MAIN(tst_LoadSignals) #include "tst_loadsignals.moc" diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp index 797d4e4e7..7e92ad8cc 100644 --- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp +++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp @@ -174,8 +174,6 @@ private Q_SLOTS: void setUrlUsingStateObject(); void setUrlThenLoads_data(); void setUrlThenLoads(); - void loadFinishedAfterNotFoundError_data(); - void loadFinishedAfterNotFoundError(); void loadInSignalHandlers_data(); void loadInSignalHandlers(); void loadFromQrc(); @@ -2824,51 +2822,6 @@ void tst_QWebEnginePage::setUrlThenLoads() QCOMPARE(baseUrlSync(m_page), extractBaseUrl(urlToLoad2)); } -void tst_QWebEnginePage::loadFinishedAfterNotFoundError_data() -{ - QTest::addColumn("rfcInvalid"); - QTest::addColumn("withServer"); - QTest::addRow("rfc_invalid") << true << false; - QTest::addRow("non_existent") << false << false; - QTest::addRow("server_404") << false << true; -} - -void tst_QWebEnginePage::loadFinishedAfterNotFoundError() -{ - QFETCH(bool, withServer); - QFETCH(bool, rfcInvalid); - - QScopedPointer server; - if (withServer) { - server.reset(new HttpServer); - QVERIFY(server->start()); - } - - QWebEnginePage page; - QSignalSpy spy(&page, SIGNAL(loadFinished(bool))); - - page.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, false); - auto url = server - ? server->url("/not-found-page.html") - : QUrl(rfcInvalid ? "http://some.invalid" : "http://non.existent/url"); - page.setUrl(url); - QTRY_COMPARE_WITH_TIMEOUT(spy.count(), 1, 20000); - QVERIFY(!spy.at(0).at(0).toBool()); - QCOMPARE(toPlainTextSync(&page), QString()); - QCOMPARE(spy.count(), 1); - - page.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, true); - url = server - ? server->url("/another-missing-one.html") - : QUrl(rfcInvalid ? "http://some.other.invalid" : "http://another.non.existent/url"); - page.setUrl(url); - QTRY_COMPARE_WITH_TIMEOUT(spy.count(), 2, 20000); - QVERIFY(!spy.at(1).at(0).toBool()); - - QEXPECT_FAIL("", "No more loads (like separate load for error pages) are expected", Continue); - QTRY_COMPARE_WITH_TIMEOUT(spy.count(), 3, 1000); -} - class URLSetter : public QObject { Q_OBJECT -- cgit v1.2.1