From 445235bc012fcdc73acc881f865a21769c46a6d3 Mon Sep 17 00:00:00 2001 From: Michal Klocek Date: Wed, 18 Dec 2019 20:31:13 +0100 Subject: 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 --- src/core/web_contents_adapter.cpp | 6 +-- src/core/web_contents_adapter_client.h | 2 +- src/core/web_contents_delegate_qt.cpp | 47 +++++++++++----------- src/core/web_contents_delegate_qt.h | 4 +- src/webengine/api/qquickwebengineview.cpp | 31 +++++++------- src/webengine/api/qquickwebengineview_p_p.h | 2 +- src/webenginewidgets/api/qwebenginepage.cpp | 18 +++++---- src/webenginewidgets/api/qwebenginepage_p.h | 4 +- tests/auto/quick/qmltests/data/tst_loadUrl.qml | 2 +- .../widgets/qwebenginepage/tst_qwebenginepage.cpp | 11 ++--- 10 files changed, 65 insertions(+), 62 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 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 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 m_weakPtrFactory { this }; }; diff --git a/src/webengine/api/qquickwebengineview.cpp b/src/webengine/api/qquickwebengineview.cpp index b05bbfdbc..83ada3c11 100644 --- a/src/webengine/api/qquickwebengineview.cpp +++ b/src/webengine/api/qquickwebengineview.cpp @@ -369,11 +369,14 @@ void QQuickWebEngineViewPrivate::titleChanged(const QString &title) Q_EMIT q->titleChanged(); } -void QQuickWebEngineViewPrivate::urlChanged(const QUrl &url) +void QQuickWebEngineViewPrivate::urlChanged() { Q_Q(QQuickWebEngineView); - Q_UNUSED(url); - Q_EMIT q->urlChanged(); + QUrl url = adapter->activeUrl(); + if (m_url != url) { + m_url = url; + Q_EMIT q->urlChanged(); + } } void QQuickWebEngineViewPrivate::iconChanged(const QUrl &url) @@ -885,7 +888,7 @@ void QQuickWebEngineViewPrivate::initializationFinished() emit q->titleChanged(); emit q->urlChanged(); emit q->iconChanged(); - QQuickWebEngineLoadRequest loadRequest(adapter->activeUrl(), QQuickWebEngineView::LoadSucceededStatus); + QQuickWebEngineLoadRequest loadRequest(m_url, QQuickWebEngineView::LoadSucceededStatus); emit q->loadingChanged(&loadRequest); emit q->loadProgressChanged(); @@ -1017,9 +1020,6 @@ void QQuickWebEngineViewPrivate::updateEditActions() QUrl QQuickWebEngineView::url() const { Q_D(const QQuickWebEngineView); - if (d->adapter->isInitialized()) - return d->adapter->activeUrl(); - else return d->m_url; } @@ -1029,13 +1029,15 @@ void QQuickWebEngineView::setUrl(const QUrl& url) if (url.isEmpty()) return; + if (d->m_url != url) { + d->m_url = url; + d->m_html.clear(); + emit urlChanged(); + } + if (d->adapter->isInitialized()) { d->adapter->load(url); - return; } - - d->m_url = url; - d->m_html.clear(); } QUrl QQuickWebEngineView::icon() const @@ -1147,14 +1149,13 @@ void QQuickWebEngineViewPrivate::updateAdapter() { // When the profile changes we need to create a new WebContentAdapter and reload the active URL. bool wasInitialized = adapter->isInitialized(); - QUrl activeUrl = adapter->activeUrl(); adapter = QSharedPointer::create(); adapter->setClient(this); if (wasInitialized) { if (!m_html.isEmpty()) - adapter->setContent(m_html.toUtf8(), defaultMimeType, activeUrl); - else if (activeUrl.isValid()) - adapter->load(activeUrl); + adapter->setContent(m_html.toUtf8(), defaultMimeType, m_url); + else if (m_url.isValid()) + adapter->load(m_url); else adapter->loadDefault(); } diff --git a/src/webengine/api/qquickwebengineview_p_p.h b/src/webengine/api/qquickwebengineview_p_p.h index df6843ac3..a2ae86f91 100644 --- a/src/webengine/api/qquickwebengineview_p_p.h +++ b/src/webengine/api/qquickwebengineview_p_p.h @@ -106,7 +106,7 @@ public: void recommendedStateChanged(LifecycleState state) override; void visibleChanged(bool visible) override; void titleChanged(const QString&) override; - void urlChanged(const QUrl&) override; + void urlChanged() override; void iconChanged(const QUrl&) override; void loadProgressChanged(int progress) override; void didUpdateTargetURL(const QUrl&) override; diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp index 958238c9d..33a7721e7 100644 --- a/src/webenginewidgets/api/qwebenginepage.cpp +++ b/src/webenginewidgets/api/qwebenginepage.cpp @@ -230,11 +230,14 @@ void QWebEnginePagePrivate::titleChanged(const QString &title) Q_EMIT q->titleChanged(title); } -void QWebEnginePagePrivate::urlChanged(const QUrl &url) +void QWebEnginePagePrivate::urlChanged() { Q_Q(QWebEnginePage); - explicitUrl = QUrl(); - Q_EMIT q->urlChanged(url); + QUrl qurl = adapter->activeUrl(); + if (url != qurl) { + url = qurl; + Q_EMIT q->urlChanged(qurl); + } } void QWebEnginePagePrivate::iconChanged(const QUrl &url) @@ -313,8 +316,6 @@ void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isE } isLoading = false; - if (success) - explicitUrl = QUrl(); // 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)) { @@ -2077,14 +2078,17 @@ QString QWebEnginePage::title() const void QWebEnginePage::setUrl(const QUrl &url) { Q_D(QWebEnginePage); - d->explicitUrl = url; + if (d->url != url) { + d->url = url; + emit urlChanged(url); + } load(url); } QUrl QWebEnginePage::url() const { Q_D(const QWebEnginePage); - return d->explicitUrl.isValid() ? d->explicitUrl : d->adapter->activeUrl(); + return d->url; } QUrl QWebEnginePage::requestedUrl() const diff --git a/src/webenginewidgets/api/qwebenginepage_p.h b/src/webenginewidgets/api/qwebenginepage_p.h index 2843f69c4..e78b0f926 100644 --- a/src/webenginewidgets/api/qwebenginepage_p.h +++ b/src/webenginewidgets/api/qwebenginepage_p.h @@ -97,7 +97,7 @@ public: void recommendedStateChanged(LifecycleState state) override; void visibleChanged(bool visible) override; void titleChanged(const QString&) override; - void urlChanged(const QUrl&) override; + void urlChanged() override; void iconChanged(const QUrl&) override; void loadProgressChanged(int progress) override; void didUpdateTargetURL(const QUrl&) override; @@ -186,7 +186,7 @@ public: QWebEngineProfile *profile; QWebEngineSettings *settings; QWebEngineView *view; - QUrl explicitUrl; + QUrl url; QWebEngineContextMenuData contextData; bool isLoading; QWebEngineScriptCollection scriptCollection; diff --git a/tests/auto/quick/qmltests/data/tst_loadUrl.qml b/tests/auto/quick/qmltests/data/tst_loadUrl.qml index ec5c965ea..7bdd0c761 100644 --- a/tests/auto/quick/qmltests/data/tst_loadUrl.qml +++ b/tests/auto/quick/qmltests/data/tst_loadUrl.qml @@ -230,7 +230,7 @@ TestWebEngineView { compare(loadRequest.activeUrl, aboutBlank); loadRequest = loadRequestArray[2]; compare(loadRequest.status, WebEngineView.LoadStartedStatus); - compare(loadRequest.activeUrl, aboutBlank); + compare(loadRequest.activeUrl, "data:text/html;charset=UTF-8,load failed"); compare(loadRequest.url, "data:text/html;charset=UTF-8,load failed") loadRequest = loadRequestArray[3]; compare(loadRequest.status, WebEngineView.LoadSucceededStatus); diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp index 53b846bb5..0f087b7fb 100644 --- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp +++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp @@ -1914,7 +1914,8 @@ void tst_QWebEnginePage::urlChange() QUrl testUrl("http://test.qt.io/"); m_view->setHtml(QStringLiteral("

Testload(urlToLoad1); - QCOMPARE(m_page->url(), urlToLoad1); - QCOMPARE(m_page->requestedUrl(), urlToLoad1); + QTRY_COMPARE(m_page->url(), urlToLoad1); + QTRY_COMPARE(m_page->requestedUrl(), urlToLoad1); // baseUrlSync spins an event loop and this sometimes return the next result. // QCOMPARE(baseUrlSync(m_page), baseUrl); QTRY_COMPARE(startedSpy.count(), 2); @@ -2726,8 +2727,8 @@ void tst_QWebEnginePage::setUrlThenLoads() QCOMPARE(baseUrlSync(m_page), extractBaseUrl(urlToLoad1)); m_page->load(urlToLoad2); - QCOMPARE(m_page->url(), urlToLoad2); - QCOMPARE(m_page->requestedUrl(), urlToLoad2); + QTRY_COMPARE(m_page->url(), urlToLoad2); + QTRY_COMPARE(m_page->requestedUrl(), urlToLoad2); QCOMPARE(baseUrlSync(m_page), extractBaseUrl(urlToLoad1)); QTRY_COMPARE(startedSpy.count(), 3); -- cgit v1.2.3