Fix crash if createWindow returns this for AddNewContents

For a QWebEnginePage subclass where createWindow returns the this-pointer, we
get a crash in WebContentsDelegateQt::AddNewContents because AddNewContents
expects the adapter to be adopted already, but QWebEnginePage delays adoption.

Revert 3855015600 by getting rid of the delayed adoption code path. It's not
needed anymore with the delayed initialization of the WebContentsAdapter.

Revert 8a4091c210 by forcing adoption of the adapter even when it doesn't have
contents. This no longer results in a crash since OpenURLFromTab ensures that
the adapter is initialized before use. However, it does result in a behavior
change since return-this now consistently overrides the adapter, so, e.g.
navigation history is now always cleared whereas previously it was only cleared
by the AddNewContents code path. Fixed with new approach in next patch.

Fixes: QTBUG-80596
Change-Id: I4d2230c1bffcf2d77fa59ded9be51da49a820474
Reviewed-by: Kirill Burtsev <kirill.burtsev@qt.io>

3 files changed, 158 insertions, 22 deletions

diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp
index b267c5dd1..6f0009183 100644
--- a/src/webenginewidgets/api/qwebenginepage.cpp
+++ b/src/webenginewidgets/api/qwebenginepage.cpp
@@ -358,25 +358,6 @@ void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer<WebContentsAdapter> ne
     if (!newPage)
         return;
 
-    if (newPage->d_func() == this) {
-        // If createWindow returns /this/ we must delay the adoption.
-        Q_ASSERT(q == newPage);
-        // WebContents might be null if we just opened a new page for navigation, in that case
-        // avoid referencing newWebContents so that it is deleted and WebContentsDelegateQt::OpenURLFromTab
-        // will fall back to navigating current page.
-        if (newWebContents->webContents()) {
-            QTimer::singleShot(0, q, [this, newPage, newWebContents, initialGeometry] () {
-                adoptNewWindowImpl(newPage, newWebContents, initialGeometry);
-            });
-        }
-    } else {
-        adoptNewWindowImpl(newPage, newWebContents, initialGeometry);
-    }
-}
-
-void QWebEnginePagePrivate::adoptNewWindowImpl(QWebEnginePage *newPage,
-        const QSharedPointer<WebContentsAdapter> &newWebContents, const QRect &initialGeometry)
-{
     // 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
diff --git a/src/webenginewidgets/api/qwebenginepage_p.h b/src/webenginewidgets/api/qwebenginepage_p.h
index f37413b8e..ae845c530 100644
--- a/src/webenginewidgets/api/qwebenginepage_p.h
+++ b/src/webenginewidgets/api/qwebenginepage_p.h
@@ -113,9 +113,6 @@ public:
     void focusContainer() override;
     void unhandledKeyEvent(QKeyEvent *event) override;
     void adoptNewWindow(QSharedPointer<QtWebEngineCore::WebContentsAdapter> newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &initialGeometry, const QUrl &targetUrl) override;
-    void adoptNewWindowImpl(QWebEnginePage *newPage,
-            const QSharedPointer<QtWebEngineCore::WebContentsAdapter> &newWebContents,
-            const QRect &initialGeometry);
     bool isBeingAdopted() override;
     void close() override;
     void windowCloseRejected() override;
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index a6b3e0905..a638b7183 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -198,6 +198,8 @@ private Q_SLOTS:
     void dataURLFragment();
     void devTools();
     void openLinkInDifferentProfile();
+    void openLinkInNewPage_data();
+    void openLinkInNewPage();
     void triggerActionWithoutMenu();
     void dynamicFrame();
 
@@ -3379,6 +3381,162 @@ void tst_QWebEnginePage::openLinkInDifferentProfile()
     QVERIFY(spy2.takeFirst().value(0).toBool());
 }
 
