Simplify URLRequestCustomJob handling

Improve implementation of URLRequestCustomJob:

* remove qmutex, pass values using PostTask
* do not use base::WeakPtr which is not thread safe and must
  always be dereferenced on the same thread
* add proxy object to handle interactions between threads
* do not use QPointer to track IODevice since it does not solve
  anything for us
* QIODevice in reply method is used only by IO thread
* do not make shared object to commit suicide,
  instead use refcounted object
* improve documentation about thread safety issue of QIODevice
  object in reply method

Change-Id: Ic29bf262de8082dfd46cb9217a68f3c982d16b9e
Reviewed-by: Leena Miettinen <riitta-leena.miettinen@qt.io>
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

src/core/api/qwebengineurlrequestjob.cpp

@@ -115,10 +115,16 @@ QByteArray QWebEngineUrlRequestJob::requestMethod() const
 
 /*!
     Replies to the request with \a device and the MIME type \a contentType.
+    The user has to be aware that \a device will be used on another thread
+    until the job is deleted. In case simultaneous access from the main thread
+    is desired, the user is reponsible for making access to \a device thread-safe
+    for example by using QMutex. Note that the \a device object is not owned by
+    the web engine. Therefore, the signal QObject::destroyed() of
+    QWebEngineUrlRequestJob must be monitored.
  */
 void QWebEngineUrlRequestJob::reply(const QByteArray &contentType, QIODevice *device)
 {
-    d_ptr->setReply(contentType, device);
+    d_ptr->reply(contentType, device);
 }
 
 /*!

src/core/url_request_custom_job.cpp

@@ -53,47 +53,48 @@ URLRequestCustomJob::URLRequestCustomJob(URLRequest *request,
                                          const std::string &scheme,
                                          QWeakPointer<const BrowserContextAdapter> adapter)
     : URLRequestJob(request, networkDelegate)
-    , m_scheme(scheme)
-    , m_adapter(adapter)
-    , m_proxy(new URLRequestCustomJobProxy(this))
+    , m_proxy(new URLRequestCustomJobProxy(this, scheme, adapter))
+    , m_device(nullptr)
+    , m_error(0)
 {
 }
 
 URLRequestCustomJob::~URLRequestCustomJob()
 {
-    if (m_proxy)
-        m_proxy->killJob();
-}
-
-static void startAsync(URLRequestCustomJobProxy *proxy)
-{
-    proxy->startAsync();
+    m_proxy->m_job = nullptr;
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     base::Bind(&URLRequestCustomJobProxy::release,
+                                     m_proxy));
+    if (m_device && m_device->isOpen())
+        m_device->close();
+    m_device = nullptr;
 }
 
 void URLRequestCustomJob::Start()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-                                     base::Bind(&startAsync, m_proxy));
+                                     base::Bind(&URLRequestCustomJobProxy::initialize,
+                                     m_proxy, request()->url(), request()->method()));
 }
 
 void URLRequestCustomJob::Kill()
 {
-    if (m_proxy)
-        m_proxy->killJob();
-    m_proxy = 0;
-
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    if (m_device && m_device->isOpen())
+        m_device->close();
+    m_device = nullptr;
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     base::Bind(&URLRequestCustomJobProxy::release,
+                                     m_proxy));
     URLRequestJob::Kill();
 }
 
 bool URLRequestCustomJob::GetMimeType(std::string *mimeType) const
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    if (!m_proxy)
-        return false;
-    QMutexLocker lock(&m_proxy->m_mutex);
-    if (m_proxy->m_mimeType.size() > 0) {
-        *mimeType = m_proxy->m_mimeType;
+    if (m_mimeType.size() > 0) {
+        *mimeType = m_mimeType;
         return true;
     }
     return false;
@@ -102,11 +103,8 @@ bool URLRequestCustomJob::GetMimeType(std::string *mimeType) const
 bool URLRequestCustomJob::GetCharset(std::string* charset)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    if (!m_proxy)
-        return false;
-    QMutexLocker lock(&m_proxy->m_mutex);
-    if (m_proxy->m_charset.size() > 0) {
-        *charset = m_proxy->m_charset;
+    if (m_charset.size() > 0) {
+        *charset = m_charset;
         return true;
     }
     return false;
@@ -115,11 +113,8 @@ bool URLRequestCustomJob::GetCharset(std::string* charset)
 bool URLRequestCustomJob::IsRedirectResponse(GURL* location, int* http_status_code)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    if (!m_proxy)
-        return false;
-    QMutexLocker lock(&m_proxy->m_mutex);
-    if (m_proxy->m_redirect.is_valid()) {
-        *location = m_proxy->m_redirect;
+    if (m_redirect.is_valid()) {
+        *location = m_redirect;
         *http_status_code = 303;
         return true;
     }
@@ -129,17 +124,15 @@ bool URLRequestCustomJob::IsRedirectResponse(GURL* location, int* http_status_co
 int URLRequestCustomJob::ReadRawData(IOBuffer *buf, int bufSize)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    Q_ASSERT(m_proxy);
-    QMutexLocker lock(&m_proxy->m_mutex);
-    if (m_proxy->m_error)
-        return m_proxy->m_error;
-    qint64 rv = m_proxy->m_device ? m_proxy->m_device->read(buf->data(), bufSize) : -1;
-    if (rv >= 0)
+    if (m_error)
+        return m_error;
+    qint64 rv = m_device ? m_device->read(buf->data(), bufSize) : -1;
+    if (rv >= 0) {
         return static_cast<int>(rv);
-    else {
+    } else {
         // QIODevice::read might have called fail on us.
-        if (m_proxy->m_error)
-            return m_proxy->m_error;
+        if (m_error)
+            return m_error;
         return ERR_FAILED;
     }
 }

src/core/url_request_custom_job.h

@@ -44,6 +44,8 @@
 #include "url/gurl.h"
 #include <QtCore/QWeakPointer>
 
+QT_FORWARD_DECLARE_CLASS(QIODevice)
+
 namespace QtWebEngineCore {
 
 class BrowserContextAdapter;
@@ -53,7 +55,10 @@ class URLRequestCustomJobProxy;
 // A request job that handles reading custom URL schemes
 class URLRequestCustomJob : public net::URLRequestJob {
 public:
-    URLRequestCustomJob(net::URLRequest *request, net::NetworkDelegate *networkDelegate, const std::string &scheme, QWeakPointer<const BrowserContextAdapter> adapter);
+    URLRequestCustomJob(net::URLRequest *request,
+                        net::NetworkDelegate *networkDelegate,
+                        const std::string &scheme,
+                        QWeakPointer<const BrowserContextAdapter> adapter);
     void Start() override;
     void Kill() override;
     int ReadRawData(net::IOBuffer *buf, int buf_size)  override;
@@ -65,9 +70,12 @@ protected:
     virtual ~URLRequestCustomJob();
 
 private:
-    std::string m_scheme;
-    QWeakPointer<const BrowserContextAdapter> m_adapter;
-    URLRequestCustomJobProxy *m_proxy;
+    scoped_refptr<URLRequestCustomJobProxy> m_proxy;
+    std::string m_mimeType;
+    std::string m_charset;
+    GURL m_redirect;
+    QIODevice *m_device;
+    int m_error;
 
     friend class URLRequestCustomJobProxy;
 

src/core/url_request_custom_job_delegate.cpp

@@ -42,45 +42,54 @@
 
 #include "type_conversion.h"
 #include "net/base/net_errors.h"
+#include "content/public/browser/browser_thread.h"
 
 #include <QByteArray>
 
 namespace QtWebEngineCore {
 
-URLRequestCustomJobDelegate::URLRequestCustomJobDelegate(URLRequestCustomJobProxy *proxy)
-    : m_proxy(proxy)
+URLRequestCustomJobDelegate::URLRequestCustomJobDelegate(URLRequestCustomJobProxy *proxy,
+                                                         const QUrl &url,
+                                                         const QByteArray &method)
+    : m_proxy(proxy),
+      m_request(url),
+      m_method(method)
 {
 }
 
 URLRequestCustomJobDelegate::~URLRequestCustomJobDelegate()
 {
-    m_proxy->unsetJobDelegate();
 }
 
 QUrl URLRequestCustomJobDelegate::url() const
 {
-    return toQt(m_proxy->requestUrl());
+    return m_request;
 }
 
 QByteArray URLRequestCustomJobDelegate::method() const
 {
-    return QByteArray::fromStdString(m_proxy->requestMethod());
+    return m_method;
 }
 
-void URLRequestCustomJobDelegate::setReply(const QByteArray &contentType, QIODevice *device)
+void URLRequestCustomJobDelegate::reply(const QByteArray &contentType, QIODevice *device)
 {
-    m_proxy->setReplyMimeType(contentType.toStdString());
-    m_proxy->setReplyDevice(device);
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                     base::Bind(&URLRequestCustomJobProxy::reply,
+                                                m_proxy,contentType.toStdString(),device));
 }
 
 void URLRequestCustomJobDelegate::abort()
 {
-    m_proxy->abort();
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                     base::Bind(&URLRequestCustomJobProxy::abort,
+                                                m_proxy));
 }
 
 void URLRequestCustomJobDelegate::redirect(const QUrl &url)
 {
-    m_proxy->redirect(toGurl(url));
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                     base::Bind(&URLRequestCustomJobProxy::redirect,
+                                                m_proxy, toGurl(url)));
 }
 
 void URLRequestCustomJobDelegate::fail(Error error)
@@ -105,8 +114,11 @@ void URLRequestCustomJobDelegate::fail(Error error)
         net_error = net::ERR_FAILED;
         break;
     }
-    if (net_error)
-        m_proxy->fail(net_error);
+    if (net_error) {
+        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                         base::Bind(&URLRequestCustomJobProxy::fail,
+                                                    m_proxy, net_error));
+    }
 }
 
 } // namespace

src/core/url_request_custom_job_delegate.h

@@ -40,6 +40,7 @@
 #ifndef URL_REQUEST_CUSTOM_JOB_DELEGATE_H_
 #define URL_REQUEST_CUSTOM_JOB_DELEGATE_H_
 
+#include "base/memory/ref_counted.h"
 #include "qtwebenginecoreglobal.h"
 
 #include <QObject>
@@ -68,17 +69,20 @@ public:
     QUrl url() const;
     QByteArray method() const;
 
-    void setReply(const QByteArray &contentType, QIODevice *device);
+    void reply(const QByteArray &contentType, QIODevice *device);
     void redirect(const QUrl& url);
     void abort();
-
     void fail(Error);
 
 private:
-    URLRequestCustomJobDelegate(URLRequestCustomJobProxy *proxy);
+    URLRequestCustomJobDelegate(URLRequestCustomJobProxy *proxy,
+                                const QUrl &url,
+                                const QByteArray &method);
 
     friend class URLRequestCustomJobProxy;
-    URLRequestCustomJobProxy *m_proxy;
+    scoped_refptr<URLRequestCustomJobProxy> m_proxy;
+    QUrl m_request;
+    QByteArray m_method;
 };
 
 } // namespace

src/core/url_request_custom_job_proxy.cpp

@@ -49,217 +49,115 @@ using namespace net;
 
 namespace QtWebEngineCore {
 
-URLRequestCustomJobProxy::URLRequestCustomJobProxy(URLRequestCustomJob *job)
-    : m_mutex(QMutex::Recursive)
-    , m_job(job)
-    , m_delegate(0)
-    , m_error(0)
+URLRequestCustomJobProxy::URLRequestCustomJobProxy(URLRequestCustomJob *job,
+                                                   const std::string &scheme,
+                                                   QWeakPointer<const BrowserContextAdapter> adapter)
+    : m_job(job)
     , m_started(false)
-    , m_asyncInitialized(false)
-    , m_weakFactory(this)
+    , m_scheme(scheme)
+    , m_delegate(nullptr)
+    , m_adapter(adapter)
 {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 }
 
 URLRequestCustomJobProxy::~URLRequestCustomJobProxy()
 {
-    Q_ASSERT(!m_job);
-    Q_ASSERT(!m_delegate);
 }
 
-void URLRequestCustomJobProxy::killJob()
+void URLRequestCustomJobProxy::release()
 {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
-    m_job = 0;
-    bool doDelete = false;
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     if (m_delegate) {
         m_delegate->deleteLater();
-    } else {
-        // Do not delete yet if startAsync has not yet run.
-        doDelete = m_asyncInitialized;
+        m_delegate = nullptr;
     }
-    if (m_device && m_device->isOpen())
-        m_device->close();
-    m_device = 0;
-    m_weakFactory.InvalidateWeakPtrs();
-    lock.unlock();
-    if (doDelete)
-        delete this;
-}
-
-void URLRequestCustomJobProxy::unsetJobDelegate()
-{
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    QMutexLocker lock(&m_mutex);
-    m_delegate = 0;
-    bool doDelete = false;
-    if (m_job)
-        abort();
-    else
-        doDelete = true;
-    lock.unlock();
-    if (doDelete)
-        delete this;
-}
-
-void URLRequestCustomJobProxy::setReplyMimeType(const std::string &mimeType)
-{
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    QMutexLocker lock(&m_mutex);
-    m_mimeType = mimeType;
 }
 
+// Fix me: this is  never used
+/*
 void URLRequestCustomJobProxy::setReplyCharset(const std::string &charset)
 {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    QMutexLocker lock(&m_mutex);
-    m_charset = charset;
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    if (!m_job)
+        return;
+    m_job->m_charset = charset;
 }
-
-void URLRequestCustomJobProxy::setReplyDevice(QIODevice *device)
+*/
+void URLRequestCustomJobProxy::reply(std::string mimeType, QIODevice *device)
 {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    QMutexLocker lock(&m_mutex);
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     if (!m_job)
         return;
-    m_device = device;
-    if (m_device && !m_device->isReadable())
-        m_device->open(QIODevice::ReadOnly);
+    m_job->m_mimeType = mimeType;
+    m_job->m_device = device;
+    if (m_job->m_device && !m_job->m_device->isReadable())
+        m_job->m_device->open(QIODevice::ReadOnly);
 
-    qint64 size = m_device ? m_device->size() : -1;
+    qint64 size = m_job->m_device ? m_job->m_device->size() : -1;
     if (size > 0)
         m_job->set_expected_content_size(size);
-    if (m_device && m_device->isReadable()) {
-        content::BrowserThread::PostTask(
-                    content::BrowserThread::IO, FROM_HERE,
-                    base::Bind(&URLRequestCustomJobProxy::notifyStarted,
-                               m_weakFactory.GetWeakPtr()));
+    if (m_job->m_device && m_job->m_device->isReadable()) {
+        m_started = true;
+        m_job->NotifyHeadersComplete();
     } else {
         fail(ERR_INVALID_URL);
     }
 }
 
-void URLRequestCustomJobProxy::redirect(const GURL &url)
+void URLRequestCustomJobProxy::redirect(GURL url)
 {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-    QMutexLocker lock(&m_mutex);
-    if (m_device || m_error)
-        return;
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     if (!m_job)
         return;
-    m_redirect = url;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                     base::Bind(&URLRequestCustomJobProxy::notifyStarted,
-                                                m_weakFactory.GetWeakPtr()));
-}
-
-void URLRequestCustomJobProxy::abort()
-{
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    QMutexLocker lock(&m_mutex);
-    if (m_device && m_device->isOpen())
-        m_device->close();
-    m_device = 0;
-    if (!m_job)
+    if (m_job->m_device || m_job->m_error)
         return;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                     base::Bind(&URLRequestCustomJobProxy::notifyCanceled,
-                                                m_weakFactory.GetWeakPtr()));
+    m_job->m_redirect = url;
+    m_started = true;
+    m_job->NotifyHeadersComplete();
 }
 
