From 31a954a5e8be1ea30336d99a21ad2e76d21cdbcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCri=20Valdmann?= Date: Thu, 8 Mar 2018 14:08:04 +0100 Subject: Fix active URLRequest tracking in NetworkDelegateQt NetworkDelegateQt maintains a set of pointers QSet m_activeRequests Pointers are inserted in OnBeforeURLRequest, checked in CompleteURLRequestOnIOThread, and removed in OnURLRequestDestroyed. This design breaks however if malloc decides to reuse the address of a recently-freed URLRequest for a new one. For example: 1. A new URLRequest is created and passed to OnBeforeURLRequest. Inside this method a pointer is added to m_activeRequests and a task is posted to the UI thread. 2. The URLRequest is destroyed and OnURLRequestDestroyed is called. The pointer is removed from the set. 3. A new URLRequest is created at the same address and again passed to OnBeforeURLRequest. The pointer is added back to the set. 4. The task from step 1 finally returns from the UI thread to the IO thread by executing CompleteURLRequestOnIOThread. This method is supposed to invoke a callback, but only if the original URLRequest hasn't been destroyed yet. So it checks if the URLRequest is still in the m_activeRequests set, sees that it is, and invokes the callback. Of course this does not work since in actuality we are dealing with a completely different URLRequest object that just happens to live at the same address. Fix by changing the tracking to work per-task and not per-URLRequest. The new URLRequestNotification class encapsulates the logic for delivering the notification and completing the request while dealing with potential mid-sequence URLRequest destruction. Change-Id: I0f61df0dccb9cb2b60893cd4d8f1b4bba844a4cd Reviewed-by: Peter Varga --- src/core/network_delegate_qt.cpp | 189 ++++++++++++++++++++++++--------------- src/core/network_delegate_qt.h | 16 ---- 2 files changed, 117 insertions(+), 88 deletions(-) (limited to 'src') diff --git a/src/core/network_delegate_qt.cpp b/src/core/network_delegate_qt.cpp index 0a7d1f2b4..a378584b4 100644 --- a/src/core/network_delegate_qt.cpp +++ b/src/core/network_delegate_qt.cpp @@ -81,6 +81,8 @@ WebContentsAdapterClient::NavigationType pageTransitionToNavigationType(ui::Page } } +namespace { + QWebEngineUrlRequestInfo::ResourceType toQt(content::ResourceType resourceType) { if (resourceType >= 0 && resourceType < content::ResourceType(QWebEngineUrlRequestInfo::ResourceTypeLast)) @@ -93,6 +95,114 @@ QWebEngineUrlRequestInfo::NavigationType toQt(WebContentsAdapterClient::Navigati return static_cast(navigationType); } +// Notifies WebContentsAdapterClient of a new URLRequest. +class URLRequestNotification { +public: + URLRequestNotification(net::URLRequest *request, + const QUrl &url, + bool isMainFrameRequest, + int navigationType, + int frameTreeNodeId, + const net::CompletionCallback &callback) + : m_request(request) + , m_url(url) + , m_isMainFrameRequest(isMainFrameRequest) + , m_navigationType(navigationType) + , m_frameTreeNodeId(frameTreeNodeId) + , m_callback(callback) + { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + + m_request->SetUserData(UserData::key, std::make_unique(this)); + + content::BrowserThread::PostTask( + content::BrowserThread::UI, + FROM_HERE, + base::Bind(&URLRequestNotification::notify, base::Unretained(this))); + } + +private: + // Calls cancel() when the URLRequest is destroyed. + class UserData : public base::SupportsUserData::Data { + public: + UserData(URLRequestNotification *ptr) : m_ptr(ptr) {} + ~UserData() { m_ptr->cancel(); } + static const char key[]; + private: + URLRequestNotification *m_ptr; + }; + + void cancel() + { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + + // May run concurrently with notify() but we only touch m_request here. + + m_request = nullptr; + } + + void notify() + { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + + // May run concurrently with cancel() so no peeking at m_request here. + + int error = net::OK; + content::WebContents *webContents = content::WebContents::FromFrameTreeNodeId(m_frameTreeNodeId); + if (webContents) { + int navigationRequestAction = WebContentsAdapterClient::AcceptRequest; + WebContentsAdapterClient *client = + WebContentsViewQt::from(static_cast(webContents)->GetView())->client(); + client->navigationRequested(m_navigationType, + m_url, + navigationRequestAction, + m_isMainFrameRequest); + error = net::ERR_FAILED; + switch (static_cast(navigationRequestAction)) { + case WebContentsAdapterClient::AcceptRequest: + error = net::OK; + break; + case WebContentsAdapterClient::IgnoreRequest: + error = net::ERR_ABORTED; + break; + } + DCHECK(error != net::ERR_FAILED); + } + + // Run the callback on the IO thread. + content::BrowserThread::PostTask( + content::BrowserThread::IO, + FROM_HERE, + base::Bind(&URLRequestNotification::complete, base::Unretained(this), error)); + } + + void complete(int error) + { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + + if (m_request) { + if (m_request->status().status() != net::URLRequestStatus::CANCELED) + m_callback.Run(error); + m_request->RemoveUserData(UserData::key); + } + + delete this; + } + + ~URLRequestNotification() {} + + net::URLRequest *m_request; + QUrl m_url; + bool m_isMainFrameRequest; + int m_navigationType; + int m_frameTreeNodeId; + net::CompletionCallback m_callback; +}; + +const char URLRequestNotification::UserData::key[] = "QtWebEngineCore::URLRequestNotification"; + +} // namespace + NetworkDelegateQt::NetworkDelegateQt(URLRequestContextGetterQt *requestContext) : m_requestContextGetter(requestContext) { @@ -149,92 +259,27 @@ int NetworkDelegateQt::OnBeforeURLRequest(net::URLRequest *request, const net::C if (!content::IsResourceTypeFrame(resourceType) || frameTreeNodeId == -1) return net::OK; - // Track active requests since |callback| and |new_url| are valid - // only until OnURLRequestDestroyed is called for this request. - m_activeRequests.insert(request); - - RequestParams params = { + new URLRequestNotification( + request, qUrl, resourceInfo->IsMainFrame(), - navigationType - }; - - content::BrowserThread::PostTask( - content::BrowserThread::UI, - FROM_HERE, - base::Bind(&NetworkDelegateQt::NotifyNavigationRequestedOnUIThread, - base::Unretained(this), - request, - params, - frameTreeNodeId, - callback) - ); + navigationType, + frameTreeNodeId, + callback + ); // We'll run the callback after we notified the UI thread. return net::ERR_IO_PENDING; } -void NetworkDelegateQt::OnURLRequestDestroyed(net::URLRequest* request) +void NetworkDelegateQt::OnURLRequestDestroyed(net::URLRequest*) { - m_activeRequests.remove(request); } void NetworkDelegateQt::OnCompleted(net::URLRequest */*request*/, bool /*started*/, int /*net_error*/) { } -void NetworkDelegateQt::CompleteURLRequestOnIOThread(net::URLRequest *request, - int navigationRequestAction, - const net::CompletionCallback &callback) -{ - Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); - if (!m_activeRequests.contains(request)) - return; - - if (request->status().status() == net::URLRequestStatus::CANCELED) - return; - - int error = net::OK; - switch (navigationRequestAction) { - case WebContentsAdapterClient::AcceptRequest: - error = net::OK; - break; - case WebContentsAdapterClient::IgnoreRequest: - error = net::ERR_ABORTED; - break; - default: - error = net::ERR_FAILED; - Q_UNREACHABLE(); - } - callback.Run(error); -} - -void NetworkDelegateQt::NotifyNavigationRequestedOnUIThread(net::URLRequest *request, - RequestParams params, - int frameTreeNodeId, - const net::CompletionCallback &callback) -{ - Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - - int navigationRequestAction = WebContentsAdapterClient::AcceptRequest; - content::WebContents *webContents = content::WebContents::FromFrameTreeNodeId(frameTreeNodeId); - if (webContents) { - WebContentsAdapterClient *client = WebContentsViewQt::from(static_cast(webContents)->GetView())->client(); - client->navigationRequested(params.navigationType, params.url, navigationRequestAction, params.isMainFrameRequest); - } - - // Run the callback on the IO thread. - content::BrowserThread::PostTask( - content::BrowserThread::IO, - FROM_HERE, - base::Bind(&NetworkDelegateQt::CompleteURLRequestOnIOThread, - base::Unretained(this), - request, - navigationRequestAction, - callback) - ); -} - bool NetworkDelegateQt::OnCanSetCookie(const net::URLRequest& request, const std::string& cookie_line, net::CookieOptions*) diff --git a/src/core/network_delegate_qt.h b/src/core/network_delegate_qt.h index 44cf96d5b..21e00c710 100644 --- a/src/core/network_delegate_qt.h +++ b/src/core/network_delegate_qt.h @@ -55,26 +55,10 @@ namespace QtWebEngineCore { class URLRequestContextGetterQt; class NetworkDelegateQt : public net::NetworkDelegate { - QSet m_activeRequests; URLRequestContextGetterQt *m_requestContextGetter; public: NetworkDelegateQt(URLRequestContextGetterQt *requestContext); - struct RequestParams { - QUrl url; - bool isMainFrameRequest; - int navigationType; - }; - - void NotifyNavigationRequestedOnUIThread(net::URLRequest *request, - RequestParams params, - int frameTreeNodeId, - const net::CompletionCallback &callback); - - void CompleteURLRequestOnIOThread(net::URLRequest *request, - int navigationRequestAction, - const net::CompletionCallback &callback); - // net::NetworkDelegate implementation int OnBeforeURLRequest(net::URLRequest* request, const net::CompletionCallback& callback, GURL* newUrl) override; void OnURLRequestDestroyed(net::URLRequest* request) override; -- cgit v1.2.3