Speculative fix for titleUpdate test case

After tedious investigation the issue boils down to error prone
synchronization of web engine settings. WebEngineSettings are
synchronized between the browser process and the render process.
Moreover in the browser process the sync message is send to the render
with QTimer::singleShot, which can cause race conditions if
for example QWebPage::setUrl was used meanwhile. This makes
current settings not being picked up by the render process and results
in 'titleUpdate' test case flaky. This happens due to the fact that
ShouldDisplayErrorPageForFailedLoad in the render process frame view
could have invalid value.

Try to sync web engine settings on every adapter load, setContent or
reload.

Mark some flaky settings in tests.

Fixes: QTBUG-83078
Change-Id: I5289472f146e104d5cb6c3b9b20b26d3dc42f4b1
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

4 files changed, 14 insertions, 1 deletions

diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp
index 138ea7039..9cecb9ecb 100644
--- a/src/core/web_contents_adapter.cpp
+++ b/src/core/web_contents_adapter.cpp
@@ -614,6 +614,8 @@ void WebContentsAdapter::reload()
     bool wasDiscarded = (m_lifecycleState == LifecycleState::Discarded);
     setLifecycleState(LifecycleState::Active);
     CHECK_VALID_RENDER_WIDGET_HOST_VIEW(m_webContents->GetRenderViewHost());
+    WebEngineSettings *settings = m_adapterClient->webEngineSettings();
+    settings->doApply();
     if (!wasDiscarded) // undiscard() already triggers a reload
         m_webContents->GetController().Reload(content::ReloadType::NORMAL, /*checkRepost = */false);
     focusIfNecessary();
@@ -625,6 +627,8 @@ void WebContentsAdapter::reloadAndBypassCache()
     bool wasDiscarded = (m_lifecycleState == LifecycleState::Discarded);
     setLifecycleState(LifecycleState::Active);
     CHECK_VALID_RENDER_WIDGET_HOST_VIEW(m_webContents->GetRenderViewHost());
+    WebEngineSettings *settings = m_adapterClient->webEngineSettings();
+    settings->doApply();
     if (!wasDiscarded) // undiscard() already triggers a reload
         m_webContents->GetController().Reload(content::ReloadType::BYPASSING_CACHE, /*checkRepost = */false);
     focusIfNecessary();
@@ -655,6 +659,9 @@ void WebContentsAdapter::load(const QWebEngineHttpRequest &request)
 
     CHECK_VALID_RENDER_WIDGET_HOST_VIEW(m_webContents->GetRenderViewHost());
 
+    WebEngineSettings *settings = m_adapterClient->webEngineSettings();
+    settings->doApply();
+
     // The situation can occur when relying on the editingFinished signal in QML to set the url
     // of the WebView.
     // When enter is pressed, onEditingFinished fires and the url of the webview is set, which
@@ -740,6 +747,9 @@ void WebContentsAdapter::setContent(const QByteArray &data, const QString &mimeT
 
     CHECK_VALID_RENDER_WIDGET_HOST_VIEW(m_webContents->GetRenderViewHost());
 
+    WebEngineSettings *settings = m_adapterClient->webEngineSettings();
+    settings->doApply();
+
     QByteArray encodedData = data.toPercentEncoding();
     std::string urlString;
     if (!mimeType.isEmpty())
diff --git a/src/core/web_engine_settings.cpp b/src/core/web_engine_settings.cpp
index eb6db9793..c54570b33 100644
--- a/src/core/web_engine_settings.cpp
+++ b/src/core/web_engine_settings.cpp
@@ -333,6 +333,8 @@ void WebEngineSettings::doApply()
 {
     if (webPreferences.isNull())
         return;
+
+    m_batchTimer.stop();
     // Override with our settings when applicable
     applySettingsToWebPreferences(webPreferences.data());
     Q_ASSERT(m_adapter);
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index 4945f6c91..e1d97af25 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -828,6 +828,7 @@ void tst_QWebEnginePage::localStorageVisibility()
     // Toggle local setting for every page and...
     webPage1.settings()->setAttribute(QWebEngineSettings::LocalStorageEnabled, false);
     webPage2.settings()->setAttribute(QWebEngineSettings::LocalStorageEnabled, true);
+    // TODO: note this setting is flaky, consider settings().commit()
     // ...first check second page (for storage to appear) as applying settings is batched and done asynchronously
     QTRY_VERIFY(evaluateJavaScriptSync(&webPage2, QString("(window.localStorage != undefined)")).toBool());
     // Switching the feature off does not actively remove the object from webPage1.
diff --git a/tests/auto/widgets/qwebenginesettings/tst_qwebenginesettings.cpp b/tests/auto/widgets/qwebenginesettings/tst_qwebenginesettings.cpp
index b4061b984..a09901e69 100644
--- a/tests/auto/widgets/qwebenginesettings/tst_qwebenginesettings.cpp
+++ b/tests/auto/widgets/qwebenginesettings/tst_qwebenginesettings.cpp
@@ -168,7 +168,7 @@ protected:
 
         if (isMainFrame && url.scheme().startsWith("data"))
             settings()->setAttribute(QWebEngineSettings::JavascriptEnabled, true);
-
+            // TODO: note this setting is flaky, consider settings().commit()
         return true;
     }
 };