-void URLRequestCustomJobProxy::notifyCanceled()
+void URLRequestCustomJobProxy::abort()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
     if (!m_job)
         return;
+    if (m_job->m_device && m_job->m_device->isOpen())
+        m_job->m_device->close();
+    m_job->m_device = nullptr;
     if (m_started)
         m_job->NotifyCanceled();
     else
         m_job->NotifyStartError(URLRequestStatus(URLRequestStatus::CANCELED, ERR_ABORTED));
 }
 
-void URLRequestCustomJobProxy::notifyStarted()
-{
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
-    if (!m_job)
-        return;
-    Q_ASSERT(!m_started);
-    m_started = true;
-    m_job->NotifyHeadersComplete();
-}
-
 void URLRequestCustomJobProxy::fail(int error)
-{
-    QMutexLocker lock(&m_mutex);
-    m_error = error;
-    if (content::BrowserThread::CurrentlyOn(content::BrowserThread::IO))
-        return;
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    if (!m_job)
-        return;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                     base::Bind(&URLRequestCustomJobProxy::notifyFailure,
-                                                m_weakFactory.GetWeakPtr()));
-}
-
-void URLRequestCustomJobProxy::notifyFailure()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
     if (!m_job)
         return;
-    if (m_device)
-        m_device->close();
+    m_job->m_error = error;
+    if (m_job->m_device)
+        m_job->m_device->close();
     if (!m_started)