+// What does createWindow do?
+enum class OpenLinkInNewPageDecision {
+    // Returns nullptr,
+    ReturnNull,
+    // Returns this,
+    ReturnSelf,
+    // Returns page != this
+    ReturnOther,
+};
+
+// What causes createWindow to be called?
+enum class OpenLinkInNewPageCause {
+    // User clicks on a link with target=_blank.
+    TargetBlank,
+    // User clicks with MiddleButton.
+    MiddleClick,
+};
+
+// What happens after createWindow?
+enum class OpenLinkInNewPageEffect {
+    // The navigation request disappears into the ether.
+    Blocked,
+    // The navigation request becomes a navigation in the original page.
+    LoadInSelf,
+    // The navigation request becomes a navigation in a different page.
+    LoadInOther,
+};
+
+Q_DECLARE_METATYPE(OpenLinkInNewPageCause)
+Q_DECLARE_METATYPE(OpenLinkInNewPageDecision)
+Q_DECLARE_METATYPE(OpenLinkInNewPageEffect)
+
+void tst_QWebEnginePage::openLinkInNewPage_data()
+{
+    using Decision = OpenLinkInNewPageDecision;
+    using Cause = OpenLinkInNewPageCause;
+    using Effect = OpenLinkInNewPageEffect;
+
+    QTest::addColumn<Decision>("decision");
+    QTest::addColumn<Cause>("cause");
+    QTest::addColumn<Effect>("effect");
+
+    // Note that the meaning of returning nullptr from createWindow is not
+    // consistent between the TargetBlank and MiddleClick scenarios.
+    //
+    // With TargetBlank, the open-in-new-page disposition comes from the HTML
+    // target attribute; something the user is probably not aware of. Returning
+    // nullptr is interpreted as a decision by the app to block an unwanted
+    // popup.
+    //
+    // With MiddleClick, the open-in-new-page disposition comes from the user's
+    // explicit intent. Returning nullptr is then interpreted as a failure by
+    // the app to fulfill this intent, which we try to compensate by ignoring
+    // the disposition and performing the navigation request normally.
+
+    QTest::newRow("BlockPopup")     << Decision::ReturnNull  << Cause::TargetBlank << Effect::Blocked;
+    QTest::newRow("IgnoreIntent")   << Decision::ReturnNull  << Cause::MiddleClick << Effect::LoadInSelf;
+    QTest::newRow("OverridePopup")  << Decision::ReturnSelf  << Cause::TargetBlank << Effect::LoadInSelf;
+    QTest::newRow("OverrideIntent") << Decision::ReturnSelf  << Cause::MiddleClick << Effect::LoadInSelf;
+    QTest::newRow("AcceptPopup")    << Decision::ReturnOther << Cause::TargetBlank << Effect::LoadInOther;
+    QTest::newRow("AcceptIntent")   << Decision::ReturnOther << Cause::MiddleClick << Effect::LoadInOther;
+}
+
+void tst_QWebEnginePage::openLinkInNewPage()
+{
+    using Decision = OpenLinkInNewPageDecision;
+    using Cause = OpenLinkInNewPageCause;
+    using Effect = OpenLinkInNewPageEffect;
+
+    class Page : public QWebEnginePage
+    {
+    public:
+        Page *targetPage = nullptr;
+        QSignalSpy spy{this, &QWebEnginePage::loadFinished};
+        Page(QWebEngineProfile *profile) : QWebEnginePage(profile) {}
+    private:
+        QWebEnginePage *createWindow(WebWindowType) override { return targetPage; }
+    };
+
+    class View : public QWebEngineView
+    {
+    public:
+        View(Page *page)
+        {
+            resize(500, 500);
+            setPage(page);
+        }
+    };
+
+    QFETCH(Decision, decision);
+    QFETCH(Cause, cause);
+    QFETCH(Effect, effect);
+
+    QWebEngineProfile profile;
+    Page page1(&profile);
+    Page page2(&profile);
+    View view1(&page1);
+    View view2(&page2);
+
+    view1.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&view1));
+
+    page1.setHtml("<html><body>"
+                  "<a id='link' href='data:,hello' target='_blank'>link</a>"
+                  "</body></html>");
+    QTRY_COMPARE(page1.spy.count(), 1);
+    QVERIFY(page1.spy.takeFirst().value(0).toBool());
+
+    switch (decision) {
+    case Decision::ReturnNull:
+        page1.targetPage = nullptr;
+        break;
+    case Decision::ReturnSelf:
+        page1.targetPage = &page1;
+        break;
+    case Decision::ReturnOther:
+        page1.targetPage = &page2;
+        break;
+    }
+
+    Qt::MouseButton button;
+    switch (cause) {
+    case Cause::TargetBlank:
+        button = Qt::LeftButton;
+        break;
+    case Cause::MiddleClick:
+        button = Qt::MiddleButton;
+        break;
+    }
+    QTest::mouseClick(view1.focusProxy(), button, {}, elementCenter(&page1, "link"));
+
+    switch (effect) {
+    case Effect::Blocked:
+        // Nothing to test
+        break;
+    case Effect::LoadInSelf:
+        QTRY_COMPARE(page1.spy.count(), 1);
+        QVERIFY(page1.spy.takeFirst().value(0).toBool());
+        QCOMPARE(page2.spy.count(), 0);
+        if (decision == Decision::ReturnSelf)
+            // History was discarded
+            QCOMPARE(page1.history()->count(), 1);
+        else
+            QCOMPARE(page1.history()->count(), 2);
+        QCOMPARE(page2.history()->count(), 0);
+        break;
+    case Effect::LoadInOther:
+        QTRY_COMPARE(page2.spy.count(), 1);
+        QVERIFY(page2.spy.takeFirst().value(0).toBool());
+        QCOMPARE(page1.spy.count(), 0);
+        QCOMPARE(page1.history()->count(), 1);
+        QCOMPARE(page2.history()->count(), 1);
+        break;
+    }
+}
+
 void tst_QWebEnginePage::triggerActionWithoutMenu()
 {
     // Calling triggerAction should not crash even when for