Fix handling of new window request

Fixes heap-use-after-free for WebContentsAdapter, which is replaced in
the case, when new window set to be opened and adopted by the same page,
which triggered this request: for example, when 'this' is returned by
'createWindow' override. Achieve this by scheduling 'deleteLater' on an
old adapter. This was already implemented that way for internal
'adoptWebContents', but was overlooked for page's 'createWindow' API. So
just unify handling logic. Also, adapt 'customUserAgentInNewTab' test,
since adopting existing WebContents from different profile is not
supposed to work, and now enforced by the check in 'adoptWebContents'.
Unfortunately, test should also be blacklisted, since it's appeared that
custom user agent is still not reliably set for newly created window.

Task-number: QTBUG-76249
Fixes: QTBUG-94772
Change-Id: Ic9dff33eae99cc242a294d45a92be96306cef93d
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
(cherry picked from commit e04d8c65b350146fc4458ded5576c4a07601d041)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

6 files changed, 103 insertions, 70 deletions

diff --git a/src/core/api/qwebenginepage.cpp b/src/core/api/qwebenginepage.cpp
index f2c903638..84b909bbe 100644
--- a/src/core/api/qwebenginepage.cpp
+++ b/src/core/api/qwebenginepage.cpp
@@ -353,20 +353,8 @@ QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> newWebC
         if (!newWebContents->webContents())
             return newPage->d_func()->adapter; // Reuse existing adapter
 
-        // Mark the new page as being in the process of being adopted, so that a second mouse move event
-        // sent by newWebContents->initialize() gets filtered in RenderWidgetHostViewQt::forwardEvent.
-        // The first mouse move event is being sent by q->createWindow(). This is necessary because
-        // Chromium does not get a mouse move acknowledgment message between the two events, and
-        // InputRouterImpl::ProcessMouseAck is not executed, thus all subsequent mouse move events
-        // get coalesced together, and don't get processed at all.
-        // The mouse move events are actually sent as a result of show() being called on
-        // RenderWidgetHostViewQtDelegateWidget, both when creating the window and when initialize is
-        // called.
-        newPage->d_func()->m_isBeingAdopted = true;
-
-        // Overwrite the new page's WebContents with ours.
-        newPage->d_func()->adapter = newWebContents;
-        newWebContents->setClient(newPage->d_func());
+        if (!newPage->d_func()->adoptWebContents(newWebContents.get()))
+            return nullptr;
 
         if (!initialGeometry.isEmpty())
             emit newPage->geometryChangeRequested(initialGeometry);
@@ -411,18 +399,12 @@ private:
     AdapterPtr adapter;
 };
 
-void QWebEnginePagePrivate::adoptWebContents(WebContentsAdapter *webContents)
+bool QWebEnginePagePrivate::adoptWebContents(WebContentsAdapter *webContents)
 {
-    if (!webContents) {
-        qWarning("Trying to open an empty request, it was either already used or was invalidated."
-            "\nYou must complete the request synchronously within the newPageRequested signal handler."
-            " If a view hasn't been adopted before returning, the request will be invalidated.");
-        return;
-    }
-
+    Q_ASSERT(webContents);
     if (webContents->profileAdapter() && profileAdapter() != webContents->profileAdapter()) {
         qWarning("Can not adopt content from a different WebEngineProfile.");
-        return;
+        return false;
     }
 
     m_isBeingAdopted = true;
@@ -434,6 +416,7 @@ void QWebEnginePagePrivate::adoptWebContents(WebContentsAdapter *webContents)
 
     adapter = webContents->sharedFromThis();
     adapter->setClient(this);
+    return true;
 }
 
 bool QWebEnginePagePrivate::isBeingAdopted()
@@ -2342,10 +2325,10 @@ void QWebEnginePage::acceptAsNewWindow(QWebEngineNewWindowRequest &request)
         return;
     }
 
-    if (adapter)
-        d->adoptWebContents(adapter.data());
-    else
+    if (!adapter)
         setUrl(url);
+    else if (!d->adoptWebContents(adapter.data()))
+        return;
 
     QRect geometry = request.requestedGeometry();
     if (!geometry.isEmpty())
