Rework url changed logic

Due security changes to prevent url spoofing, our implementation
is getting extra invalidate url requests. Unfortunately, this breaks our
url handling, which now gets lots of new back and fort url changed signals and
make several unit test failures. After tedious investigation of Chromium
omnibox handing and trying out different approaches, it seems that
only sensible solution is to follow Chromium logic and make
NavigationStateChanged to update 'ui' in asynchronous matter.

This change tries not break any tests and simplify url handling.
The only side effect of this change is that WebEnginePage::setContent will
get extra 'url' signal of initial 'urlData' and later 'baseUrl' change
is emitted.

Fix one of qml tests which did not expect to have url on LoadStartedStatus.

Task-number: QTBUG-63388
Task-number: QTBUG-48995
Change-Id: Id347f4325c036e16bfae7bf2f694905e0f21f8d7
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

4 files changed, 28 insertions, 31 deletions

diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp
index 4cfcf6acd..8cc8179cf 100644
--- a/src/core/web_contents_adapter.cpp
+++ b/src/core/web_contents_adapter.cpp
@@ -710,8 +710,6 @@ void WebContentsAdapter::load(const QWebEngineHttpRequest &request)
         if (!adapter)
             return;
         adapter->webContents()->GetController().LoadURLWithParams(params);
-        // Follow chrome::Navigate and invalidate the URL immediately.
-        adapter->m_webContentsDelegate->NavigationStateChanged(adapter->webContents(), content::INVALIDATE_TYPE_URL);
         adapter->focusIfNecessary();
     };
 
@@ -769,7 +767,7 @@ void WebContentsAdapter::save(const QString &filePath, int savePageFormat)
 QUrl WebContentsAdapter::activeUrl() const
 {
     CHECK_INITIALIZED(QUrl());
-    return m_webContentsDelegate->url();
+    return m_webContentsDelegate->url(webContents());
 }
 
 QUrl WebContentsAdapter::requestedUrl() const