-        m_job->NotifyStartError(URLRequestStatus::FromError(m_error));
+        m_job->NotifyStartError(URLRequestStatus::FromError(error));
     // else we fail on the next read, or the read that might already be in progress
 }
 
-GURL URLRequestCustomJobProxy::requestUrl()
-{
-    QMutexLocker lock(&m_mutex);
-    if (!m_job)
-        return GURL();
-    return m_job->request()->url();
-}
-
-std::string URLRequestCustomJobProxy::requestMethod()
-{
-    QMutexLocker lock(&m_mutex);
-    if (!m_job)
-        return std::string();
-    return m_job->request()->method();
-}
-
-void URLRequestCustomJobProxy::startAsync()
+void URLRequestCustomJobProxy::initialize(GURL url, std::string method)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    Q_ASSERT(!m_started);
     Q_ASSERT(!m_delegate);
-    QMutexLocker lock(&m_mutex);
-    if (!m_job) {
-        lock.unlock();
-        delete this;
-        return;
-    }
 
     QWebEngineUrlSchemeHandler *schemeHandler = 0;
-    QSharedPointer<const BrowserContextAdapter> browserContext = m_job->m_adapter.toStrongRef();
+    QSharedPointer<const BrowserContextAdapter> browserContext = m_adapter.toStrongRef();
     if (browserContext)
