Stop preserving aborted navigation entries

Consider the scenario

  1. user enters url "http://localhost:8000/"

     A new navigation entry is created and committed in the
     NavigationController.

  2. user enters url "http://localhost:8000/download.bin"

     A new navigation entry is created and a download is triggered, but the
     pending navigation entry in the NavigationController is neither committed
     nor discarded (since our WebContentsDelegate's ShouldPreserveAbortedURLs()
     returns true).

  3. user enters url "http://localhost:8000/download.bin"

     At this point the NavigationController will have "http://localhost:8000/"
     as the committed navigation entry and "http://localhost:8000/download.bin"
     as the pending entry. NavigateToPendingEntry will see that the user is
     trying to navigate again to the same URL as the last pending entry and will
     therefore identify this new navigation as a reload.

     However Blink interprets 'reload' to mean reloading the last committed
     entry, i.e. "http://localhost:8000/", and so we end up trying to download
     "http://localhost:8000/" instead of "http://localhost:8000/download.bin" as
     the user might have expected.

The patch removes the ShouldPreserveAbortedURLs override and relies on the
default implementation which always returns false. As a result the pending
navigation entry in step 2 above is discarded once the download has been
triggered and the unexpected behavior in step 3 is no longer triggered.

Removing the override resurrects QTBUG-48995 where, for example, calling
QWebEnginePage::setUrl triggers first a urlChanged signal for the *old* URL. The
patch adds url and title properties to WebContentsDelegateQt so that property
change signals are triggered only if the properties have actually changed. A
consequence of this fix is that the first urlChanged signal is delivered
directly from the setUrl/load method and not asynchronously once the loading
starts (this is also how Chrome's URL bar is updated).

Task-number: QTBUG-63388
Change-Id: Icfa300b165e5e56f1fbc8978a00a237c263df183
Reviewed-by: Peter Varga <pvarga@inf.u-szeged.hu>

6 files changed, 121 insertions, 57 deletions

diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp
index effd6e340..ed13883c0 100644
--- a/src/core/web_contents_adapter.cpp
+++ b/src/core/web_contents_adapter.cpp
@@ -594,6 +594,8 @@ void WebContentsAdapter::load(const QWebEngineHttpRequest &request)
     }
 
     d->webContents->GetController().LoadURLWithParams(params);
+    // Follow chrome::Navigate and invalidate the URL immediately.
+    d->webContentsDelegate->NavigationStateChanged(d->webContents.get(), content::INVALIDATE_TYPE_URL);
     focusIfNecessary();
 }
 
@@ -635,7 +637,7 @@ void WebContentsAdapter::save(const QString &filePath, int savePageFormat)
 QUrl WebContentsAdapter::activeUrl() const
 {
     Q_D(const WebContentsAdapter);
-    return toQt(d->webContents->GetLastCommittedURL());
+    return d->webContentsDelegate->url();
 }
 
 QUrl WebContentsAdapter::requestedUrl() const