@@ -1894,7 +1892,7 @@ void WebContentsAdapter::discard()
     // Based on TabLifecycleUnitSource::TabLifecycleUnit::FinishDiscard
 
     if (m_webContents->IsLoading()) {
-        m_webContentsDelegate->didFailLoad(m_webContentsDelegate->url(), net::Error::ERR_ABORTED,
+        m_webContentsDelegate->didFailLoad(m_webContentsDelegate->url(webContents()), net::Error::ERR_ABORTED,
                                            QStringLiteral("Discarded"));
     }
 
diff --git a/src/core/web_contents_adapter_client.h b/src/core/web_contents_adapter_client.h
index 75fb112c6..e2f667358 100644
--- a/src/core/web_contents_adapter_client.h
+++ b/src/core/web_contents_adapter_client.h
@@ -452,7 +452,7 @@ public:
     virtual void recommendedStateChanged(LifecycleState) = 0;
     virtual void visibleChanged(bool) = 0;
     virtual void titleChanged(const QString&) = 0;
-    virtual void urlChanged(const QUrl&) = 0;
+    virtual void urlChanged() = 0;
     virtual void iconChanged(const QUrl&) = 0;
     virtual void loadProgressChanged(int progress) = 0;
     virtual void didUpdateTargetURL(const QUrl&) = 0;
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index 2a89556cf..38f139513 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -189,29 +189,10 @@ static bool shouldUseActualURL(content::NavigationEntry *entry)
 
 void WebContentsDelegateQt::NavigationStateChanged(content::WebContents* source, content::InvalidateTypes changed_flags)
 {
-    if (changed_flags & content::INVALIDATE_TYPE_URL) {
-        content::NavigationEntry *entry = source->GetController().GetVisibleEntry();
-
-        QUrl newUrl;
-        if (source->GetVisibleURL().SchemeIs(content::kViewSourceScheme)) {
-            Q_ASSERT(entry);
-            GURL url = entry->GetURL();
-
-            // Strip user name, password and reference section from view-source URLs
-            if (url.has_password() || url.has_username() || url.has_ref()) {
-                GURL strippedUrl = net::SimplifyUrlForRequest(entry->GetURL());
-                newUrl = QUrl(QString("%1:%2").arg(content::kViewSourceScheme, QString::fromStdString(strippedUrl.spec())));
-            }
-        }
-
-        // If there is a visible entry there are special cases when we dont wan't to use the actual URL
-        if (entry && newUrl.isEmpty())
-            newUrl = shouldUseActualURL(entry) ? toQt(entry->GetURL()) : toQt(entry->GetVirtualURL());
-
-        if (m_url != newUrl) {
-            m_url = newUrl;
-            m_viewClient->urlChanged(m_url);
-        }
+    if (changed_flags & content::INVALIDATE_TYPE_URL && !m_pendingUrlUpdate) {
+        m_pendingUrlUpdate = true;
+        base::WeakPtr<WebContentsDelegateQt> delegate = AsWeakPtr();
+        QTimer::singleShot(0, [delegate, this](){ if (delegate) m_viewClient->urlChanged();});
     }
 
     if (changed_flags & content::INVALIDATE_TYPE_TITLE) {
@@ -232,6 +213,25 @@ void WebContentsDelegateQt::NavigationStateChanged(content::WebContents* source,
     }
 }
 
+QUrl WebContentsDelegateQt::url(content::WebContents* source) const {
+
+    content::NavigationEntry *entry = source->GetController().GetVisibleEntry();
+    QUrl newUrl;
+    if (entry) {
+        GURL url = entry->GetURL();
+        // Strip user name, password and reference section from view-source URLs
+        if (source->GetVisibleURL().SchemeIs(content::kViewSourceScheme) &&
+            (url.has_password() || url.has_username() || url.has_ref())) {
+            GURL strippedUrl = net::SimplifyUrlForRequest(url);
+            newUrl = QUrl(QString("%1:%2").arg(content::kViewSourceScheme, QString::fromStdString(strippedUrl.spec())));
+        }
+        // If there is a visible entry there are special cases when we dont wan't to use the actual URL
+        if (newUrl.isEmpty())
+            newUrl = shouldUseActualURL(entry) ? toQt(url) : toQt(entry->GetVirtualURL());
+    }
+    m_pendingUrlUpdate = false;
+    return newUrl;
+}
 void WebContentsDelegateQt::AddNewContents(content::WebContents* source, std::unique_ptr<content::WebContents> new_contents, WindowOpenDisposition disposition, const gfx::Rect& initial_pos, bool user_gesture, bool* was_blocked)
 {
     Q_UNUSED(source)
@@ -810,7 +810,6 @@ WebContentsAdapter *WebContentsDelegateQt::webContentsAdapter() const
 
 void WebContentsDelegateQt::copyStateFrom(WebContentsDelegateQt *source)
 {
-    m_url = source->m_url;
     m_title = source->m_title;
     NavigationStateChanged(web_contents(), content::INVALIDATE_TYPE_URL);
     m_faviconManager->copyStateFrom(source->m_faviconManager.data());
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index ba8c6b5a1..33fd49b3d 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -114,7 +114,7 @@ public:
     WebContentsDelegateQt(content::WebContents*, WebContentsAdapterClient *adapterClient);
     ~WebContentsDelegateQt();
 
-    QUrl url() const { return m_url; }
+    QUrl url(content::WebContents *source) const;
     QString title() const { return m_title; }
 
     // WebContentsDelegate overrides
@@ -223,12 +223,12 @@ private:
     bool m_didStartLoadingSeen;
     FrameFocusedObserver m_frameFocusedObserver;
 
-    QUrl m_url;
     QString m_title;
     int m_audioStreamCount = 0;
     int m_videoStreamCount = 0;
     int m_mirroringStreamCount = 0;
     int m_desktopStreamCount = 0;
+    mutable bool m_pendingUrlUpdate = false;
 
     base::WeakPtrFactory<WebContentsDelegateQt> m_weakPtrFactory { this };
 };