diff --git a/src/core/api/qwebenginepage_p.h b/src/core/api/qwebenginepage_p.h
index b406382b9..4862763aa 100644
--- a/src/core/api/qwebenginepage_p.h
+++ b/src/core/api/qwebenginepage_p.h
@@ -197,7 +197,7 @@ public:
     void _q_webActionTriggered(bool checked);
 
     void createNewWindow(WindowOpenDisposition disposition, bool userGesture, const QUrl &targetUrl);
-    void adoptWebContents(QtWebEngineCore::WebContentsAdapter *webContents);
+    bool adoptWebContents(QtWebEngineCore::WebContentsAdapter *webContents);
     QtWebEngineCore::WebContentsAdapter *webContents() { return adapter.data(); }
     void recreateFromSerializedHistory(QDataStream &input);
 
diff --git a/src/webenginequick/api/qquickwebengineview.cpp b/src/webenginequick/api/qquickwebengineview.cpp
index cf1bff708..ae01aad23 100644
--- a/src/webenginequick/api/qquickwebengineview.cpp
+++ b/src/webenginequick/api/qquickwebengineview.cpp
@@ -772,18 +772,12 @@ private:
     AdapterPtr adapter;
 };
 
-void QQuickWebEngineViewPrivate::adoptWebContents(WebContentsAdapter *webContents)
+bool QQuickWebEngineViewPrivate::adoptWebContents(WebContentsAdapter *webContents)
 {
-    if (!webContents) {
-        qWarning("Trying to open an empty request, it was either already used or was invalidated."
-            "\nYou must complete the request synchronously within the newWindowRequested signal handler."
-            " If a view hasn't been adopted before returning, the request will be invalidated.");
-        return;
-    }
-
+    Q_ASSERT(webContents);
     if (webContents->profileAdapter() && profileAdapter() != webContents->profileAdapter()) {
         qWarning("Can not adopt content from a different WebEngineProfile.");
-        return;
+        return false;
     }
 
     m_isBeingAdopted = true;
@@ -795,6 +789,7 @@ void QQuickWebEngineViewPrivate::adoptWebContents(WebContentsAdapter *webContent
 
     adapter = webContents->sharedFromThis();
     adapter->setClient(this);
+    return true;
 }
 
 QQuickWebEngineView::QQuickWebEngineView(QQuickItem *parent)
@@ -1651,10 +1646,11 @@ void QQuickWebEngineView::acceptAsNewWindow(QWebEngineNewWindowRequest *request)
         return;
     }
 
-    if (auto adapter = request->d_ptr->adapter)
-        d->adoptWebContents(adapter.data());
-    else
+    auto adapter = request->d_ptr->adapter;
+    if (!adapter)
         setUrl(request->requestedUrl());
+    else if (!d->adoptWebContents(adapter.data()))
+        return;
 
     request->d_ptr->setHandled();
 }
diff --git a/src/webenginequick/api/qquickwebengineview_p_p.h b/src/webenginequick/api/qquickwebengineview_p_p.h
index 2e70e423d..4647d671e 100644
--- a/src/webenginequick/api/qquickwebengineview_p_p.h
+++ b/src/webenginequick/api/qquickwebengineview_p_p.h
@@ -164,7 +164,7 @@ public:
     void printRequested() override;
     void findTextFinished(const QWebEngineFindTextResult &result) override;
     void updateAction(QQuickWebEngineView::WebAction) const;
-    void adoptWebContents(QtWebEngineCore::WebContentsAdapter *webContents);
+    bool adoptWebContents(QtWebEngineCore::WebContentsAdapter *webContents);
     void setProfile(QQuickWebEngineProfile *profile);
     void updateAdapter();
     void ensureContentsAdapter();
diff --git a/tests/auto/widgets/qwebenginepage/BLACKLIST b/tests/auto/widgets/qwebenginepage/BLACKLIST
index 0c84b8de1..2fb7c4776 100644
--- a/tests/auto/widgets/qwebenginepage/BLACKLIST
+++ b/tests/auto/widgets/qwebenginepage/BLACKLIST
@@ -7,3 +7,6 @@ macos # Can't move cursor (QTBUG-76312)
 
 [acceptNavigationRequestNavigationType]
 b2qt arm