@@ -670,7 +672,7 @@ QUrl WebContentsAdapter::iconUrl() const
 QString WebContentsAdapter::pageTitle() const
 {
     Q_D(const WebContentsAdapter);
-    return toQt(d->webContents->GetTitle());
+    return d->webContentsDelegate->title();
 }
 
 QString WebContentsAdapter::selectedText() const
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index f35620c86..96c7c4c41 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -131,10 +131,21 @@ content::WebContents *WebContentsDelegateQt::OpenURLFromTab(content::WebContents
 
 void WebContentsDelegateQt::NavigationStateChanged(content::WebContents* source, content::InvalidateTypes changed_flags)
 {
-    if (changed_flags & content::INVALIDATE_TYPE_URL)
-        m_viewClient->urlChanged(toQt(source->GetVisibleURL()));
-    if (changed_flags & content::INVALIDATE_TYPE_TITLE)
-        m_viewClient->titleChanged(toQt(source->GetTitle()));
+    if (changed_flags & content::INVALIDATE_TYPE_URL) {
+        QUrl newUrl = toQt(source->GetVisibleURL());
+        if (m_url != newUrl) {
+            m_url = newUrl;
+            m_viewClient->urlChanged(m_url);
+        }
+    }
+
+    if (changed_flags & content::INVALIDATE_TYPE_TITLE) {
+        QString newTitle = toQt(source->GetTitle());
+        if (m_title != newTitle) {
+            m_title = newTitle;
+            m_viewClient->titleChanged(m_title);
+        }
+    }
 
     // NavigationStateChanged gets called with INVALIDATE_TYPE_TAB by AudioStateProvider::Notify,
     // whenever an audio sound gets played or stopped, this is the only way to actually figure out
@@ -146,20 +157,6 @@ void WebContentsDelegateQt::NavigationStateChanged(content::WebContents* source,
     }
 }
 
-bool WebContentsDelegateQt::ShouldPreserveAbortedURLs(content::WebContents *source)
-{
-    Q_UNUSED(source)
-
-    // Allow failed URLs to stick around in the URL bar, but only when the error-page is enabled.
-    WebEngineSettings *settings = m_viewClient->webEngineSettings();
-    bool isErrorPageEnabled = settings->testAttribute(settings->Attribute::ErrorPageEnabled);
-
-    if (isErrorPageEnabled)
-        return true;
-
-    return false;
-}
-
 void WebContentsDelegateQt::AddNewContents(content::WebContents* source, content::WebContents* new_contents, WindowOpenDisposition disposition, const gfx::Rect& initial_pos, bool user_gesture, bool* was_blocked)
 {
     Q_UNUSED(source)
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index 44fb93093..8440ec053 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -97,6 +97,9 @@ public:
     void setLastSearchedString(const QString &s) { m_lastSearchedString = s; }
     int lastReceivedFindReply() const { return m_lastReceivedFindReply; }
 
+    QUrl url() const { return m_url; }
+    QString title() const { return m_title; }
+
     // WebContentsDelegate overrides
     content::WebContents *OpenURLFromTab(content::WebContents *source, const content::OpenURLParams &params) override;
     void NavigationStateChanged(content::WebContents* source, content::InvalidateTypes changed_flags) override;
@@ -119,7 +122,6 @@ public:
     bool IsPopupOrPanel(const content::WebContents *source) const override;
     void UpdateTargetURL(content::WebContents* source, const GURL& url) override;
     void RequestToLockMouse(content::WebContents *web_contents, bool user_gesture, bool last_unlocked_by_target) override;
-    bool ShouldPreserveAbortedURLs(content::WebContents *source) override;
     void ShowValidationMessage(content::WebContents *web_contents, const gfx::Rect &anchor_in_root_view, const base::string16 &main_text, const base::string16 &sub_text) override;
     void HideValidationMessage(content::WebContents *web_contents) override;
     void MoveValidationMessage(content::WebContents *web_contents, const gfx::Rect &anchor_in_root_view) override;
@@ -165,6 +167,9 @@ private:
     QSharedPointer<FilePickerController> m_filePickerController;
     QUrl m_initialTargetUrl;
     int m_lastLoadProgress;
+
+    QUrl m_url;
+    QString m_title;
 };
 
 } // namespace QtWebEngineCore
