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 <kirill.burtsev@qt.io>

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<WebContentsAdapter>
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<blink::mojom::FaviconURLPtr> &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<QUrl, int> m_loadProgressMap;
     QUrl m_lastLoadedUrl;
     bool m_isNavigationCommitted = false;
+    bool m_isDocumentEmpty = true;
     base::WeakPtrFactory<WebContentsDelegateQt> 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<int>(WebEngineError::NoErrorDomain) == static_cast<i
 Q_STATIC_ASSERT(static_cast<int>(WebEngineError::CertificateErrorDomain) == static_cast<int>(QQuickWebEngineView::CertificateErrorDomain));
 Q_STATIC_ASSERT(static_cast<int>(WebEngineError::DnsErrorDomain) == static_cast<int>(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<QtWebEngineCore::WebContentsAdapter>
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<QtWebEngineCore::WebContentsAdapter>
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<bool>("rfcInvalid");
+    QTest::addColumn<bool>("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<HttpServer> 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<QString>("urlPath");
+    QTest::addColumn<bool>("loadSucceed");
+    QTest::addColumn<bool>("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<QByteArray> 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("<html></html>"));
+        }
+        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<bool>("rfcInvalid");
-    QTest::addColumn<bool>("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<HttpServer> 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