+
+[customUserAgentInNewTab]
+*
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index c118bd718..84075a276 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -233,6 +233,8 @@ private Q_SLOTS:
     void editActionsWithoutSelection();
 
     void customUserAgentInNewTab();
+    void openNewTabInDifferentProfile_data();
+    void openNewTabInDifferentProfile();
     void renderProcessCrashed();
     void renderProcessPid();
     void backgroundColor();
@@ -4572,6 +4574,28 @@ void tst_QWebEnginePage::editActionsWithoutSelection()
     QVERIFY(page->action(QWebEnginePage::Unselect)->isEnabled());
 }
 
+struct PageWithNewWindowHandler : QWebEnginePage
+{
+    QScopedPointer<PageWithNewWindowHandler> newPage;
+    bool handleInSignal;
+    QWebEngineProfile *targetProfile = nullptr;
+    QSignalSpy loadSpy { this, &QWebEnginePage::loadFinished };
+    PageWithNewWindowHandler(QWebEngineProfile *p, bool inSignal = false, QWebEngineProfile *tp = nullptr)
+        : QWebEnginePage(p), handleInSignal(inSignal), targetProfile(tp) {
+        if (handleInSignal)
+            connect(this, &QWebEnginePage::newWindowRequested, this, [this] (QWebEngineNewWindowRequest &r) {
+                newPage.reset(new PageWithNewWindowHandler(targetProfile ? targetProfile : profile(), handleInSignal));
+                newPage->acceptAsNewWindow(r);
+            });
+    }
+    QWebEnginePage *createWindow(WebWindowType) override {
+        if (handleInSignal)
+            return nullptr;
+        newPage.reset(new PageWithNewWindowHandler(targetProfile ? targetProfile : profile(), handleInSignal));
+        return newPage.get();
+    }
+};
+
 void tst_QWebEnginePage::customUserAgentInNewTab()
 {
     HttpServer server;
@@ -4584,55 +4608,82 @@ void tst_QWebEnginePage::customUserAgentInNewTab()
     });
     QVERIFY(server.start());
 
-    class Page : public QWebEnginePage {
-    public:
-        QWebEngineProfile *targetProfile = nullptr;
-        QScopedPointer<QWebEnginePage> newPage;
-        Page(QWebEngineProfile *profile) : QWebEnginePage(profile) {}
-    private:
-        QWebEnginePage *createWindow(WebWindowType) override
-        {
-            newPage.reset(new QWebEnginePage(targetProfile ? targetProfile : profile(), nullptr));
-            return newPage.data();
-        }
-    };
-    QWebEngineProfile profile1, profile2;
-    profile1.setHttpUserAgent(QStringLiteral("custom 1"));
-    profile2.setHttpUserAgent(QStringLiteral("custom 2"));
-    Page page(&profile1);
-    QWebEngineView view;
-    view.resize(500, 500);
-    view.setPage(&page);
-    view.show();
+    QString expectedUserAgent("custom 1");
+    QWebEngineProfile profile;
+    profile.setHttpUserAgent(expectedUserAgent);
+
+    PageWithNewWindowHandler page(&profile);
+    QWebEngineView view; view.resize(500, 500); view.setPage(&page); view.show();
     QVERIFY(QTest::qWaitForWindowExposed(&view));
-    QSignalSpy spy(&page, &QWebEnginePage::loadFinished);
 
     // First check we can get the user-agent passed through normally
     page.setHtml(QString("<html><body><a id='link' target='_blank' href='") +
                  server.url("/test1").toEncoded() +
                  QString("'>link</a></body></html>"));
-    QTRY_COMPARE(spy.count(), 1);
-    QVERIFY(spy.takeFirst().value(0).toBool());
-    QCOMPARE(evaluateJavaScriptSync(&page, QStringLiteral("navigator.userAgent")).toString(), profile1.httpUserAgent());
+    QTRY_COMPARE(page.loadSpy.count(), 1);
+    QVERIFY(page.loadSpy.takeFirst().value(0).toBool());
+    QCOMPARE(evaluateJavaScriptSync(&page, QStringLiteral("navigator.userAgent")).toString(), expectedUserAgent);
     QTest::mouseClick(view.focusProxy(), Qt::LeftButton, {}, elementCenter(&page, "link"));
     QTRY_VERIFY(page.newPage);
     QTRY_VERIFY(!lastUserAgent.isEmpty());