-        schemeHandler = browserContext->customUrlSchemeHandlers()[toQByteArray(m_job->m_scheme)];
+        schemeHandler = browserContext->customUrlSchemeHandlers()[toQByteArray(m_scheme)];
     if (schemeHandler) {
-        m_delegate = new URLRequestCustomJobDelegate(this);
-        m_asyncInitialized = true;
+        m_delegate = new URLRequestCustomJobDelegate(this, toQt(url),
+                                                     QByteArray::fromStdString(method));
         QWebEngineUrlRequestJob *requestJob = new QWebEngineUrlRequestJob(m_delegate);
         schemeHandler->requestStarted(requestJob);
-    } else {
-        lock.unlock();
-        abort();
-        delete this;
-        return;
     }
 }
 

src/core/url_request_custom_job_proxy.h

@@ -40,11 +40,9 @@
 #ifndef URL_REQUEST_CUSTOM_JOB_PROXY_H_
 #define URL_REQUEST_CUSTOM_JOB_PROXY_H_
 
-#include "url/gurl.h"
 #include "base/memory/weak_ptr.h"
-#include <QtCore/QMutex>
-#include <QtCore/QPointer>
-
+#include "url/gurl.h"
+#include <QtCore/QWeakPointer>
 
 QT_FORWARD_DECLARE_CLASS(QIODevice)
 
