Fix active URLRequest tracking in NetworkDelegateQt

NetworkDelegateQt maintains a set of pointers

    QSet<net::URLRequest *> 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 <pvarga@inf.u-szeged.hu>

2 files changed, 117 insertions, 88 deletions

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<QWebEngineUrlRequestInfo::NavigationType>(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<UserData>(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<content::WebContentsImpl*>(webContents)->GetView())->client();
+            client->navigationRequested(m_navigationType,
+                                        m_url,
+                                        navigationRequestAction,
+                                        m_isMainFrameRequest);
+            error = net::ERR_FAILED;
+            switch (static_cast<WebContentsAdapterClient::NavigationRequestAction>(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<content::WebContentsImpl*>(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<net::URLRequest *> 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;