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 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) (limited to 'src/core/api/qwebenginepage.cpp') 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()) -- cgit v1.2.1