From d590e174e8734ba88748fb60209fbec64a6e6fef Mon Sep 17 00:00:00 2001 From: Kirill Burtsev Date: Thu, 5 Aug 2021 15:59:51 +0200 Subject: 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 (cherry picked from commit e04d8c65b350146fc4458ded5576c4a07601d041) Reviewed-by: Qt Cherry-pick Bot --- src/core/api/qwebenginepage.cpp | 35 ++++++------------------ src/core/api/qwebenginepage_p.h | 2 +- src/webenginequick/api/qquickwebengineview.cpp | 20 ++++++-------- src/webenginequick/api/qquickwebengineview_p_p.h | 2 +- 4 files changed, 19 insertions(+), 40 deletions(-) (limited to 'src') 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 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(); -- cgit v1.2.1