@@ -52,44 +50,35 @@ namespace QtWebEngineCore {
 
 class URLRequestCustomJob;
 class URLRequestCustomJobDelegate;
+class BrowserContextAdapter;
 
 // Used to comunicate between URLRequestCustomJob living on the IO thread
 // and URLRequestCustomJobDelegate living on the UI thread.
-class URLRequestCustomJobProxy {
+class URLRequestCustomJobProxy
+    : public base::RefCountedThreadSafe<URLRequestCustomJobProxy> {
+
 public:
-    URLRequestCustomJobProxy(URLRequestCustomJob *job);
+    URLRequestCustomJobProxy(URLRequestCustomJob *job,
+                             const std::string &scheme,
+                             QWeakPointer<const BrowserContextAdapter> adapter);
     ~URLRequestCustomJobProxy();
 
-    void setReplyMimeType(const std::string &);
-    void setReplyCharset(const std::string &);
-    void setReplyDevice(QIODevice *);
-
-    void redirect(const GURL &url);
-    void fail(int);
+    //void setReplyCharset(const std::string &);
+    void reply(std::string mimeType, QIODevice *device);
+    void redirect(GURL url);
     void abort();
+    void fail(int error);
+    void release();
+    void initialize(GURL url, std::string method);
 
-    void killJob();
-    void unsetJobDelegate();
-
-    void startAsync();
-    void notifyStarted();
-    void notifyFailure();
-    void notifyCanceled();
-
-    GURL requestUrl();
-    std::string requestMethod();
-
-    QMutex m_mutex;
-    QPointer<QIODevice> m_device;
+    //IO thread owned
     URLRequestCustomJob *m_job;
-    URLRequestCustomJobDelegate *m_delegate;
-    std::string m_mimeType;
-    std::string m_charset;
-    int m_error;
-    GURL m_redirect;
     bool m_started;
-    bool m_asyncInitialized;
-    base::WeakPtrFactory<URLRequestCustomJobProxy> m_weakFactory;
+
+    //UI thread owned
+    std::string m_scheme;
+    URLRequestCustomJobDelegate *m_delegate;
+    QWeakPointer<const BrowserContextAdapter> m_adapter;
 };
 
 } // namespace QtWebEngineCore