Stabilize load signals emitting

Make the WebContentsDelegateQt::EmitLoadStarted() and the
WebContentsDelegateQt::EmitLoadFinished() independent from
the WebContentsDelegateQt::LoadProgressChanged() by removing
m_lastLoadProgress.

Adapt the WebContentsDelegateQt::LoadProgressChanged() to send
signal only if load is in progress.

Add a new test based on the bugreport.

Fix qmltests::WebEngineViewSource::test_viewSourceURL() flaky tests.

Fixes: QTBUG-65223
Fixes: QTBUG-87089
Change-Id: I90af4d2e85105dba801beb8102991eb4ef14c6a3
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: zombori

src/core/web_contents_delegate_qt.cpp

@@ -106,7 +106,6 @@ WebContentsDelegateQt::WebContentsDelegateQt(content::WebContents *webContents,
     : m_viewClient(adapterClient)
     , m_faviconManager(new FaviconManager(webContents, adapterClient))
     , m_findTextHelper(new FindTextHelper(webContents, adapterClient))
-    , m_lastLoadProgress(-1)
     , m_loadingState(determineLoadingState(webContents))
     , m_didStartLoadingSeen(m_loadingState == LoadingState::Loading)
     , m_frameFocusedObserver(adapterClient)
@@ -259,14 +258,14 @@ void WebContentsDelegateQt::CloseContents(content::WebContents *source)
 
 void WebContentsDelegateQt::LoadProgressChanged(double progress)
 {
-    if (!m_loadingErrorFrameList.isEmpty())
-        return;
-    if (m_lastLoadProgress < 0) // suppress signals that aren't between loadStarted and loadFinished
+    QUrl current_url(m_viewClient->webContentsAdapter()->getNavigationEntryOriginalUrl(m_viewClient->webContentsAdapter()->currentNavigationEntryIndex()));
+    int p = qMin(qRound(progress * 100), 100);
+
+    if (!m_loadingErrorFrameList.isEmpty() || !m_loadProgressMap.contains(current_url) || m_loadProgressMap[current_url] == 100 || p ==  100)
         return;
 
-    int p = qMin(qRound(progress * 100), 100);
-    if (p > m_lastLoadProgress) { // ensure strict monotonic increase
-        m_lastLoadProgress = p;
+    if (p > m_loadProgressMap[current_url]) { // ensure strict monotonic increase
+        m_loadProgressMap[current_url] = p;
         m_viewClient->loadProgressChanged(p);
     }
 }
@@ -341,12 +340,32 @@ void WebContentsDelegateQt::RenderViewHostChanged(content::RenderViewHost *, con
 
 void WebContentsDelegateQt::EmitLoadStarted(const QUrl &url, bool isErrorPage)
 {
-    if (m_lastLoadProgress >= 0 && m_lastLoadProgress < 100) // already running
-        return;
     m_viewClient->loadStarted(url, isErrorPage);
     m_viewClient->updateNavigationActions();
+
+    if ((url.hasFragment() || m_lastLoadedUrl.hasFragment())
+        && url.adjusted(QUrl::RemoveFragment) == m_lastLoadedUrl.adjusted(QUrl::RemoveFragment)
+        && !m_isNavigationCommitted) {
+        m_loadProgressMap.insert(url, 100);
+        m_lastLoadedUrl = url;
+        m_viewClient->loadProgressChanged(100);
+        return;
+    }
+
+    if (!m_loadProgressMap.isEmpty()) {
+        QMap<QUrl, int>::iterator it = m_loadProgressMap.begin();
+        while (it != m_loadProgressMap.end()) {
+            if (it.value() == 100) {
+                it = m_loadProgressMap.erase(it);
+                continue;
+            }
+            ++it;
+        }
+    }
+
+    m_lastLoadedUrl = url;
+    m_loadProgressMap.insert(url, 0);
     m_viewClient->loadProgressChanged(0);
-    m_lastLoadProgress = 0;
 }
 
 void WebContentsDelegateQt::DidStartNavigation(content::NavigationHandle *navigation_handle)
@@ -364,11 +383,15 @@ void WebContentsDelegateQt::DidStartNavigation(content::NavigationHandle *naviga
 
 void WebContentsDelegateQt::EmitLoadFinished(bool success, const QUrl &url, bool isErrorPage, int errorCode, const QString &errorDescription)
 {
-    if (m_lastLoadProgress < 0) // not currently running
+    // When error page enabled we don't need to send the error page load finished signal
+    if (m_loadProgressMap[url] == 100)
         return;
-    if (m_lastLoadProgress < 100)
-        m_viewClient->loadProgressChanged(100);
-    m_lastLoadProgress = -1;
+
+    m_lastLoadedUrl = url;
+    m_loadProgressMap[url] = 100;
+    m_isNavigationCommitted = false;
+    m_viewClient->loadProgressChanged(100);
+
     m_viewClient->loadFinished(success, url, isErrorPage, errorCode, errorDescription);
     m_viewClient->updateNavigationActions();
 }
@@ -393,8 +416,10 @@ void WebContentsDelegateQt::DidFinishNavigation(content::NavigationHandle *navig
                 profileAdapter->visitedLinksManager()->addUrl(url);
         }
 
+        m_isNavigationCommitted = true;
         EmitLoadCommitted();
     }
+
     // Success is reported by DidFinishLoad, but DidFailLoad is now dead code and needs to be handled below
     if (navigation_handle->GetNetErrorCode() == net::OK)
         return;

src/core/web_contents_delegate_qt.h

@@ -222,7 +222,6 @@ class WebContentsDelegateQt : public content::WebContentsDelegate
     SavePageInfo m_savePageInfo;
     QSharedPointer<FilePickerController> m_filePickerController;
     QUrl m_initialTargetUrl;
-    int m_lastLoadProgress;
     LoadingState m_loadingState;
     bool m_didStartLoadingSeen;
     FrameFocusedObserver m_frameFocusedObserver;
@@ -234,6 +233,9 @@ class WebContentsDelegateQt : public content::WebContentsDelegate
     int m_desktopStreamCount = 0;
     mutable bool m_pendingUrlUpdate = false;
 
+    QMap<QUrl, int> m_loadProgressMap;
+    QUrl m_lastLoadedUrl;
+    bool m_isNavigationCommitted = false;
     base::WeakPtrFactory<WebContentsDelegateQt> m_weakPtrFactory { this };
 };
 

tests/auto/quick/qmltests/BLACKLIST

@@ -301,8 +301,8 @@ TestWebEngineView {
             // In-page navigation.
             webEngineView.url = Qt.resolvedUrl("test4.html#content");
             // In-page navigation doesn't trigger load succeeded, wait for load progress instead.
+            tryCompare(loadRequestArray, "length", 3);
             tryCompare(webEngineView, "loadProgress", 100);
-            compare(loadRequestArray.length, 3);
             compare(loadRequestArray[2].status, WebEngineView.LoadStartedStatus);
 
             // Load after in-page navigation.

tests/auto/quick/qmltests/data/tst_loadUrl.qml

@@ -94,36 +94,6 @@ TestWebEngineView {
             compare(webEngineView.url, "view-source:" + Qt.resolvedUrl("test1.html"));
         }
 
-        function test_viewSourceURL_data() {
-            var testLocalUrl = "view-source:" + Qt.resolvedUrl("test1.html");
-            var testLocalUrlWithoutScheme = "view-source:" + Qt.resolvedUrl("test1.html").substring(7);
-
-            return [
-                   { tag: "view-source:", userInputUrl: "view-source:", loadSucceed: true, url: "view-source:", title: "view-source:" },
-                   { tag: "view-source:about:blank", userInputUrl: "view-source:about:blank", loadSucceed: true, url: "view-source:about:blank", title: "view-source:about:blank" },
-                   { tag: testLocalUrl, userInputUrl: testLocalUrl, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
-                   { tag: testLocalUrlWithoutScheme, userInputUrl: testLocalUrlWithoutScheme, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
-                   { tag: "view-source:http://non.existent", userInputUrl: "view-source:http://non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
-                   { tag: "view-source:non.existent", userInputUrl: "view-source:non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
-            ];
-        }
-
-        function test_viewSourceURL(row) {
-            WebEngine.settings.errorPageEnabled = true
-            webEngineView.url = row.userInputUrl;
-
-            if (row.loadSucceed) {
-                tryCompare(webEngineView, "loadStatus", WebEngineView.LoadSucceededStatus);
-            } else {
-                tryCompare(webEngineView, "loadStatus", WebEngineView.LoadFailedStatus, 15000);
-            }
-            tryVerify(function() { return titleChangedSpy.count == 1; });
-
-            compare(webEngineView.url, row.url);
-            tryCompare(webEngineView, "title", row.title);
-            verify(!webEngineView.action(WebEngineView.ViewSource).enabled);
-        }
-
         function test_viewSourceCredentials() {
             var url = "http://user:[email protected]/basic-auth/user/passwd";
 

tests/auto/quick/qmltests/data/tst_viewSource.qml

@@ -0,0 +1,133 @@
+/****************************************************************************
+**
+** Copyright (C) 2020 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the QtWebEngine module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:GPL-EXCEPT$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+import QtQuick 2.0
+import QtTest 1.0
+import QtWebEngine 1.4
+import QtWebEngine.testsupport 1.0
+import "../../qmltests/data" 1.0
+
+TestWebEngineView {
+    id: webEngineView
+    width: 200
+    height: 400
+
+    property var viewRequest: null
+    property var loadRequestArray: []
+
+    testSupport: WebEngineTestSupport {
+        errorPage.onLoadingChanged: {
+            loadRequestArray.push({
+               "status": loadRequest.status,
+            })
+        }
+    }
+
+    onLoadingChanged: {
+        loadRequestArray.push({
+            "status": loadRequest.status,
+        });
+    }
+
+    SignalSpy {
+        id: newViewRequestedSpy
+        target: webEngineView
+        signalName: "newViewRequested"
+    }
+
+    SignalSpy {
+        id: titleChangedSpy
+        target: webEngineView
+        signalName: "titleChanged"
+    }
+
+    onNewViewRequested: {
+        viewRequest = {
+            "destination": request.destination,
+            "userInitiated": request.userInitiated
+        };
+
+        request.openIn(webEngineView);
+    }
+
+    TestCase {
+        id: test
+        name: "WebEngineViewSource"
+
+        function init() {
+            webEngineView.loadStatus = null;
+            webEngineView.url = Qt.resolvedUrl("test1.html");
+            tryCompare(webEngineView, "loadStatus", WebEngineView.LoadSucceededStatus);
+            webEngineView.loadStatus = null;
+
+            newViewRequestedSpy.clear();
+            titleChangedSpy.clear();
+            viewRequest = null;
+        }
+
+        function test_viewSourceURL_data() {
+            var testLocalUrl = "view-source:" + Qt.resolvedUrl("test1.html");
+            var testLocalUrlWithoutScheme = "view-source:" + Qt.resolvedUrl("test1.html").substring(7);
+
+            return [
+                   { tag: "view-source:", userInputUrl: "view-source:", loadSucceed: true, url: "view-source:", title: "view-source:" },
+                   { tag: "view-source:about:blank", userInputUrl: "view-source:about:blank", loadSucceed: true, url: "view-source:about:blank", title: "view-source:about:blank" },
+                   { tag: testLocalUrl, userInputUrl: testLocalUrl, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
+                   { tag: testLocalUrlWithoutScheme, userInputUrl: testLocalUrlWithoutScheme, loadSucceed: true, url: testLocalUrl, title: "test1.html" },
+                   { tag: "view-source:http://non.existent", userInputUrl: "view-source:http://non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
+                   { tag: "view-source:non.existent", userInputUrl: "view-source:non.existent", loadSucceed: false, url: "http://non.existent/", title: "non.existent" },
+            ];
+        }
+
+        function test_viewSourceURL(row) {
+            loadRequestArray = [];
+            WebEngine.settings.errorPageEnabled = true
+            webEngineView.url = row.userInputUrl;
+
+            if (row.loadSucceed) {
+                tryVerify(function() { return loadRequestArray.length >= 2 });
+                compare(loadRequestArray[1].status, WebEngineView.LoadSucceededStatus);
+            } else {
+                tryVerify(function() { return loadRequestArray.length >= 2 });
+                compare(loadRequestArray[1].status, WebEngineView.LoadFailedStatus);
+                tryVerify(function() { return loadRequestArray.length == 4 });
+                compare(loadRequestArray[3].status, WebEngineView.LoadSucceededStatus);
+            }
+            tryVerify(function() { return titleChangedSpy.count == 1; });
+
+            compare(webEngineView.url, row.url);
+            tryCompare(webEngineView, "title", row.title);
+            if (row.loadSucceed) {
+                verify(!webEngineView.action(WebEngineView.ViewSource).enabled);
+            } else {
+                verify(webEngineView.action(WebEngineView.ViewSource).enabled);
+            }
+        }
+    }
+}
+

tests/auto/quick/qmltests2/data/tst_viewSoure.qml

@@ -33,6 +33,7 @@ OTHER_FILES += \
     $$PWD/data/tst_linkHovered.qml \
     $$PWD/data/tst_loadFail.qml \
     $$PWD/data/tst_mouseClick.qml \
+    $$PWD/data/tst_viewSource.qml \
     $$PWD/data/icons/favicon.png \
     $$PWD/data/icons/gray128.png \
     $$PWD/data/icons/gray16.png \

tests/auto/quick/qmltests2/qmltests2.pro

@@ -54,6 +54,7 @@ bool HttpServer::start()
 {
     m_error = false;
     m_expectingError = false;
+    m_ignoreNewConnection = false;
 
     if (!m_tcpServer->listen()) {
         qCWarning(gHttpServerLog).noquote() << m_tcpServer->errorString();
@@ -84,6 +85,9 @@ QUrl HttpServer::url(http://github.com/const%20QString%20&path) const
 
 void HttpServer::handleNewConnection()
 {
+    if (m_ignoreNewConnection)
+        return;
+
     auto rr = new HttpReqRep(m_tcpServer->nextPendingConnection(), this);
     connect(rr, &HttpReqRep::requestReceived, [this, rr]() {
         Q_EMIT newRequest(rr);
@@ -122,5 +126,9 @@ void HttpServer::handleNewConnection()
                                             << error;
         m_error = true;
     });
-    connect(rr, &HttpReqRep::closed, rr, &QObject::deleteLater);
+
+    if (!m_tcpServer->isListening()) {
+        m_ignoreNewConnection = true;
+        connect(rr, &HttpReqRep::closed, rr, &QObject::deleteLater);
+    }
 }

tests/auto/shared/httpserver.cpp

@@ -90,6 +90,7 @@ private Q_SLOTS:
     QUrl m_url;
     QStringList m_dirs;
     bool m_error = false;
+    bool m_ignoreNewConnection = false;
     bool m_expectingError = false;
 };
 

tests/auto/shared/httpserver.h

@@ -52,6 +52,7 @@ private Q_SLOTS:
     void secondLoadForError_WhenErrorPageEnabled();
     void loadAfterInPageNavigation_qtbug66869();
     void fileDownloadDoesNotTriggerLoadSignals_qtbug66661();
+    void numberOfStartedAndFinishedSignalsIsSame();
 
 private:
     QWebEngineProfile profile;
@@ -243,5 +244,44 @@ void tst_LoadSignals::fileDownloadDoesNotTriggerLoadSignals_qtbug66661()
     QCOMPARE(loadFinishedSpy.size(), 1);
 }
 
+void tst_LoadSignals::numberOfStartedAndFinishedSignalsIsSame() {
+
+    HttpServer server;
+    server.setResourceDirs({ TESTS_SOURCE_DIR "/qwebengineprofile/resources" });
+    connect(&server, &HttpServer::newRequest, [] (HttpReqRep *) {
+         QTest::qWait(250); // just add delay to trigger some progress for every sub resource
+    });
+    QVERIFY(server.start());
+
+    view.load(server.url("/hedgehog.png"));
+    QTRY_COMPARE(loadFinishedSpy.size(), 1);
+    QVERIFY(loadFinishedSpy[0][0].toBool());
+
+    loadStartedSpy.clear();
+    loadFinishedSpy.clear();
+    loadProgressSpy.clear();
+
+    view.page()->setHtml("<html><body>"
+                         "<img src=\"" + server.url("/hedgehog.png").toEncoded() + "\">"
+                         "<form method='GET' name='hiddenform' action='qrc:///resources/page1.html' />"
+                         "<script language='javascript'>document.forms[0].submit();</script>"
+                         "</body></html>");
+
+    QTRY_COMPARE(loadStartedSpy.size(), 2);
+    QTRY_COMPARE(loadFinishedSpy.size(), 2);
+
+    QTRY_VERIFY(!loadFinishedSpy[0][0].toBool());
+    QTRY_VERIFY(loadFinishedSpy[1][0].toBool());
+
+    view.page()->setHtml("<html><body>"
+                         "<form method='GET' name='hiddenform' action='qrc:///resources/page1.html' />"
+                         "<script language='javascript'>document.forms[0].submit();</script>"
+                         "</body></html>");
+    QTRY_COMPARE(loadStartedSpy.size(), 4);
+    QTRY_COMPARE(loadFinishedSpy.size(), 4);
+    QVERIFY(loadFinishedSpy[2][0].toBool());
+    QVERIFY(loadFinishedSpy[3][0].toBool());
+}
+
 QTEST_MAIN(tst_LoadSignals)
 #include "tst_loadsignals.moc"