-    QCOMPARE(lastUserAgent, profile1.httpUserAgent().toUtf8());
+    QCOMPARE(lastUserAgent, expectedUserAgent);
+    QCOMPARE(evaluateJavaScriptSync(page.newPage.get(), QStringLiteral("navigator.userAgent")).toString(), expectedUserAgent);
 
     // Now check we can get the new user-agent of the profile
     page.newPage.reset();
-    page.targetProfile = &profile2;
-    spy.clear();
+    expectedUserAgent = "custom 2";
+    profile.setHttpUserAgent(expectedUserAgent);
+    page.loadSpy.clear();
     lastUserAgent = { };
     page.setHtml(QString("<html><body><a id='link' target='_blank' href='") +
                  server.url("/test2").toEncoded() +
                  QString("'>link</a></body></html>"));
-    QTRY_COMPARE(spy.count(), 1);
-    QVERIFY(spy.takeFirst().value(0).toBool());
+    QTRY_COMPARE(page.loadSpy.count(), 1);
+    QVERIFY(page.loadSpy.takeFirst().value(0).toBool());
     QTest::mouseClick(view.focusProxy(), Qt::LeftButton, {}, elementCenter(&page, "link"));
     QTRY_VERIFY(page.newPage);
     QTRY_VERIFY(!lastUserAgent.isEmpty());
-    QCOMPARE(lastUserAgent, profile2.httpUserAgent().toUtf8());
+    QCOMPARE(lastUserAgent, expectedUserAgent);
+    QCOMPARE(evaluateJavaScriptSync(&page, QStringLiteral("navigator.userAgent")).toString(), expectedUserAgent);
+    QCOMPARE(evaluateJavaScriptSync(page.newPage.get(), QStringLiteral("navigator.userAgent")).toString(), expectedUserAgent);
+}
+
+void tst_QWebEnginePage::openNewTabInDifferentProfile_data()
+{
+    QTest::addColumn<bool>("handleInSignal");
+    QTest::addRow("handleInSignal") << true;
+    QTest::addRow("handleInOverride") << false;
+}
+
+void tst_QWebEnginePage::openNewTabInDifferentProfile()
+{
+    QFETCH(bool, handleInSignal);
+
+    HttpServer server;
+    QStringList receivedRequests;
+    connect(&server, &HttpServer::newRequest, [&] (HttpReqRep *r) {
+        receivedRequests.append(r->requestPath());
+        r->setResponseBody("DUMMY");
+        r->sendResponse();
+    });
+    QVERIFY(server.start());
+
+    QWebEngineProfile profile1, profile2;
+    PageWithNewWindowHandler page(&profile1, handleInSignal, &profile2);
+    QWebEngineView view; view.setPage(&page); view.resize(320, 240); view.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&view));
+
+    page.setHtml(QString("<html><body><a id='link' target='_blank' href='%1'>link</a></body></html>").arg(server.url("/first.html").toEncoded()));
+    QTRY_COMPARE(page.loadSpy.count(), 1);
+    QVERIFY(page.loadSpy.takeFirst().value(0).toBool());
+
+    QTest::mouseClick(view.focusProxy(), Qt::LeftButton, {}, elementCenter(&page, "link"));
+    QTRY_VERIFY(page.newPage);
+    QVERIFY(page.profile() == &profile1);
+    QVERIFY(page.newPage->profile() == &profile2);
+    // not load should occur or requests to server issued since web_contents is not expected to be adopted from other profile
+    QTRY_LOOP_IMPL(page.newPage->loadSpy.size() != 0, 1000, 100);
+    QVERIFY2(receivedRequests.isEmpty(), qPrintable(receivedRequests.join(", ")));
 }
 
 void tst_QWebEnginePage::renderProcessCrashed()