diff --git a/tests/auto/quick/qmltests/data/tst_viewSource.qml b/tests/auto/quick/qmltests/data/tst_viewSource.qml
index 22c3947d3..576035ef2 100644
--- a/tests/auto/quick/qmltests/data/tst_viewSource.qml
+++ b/tests/auto/quick/qmltests/data/tst_viewSource.qml
@@ -63,7 +63,7 @@ TestWebEngineView {
         name: "WebEngineViewSource"
 
         function init() {
-            webEngineView.url = Qt.resolvedUrl("about:blank");
+            webEngineView.url = Qt.resolvedUrl("test1.html");
             verify(webEngineView.waitForLoadSucceeded());
 
             newViewRequestedSpy.clear();
@@ -103,8 +103,8 @@ TestWebEngineView {
                    { 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: "view-source:http://non.existent/", title: "non.existent" },
-                   { tag: "view-source:non.existent", userInputUrl: "view-source:non.existent", loadSucceed: false, url: "view-source:http://non.existent/", title: "non.existent" },
+                   { 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" },
             ];
         }
 
@@ -114,11 +114,10 @@ TestWebEngineView {
 
             if (row.loadSucceed) {
                 verify(webEngineView.waitForLoadSucceeded());
-                tryVerify(function() { return titleChangedSpy.count >= 1; });
             } else {
                 verify(webEngineView.waitForLoadFailed());
-                tryVerify(function() { return titleChangedSpy.count >= 2; });
             }
+            tryVerify(function() { return titleChangedSpy.count == 1; });
 
             compare(webEngineView.url, row.url);
             tryCompare(webEngineView, "title", row.title);
diff --git a/tests/auto/widgets/qwebenginedownloads/tst_qwebenginedownloads.cpp b/tests/auto/widgets/qwebenginedownloads/tst_qwebenginedownloads.cpp
index 9531d0cf4..7094c8e4b 100644
--- a/tests/auto/widgets/qwebenginedownloads/tst_qwebenginedownloads.cpp
+++ b/tests/auto/widgets/qwebenginedownloads/tst_qwebenginedownloads.cpp
@@ -27,6 +27,7 @@
 ****************************************************************************/
 
 #include <QCoreApplication>
+#include <QSignalSpy>
 #include <QStandardPaths>
 #include <QTemporaryDir>
 #include <QTest>
@@ -37,6 +38,18 @@
 #include <httpserver.h>
 #include <waitforsignal.h>
 
+static std::unique_ptr<HttpReqRep> waitForFaviconRequest(HttpServer *server)
+{
+    auto rr = waitForRequest(server);
+    if (!rr ||
+        rr->requestMethod() != QByteArrayLiteral("GET") ||
+        rr->requestPath() != QByteArrayLiteral("/favicon.ico"))
+        return nullptr;
+    rr->setResponseStatus(404);
+    rr->sendResponse();
+    return std::move(rr);
+}
+
 class tst_QWebEngineDownloads : public QObject
 {
     Q_OBJECT
@@ -46,6 +59,7 @@ private Q_SLOTS:
     void downloadTwoLinks();
     void downloadPage_data();
     void downloadPage();
+    void downloadViaSetUrl();
 };
 
 enum DownloadTestUserAction {
@@ -317,12 +331,8 @@ void tst_QWebEngineDownloads::downloadLink()
     QVERIFY(loadOk);
 
     // 1.1. Ignore favicon request
-    auto favIconRR = waitForRequest(&server);
+    auto favIconRR = waitForFaviconRequest(&server);
     QVERIFY(favIconRR);
-    QCOMPARE(favIconRR->requestMethod(), QByteArrayLiteral("GET"));
-    QCOMPARE(favIconRR->requestPath(), QByteArrayLiteral("/favicon.ico"));
-    favIconRR->setResponseStatus(404);
-    favIconRR->sendResponse();
 
     // 2. Simulate user action
     //
@@ -439,12 +449,8 @@ void tst_QWebEngineDownloads::downloadTwoLinks()
     QVERIFY(waitForSignal(&page, &QWebEnginePage::loadFinished, [&](bool ok){ loadOk = ok; }));
     QVERIFY(loadOk);
 
-    auto favIconRR = waitForRequest(&server);
+    auto favIconRR = waitForFaviconRequest(&server);
     QVERIFY(favIconRR);
-    QCOMPARE(favIconRR->requestMethod(), QByteArrayLiteral("GET"));
-    QCOMPARE(favIconRR->requestPath(), QByteArrayLiteral("/favicon.ico"));
-    favIconRR->setResponseStatus(404);
-    favIconRR->sendResponse();
 
     QTRY_VERIFY(view.focusWidget());
     QWidget *renderWidget = view.focusWidget();
@@ -541,12 +547,8 @@ void tst_QWebEngineDownloads::downloadPage()
     QVERIFY(waitForSignal(&page, &QWebEnginePage::loadFinished, [&](bool ok){ loadOk = ok; }));
     QVERIFY(loadOk);
 
-    auto favIconRR = waitForRequest(&server);
+    auto favIconRR = waitForFaviconRequest(&server);
     QVERIFY(favIconRR);
-    QCOMPARE(favIconRR->requestMethod(), QByteArrayLiteral("GET"));
-    QCOMPARE(favIconRR->requestPath(), QByteArrayLiteral("/favicon.ico"));
-    favIconRR->setResponseStatus(404);
-    favIconRR->sendResponse();
 
     QTemporaryDir tmpDir;
     QVERIFY(tmpDir.isValid());
@@ -593,5 +595,70 @@ void tst_QWebEngineDownloads::downloadPage()
     QVERIFY(file.exists());
 }
 
+void tst_QWebEngineDownloads::downloadViaSetUrl()
+{
+    // Reproduce the scenario described in QTBUG-63388 by triggering downloads
+    // of the same file multiple times via QWebEnginePage::setUrl
+
+    HttpServer server;
+    QWebEngineProfile profile;
+    QWebEnginePage page(&profile);
+    QSignalSpy loadSpy(&page, &QWebEnginePage::loadFinished);
+    QSignalSpy urlSpy(&page, &QWebEnginePage::urlChanged);
+    const QUrl indexUrl = server.url();
+    const QUrl fileUrl = server.url(QByteArrayLiteral("/file"));
+
+    // Set up the test scenario by trying to load some unrelated HTML.
+
+    page.setUrl(indexUrl);
+
+    auto indexRR = waitForRequest(&server);
+    QVERIFY(indexRR);
+    QCOMPARE(indexRR->requestMethod(), QByteArrayLiteral("GET"));
+    QCOMPARE(indexRR->requestPath(), QByteArrayLiteral("/"));
+    indexRR->setResponseHeader(QByteArrayLiteral("content-type"), QByteArrayLiteral("text/html"));
+    indexRR->setResponseBody(QByteArrayLiteral("<html><body>Hello</body></html>"));
+    indexRR->sendResponse();
+
+    auto indexFavRR = waitForFaviconRequest(&server);
+    QVERIFY(indexFavRR);
+
+    QTRY_COMPARE(loadSpy.count(), 1);
+    QTRY_COMPARE(urlSpy.count(), 1);
+    QCOMPARE(loadSpy.takeFirst().value(0).toBool(), true);
+    QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), indexUrl);
+
+    // Download files via setUrl. With QTBUG-63388 after the first iteration the
+    // downloads would be triggered for indexUrl and not fileUrl.
+
+    QVector<QUrl> downloadUrls;
+    QObject::connect(&profile, &QWebEngineProfile::downloadRequested, [&](QWebEngineDownloadItem *item) {
+        downloadUrls.append(item->url());
+    });
+
+    for (int i = 0; i != 3; ++i) {
+        page.setUrl(fileUrl);
+        QCOMPARE(page.url(), fileUrl);
+
+        auto fileRR = waitForRequest(&server);
+        QVERIFY(fileRR);
+        fileRR->setResponseHeader(QByteArrayLiteral("content-disposition"), QByteArrayLiteral("attachment"));
+        fileRR->setResponseBody(QByteArrayLiteral("redacted"));
+        fileRR->sendResponse();
+
+        auto fileFavRR = waitForFaviconRequest(&server);
+        QVERIFY(fileFavRR);
+
+        QTRY_COMPARE(loadSpy.count(), 1);
+        QTRY_COMPARE(urlSpy.count(), 2);
+        QTRY_COMPARE(downloadUrls.count(), 1);
+        QCOMPARE(loadSpy.takeFirst().value(0).toBool(), false);
+        QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), fileUrl);
+        QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), indexUrl);
+        QCOMPARE(downloadUrls.takeFirst(), fileUrl);
+        QCOMPARE(page.url(), indexUrl);
+    }
+}
+
 QTEST_MAIN(tst_QWebEngineDownloads)
 #include "tst_qwebenginedownloads.moc"
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index 35e703bbc..47e83855c 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -3192,21 +3192,19 @@ void tst_QWebEnginePage::progressSignal()
 
 void tst_QWebEnginePage::urlChange()
 {
-    QSignalSpy urlSpy(m_page, SIGNAL(urlChanged(QUrl)));
+    QSignalSpy urlSpy(m_page, &QWebEnginePage::urlChanged);
 
     QUrl dataUrl("data:text/html,<h1>Test");
     m_view->setUrl(dataUrl);
 
-    QVERIFY(urlSpy.wait());
-
-    QCOMPARE(urlSpy.size(), 1);
+    QTRY_COMPARE(urlSpy.size(), 1);
+    QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), dataUrl);
 
     QUrl dataUrl2("data:text/html,<html><head><title>title</title></head><body><h1>Test</body></html>");
     m_view->setUrl(dataUrl2);
 
