From d4004e30ba9a09f75a810d4734dd6be18616b5e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCri=20Valdmann?= Date: Tue, 21 Apr 2020 17:15:24 +0200 Subject: 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 --- src/webenginewidgets/api/qwebenginepage.cpp | 19 ------------------- 1 file changed, 19 deletions(-) (limited to 'src/webenginewidgets/api/qwebenginepage.cpp') 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 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 &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 -- cgit v1.2.3 From 6c7148af789af401b39f0283f8060624432c5b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCri=20Valdmann?= Date: Thu, 23 Apr 2020 12:55:58 +0200 Subject: Restore behavior of OpenURLFromTab if createWindow returns this Instead of using QSharedPointer's reference count to communicate adoption/non-adoption, change adoptNewWindow to return a adapter pointer, with null meaning non-adoption. Then change QWebEnginePage's implementation to reuse already existing adapters if possible, restoring previous behavior of OpenURLFromTab when createWindow returns this. Task-number: QTBUG-80596 Change-Id: I8ee7c31e4294aabd3207c504cba67d6171c66cb0 Reviewed-by: Allan Sandfeld Jensen --- src/webenginewidgets/api/qwebenginepage.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'src/webenginewidgets/api/qwebenginepage.cpp') diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp index 6f0009183..8e6f402ff 100644 --- a/src/webenginewidgets/api/qwebenginepage.cpp +++ b/src/webenginewidgets/api/qwebenginepage.cpp @@ -348,7 +348,10 @@ void QWebEnginePagePrivate::unhandledKeyEvent(QKeyEvent *event) QGuiApplication::sendEvent(view->parentWidget(), event); } -void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer newWebContents, WindowOpenDisposition disposition, bool userGesture, const QRect &initialGeometry, const QUrl &targetUrl) +QSharedPointer +QWebEnginePagePrivate::adoptNewWindow(QSharedPointer newWebContents, + WindowOpenDisposition disposition, bool userGesture, + const QRect &initialGeometry, const QUrl &targetUrl) { Q_Q(QWebEnginePage); Q_UNUSED(userGesture); @@ -356,7 +359,10 @@ void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer ne QWebEnginePage *newPage = q->createWindow(toWindowType(disposition)); if (!newPage) - return; + return nullptr; + + 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. @@ -375,6 +381,8 @@ void QWebEnginePagePrivate::adoptNewWindow(QSharedPointer ne if (!initialGeometry.isEmpty()) emit newPage->geometryChangeRequested(initialGeometry); + + return newWebContents; } bool QWebEnginePagePrivate::isBeingAdopted() -- cgit v1.2.3