-    QVERIFY(urlSpy.wait());
-
-    QCOMPARE(urlSpy.size(), 2);
+    QTRY_COMPARE(urlSpy.size(), 1);
+    QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), dataUrl2);
 }
 
 class FakeReply : public QNetworkReply {
@@ -3317,7 +3315,7 @@ void tst_QWebEnginePage::requestedUrlAfterSetAndLoadFailures()
 
     page.load(second);
     QTRY_COMPARE_WITH_TIMEOUT(spy.count(), 2, 12000);
-    QCOMPARE(page.url(), first);
+    QCOMPARE(page.url(), second);
     QCOMPARE(page.requestedUrl(), second);
     QVERIFY(!spy.at(1).first().toBool());
 }
@@ -3850,12 +3848,11 @@ void tst_QWebEnginePage::setUrlToBadDomain()
     page.setUrl(url1);
 
     QTRY_COMPARE(urlSpy.count(), 1);
-    QTRY_COMPARE(titleSpy.count(), 2);
+    QTRY_COMPARE(titleSpy.count(), 1);
     QTRY_COMPARE(loadSpy.count(), 1);
 
     QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), url1);
     QCOMPARE(titleSpy.takeFirst().value(0).toString(), url1.host());
-    QCOMPARE(titleSpy.takeFirst().value(0).toString(), url1.host());
     QCOMPARE(loadSpy.takeFirst().value(0).toBool(), false);
 
     QCOMPARE(page.url(), url1);
@@ -3864,12 +3861,11 @@ void tst_QWebEnginePage::setUrlToBadDomain()
     page.setUrl(url2);
 
     QTRY_COMPARE(urlSpy.count(), 1);
-    QTRY_COMPARE(titleSpy.count(), 2);
+    QTRY_COMPARE(titleSpy.count(), 1);
     QTRY_COMPARE(loadSpy.count(), 1);
 
     QCOMPARE(urlSpy.takeFirst().value(0).toUrl(), url2);
     QCOMPARE(titleSpy.takeFirst().value(0).toString(), url2.host());
-    QCOMPARE(titleSpy.takeFirst().value(0).toString(), url2.host());
     QCOMPARE(loadSpy.takeFirst().value(0).toBool(), false);
 
     QCOMPARE(page.url(), url2);
@@ -4077,9 +4073,8 @@ void tst_QWebEnginePage::setUrlThenLoads()
     const QUrl urlToLoad1("qrc:/resources/test2.html");
     const QUrl urlToLoad2("qrc:/resources/test1.html");
 
-    // Just after first load. URL didn't changed yet.
     m_page->load(urlToLoad1);
-    QCOMPARE(m_page->url(), url);
+    QCOMPARE(m_page->url(), urlToLoad1);
     QCOMPARE(m_page->requestedUrl(), urlToLoad1);
     // baseUrlSync spins an event loop and this sometimes return the next result.
     // QCOMPARE(baseUrlSync(m_page), baseUrl);
@@ -4093,9 +4088,8 @@ void tst_QWebEnginePage::setUrlThenLoads()
     QCOMPARE(m_page->requestedUrl(), urlToLoad1);
     QCOMPARE(baseUrlSync(m_page), extractBaseUrl(urlToLoad1));
 
-    // Just after second load. URL didn't changed yet.
     m_page->load(urlToLoad2);
-    QCOMPARE(m_page->url(), urlToLoad1);
+    QCOMPARE(m_page->url(), urlToLoad2);
     QCOMPARE(m_page->requestedUrl(), urlToLoad2);
     QCOMPARE(baseUrlSync(m_page), extractBaseUrl(urlToLoad1));
     QTRY_COMPARE(startedSpy.count(), 3);