Fix multi-thread protection in custom url handler

The classes were not properly protected against race conditions. To
solve this there is now a class shared between the two thread that is
not deleted until the classes on both threads have been deleted.

Change-Id: I499bd98805ae7a195aca42f30610eb6c2b0fd0f7
Reviewed-by: Michael Brüning <michael.bruning@theqtcompany.com>
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

5 files changed, 206 insertions, 96 deletions

diff --git a/src/core/api/qwebengineurlrequestjob.h b/src/core/api/qwebengineurlrequestjob.h
index 922299fd9..84741b791 100644
--- a/src/core/api/qwebengineurlrequestjob.h
+++ b/src/core/api/qwebengineurlrequestjob.h
@@ -44,8 +44,8 @@
 #include <QtCore/qurl.h>
 
 namespace QtWebEngineCore {
-class URLRequestCustomJob;
 class URLRequestCustomJobDelegate;
+class URLRequestCustomJobShared;
 } // namespace
 
 QT_BEGIN_NAMESPACE
@@ -76,7 +76,7 @@ public:
 
 private:
     QWebEngineUrlRequestJob(QtWebEngineCore::URLRequestCustomJobDelegate *);
-    friend class QtWebEngineCore::URLRequestCustomJob;
+    friend class QtWebEngineCore::URLRequestCustomJobShared;
 
     QtWebEngineCore::URLRequestCustomJobDelegate* d_ptr;
 };
diff --git a/src/core/url_request_custom_job.cpp b/src/core/url_request_custom_job.cpp
index 84c898c3a..07087c247 100644
--- a/src/core/url_request_custom_job.cpp
+++ b/src/core/url_request_custom_job.cpp
@@ -56,45 +56,33 @@ namespace QtWebEngineCore {
 
 URLRequestCustomJob::URLRequestCustomJob(URLRequest *request, NetworkDelegate *networkDelegate, QWebEngineUrlSchemeHandler *schemeHandler)
     : URLRequestJob(request, networkDelegate)
-    , m_device(0)
     , m_schemeHandler(schemeHandler)
-    , m_error(0)
-    , m_started(false)
-    , m_weakFactory(this)
+    , m_shared(new URLRequestCustomJobShared(this))
 {
 }
 
 URLRequestCustomJob::~URLRequestCustomJob()
 {
-    QMutexLocker lock(&m_mutex);
-    if (m_delegate) {
-        m_delegate->m_job = 0;
-        m_delegate->deleteLater();
-    }
-    if (m_device && m_device->isOpen())
-        m_device->close();
+    if (m_shared)
+        m_shared->killJob();
+}
+
+static void startAsync(URLRequestCustomJobShared *shared)
+{
+    shared->startAsync();
 }
 
 void URLRequestCustomJob::Start()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-                                     base::Bind(&URLRequestCustomJob::startAsync, this));
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, base::Bind(&startAsync, m_shared));
 }
 
 void URLRequestCustomJob::Kill()
 {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
-    if (m_delegate) {
-        m_delegate->m_job = 0;
-        m_delegate->deleteLater();
-    }
-    m_delegate = 0;
-    if (m_device && m_device->isOpen())
-        m_device->close();
-    m_device = 0;
-    m_weakFactory.InvalidateWeakPtrs();
+    if (m_shared)
+        m_shared->killJob();
+    m_shared = 0;
 
     URLRequestJob::Kill();
 }
@@ -102,8 +90,11 @@ void URLRequestCustomJob::Kill()
 bool URLRequestCustomJob::GetMimeType(std::string *mimeType) const
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    if (m_mimeType.size() > 0) {
-        *mimeType = m_mimeType;
+    if (!m_shared)
+        return false;
+    QMutexLocker lock(&m_shared->m_mutex);
+    if (m_shared->m_mimeType.size() > 0) {
+        *mimeType = m_shared->m_mimeType;
         return true;
     }
     return false;
@@ -112,8 +103,11 @@ bool URLRequestCustomJob::GetMimeType(std::string *mimeType) const
 bool URLRequestCustomJob::GetCharset(std::string* charset)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    if (m_charset.size() > 0) {
-        *charset = m_charset;
+    if (!m_shared)
+        return false;
+    QMutexLocker lock(&m_shared->m_mutex);
+    if (m_shared->m_charset.size() > 0) {
+        *charset = m_shared->m_charset;
         return true;
     }
     return false;
@@ -122,128 +116,223 @@ bool URLRequestCustomJob::GetCharset(std::string* charset)
 bool URLRequestCustomJob::IsRedirectResponse(GURL* location, int* http_status_code)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    QMutexLocker lock(&m_mutex);
-    if (m_redirect.is_valid()) {
-        *location = m_redirect;
+    if (!m_shared)
+        return false;
+    QMutexLocker lock(&m_shared->m_mutex);
+    if (m_shared->m_redirect.is_valid()) {
+        *location = m_shared->m_redirect;
         *http_status_code = 303;
         return true;
     }
     return false;
 }
 
-void URLRequestCustomJob::setReplyMimeType(const std::string &mimeType)
+bool URLRequestCustomJob::ReadRawData(IOBuffer *buf, int bufSize, int *bytesRead)
+{
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    Q_ASSERT(bytesRead);
+    Q_ASSERT(m_shared);
+    QMutexLocker lock(&m_shared->m_mutex);
+    qint64 rv = m_shared->m_device ? m_shared->m_device->read(buf->data(), bufSize) : -1;
+    if (rv >= 0) {
+        *bytesRead = static_cast<int>(rv);
+        return true;
+    } else {
+        NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, ERR_FAILED));
+    }
+    return false;
+}
+
+URLRequestCustomJobShared::URLRequestCustomJobShared(URLRequestCustomJob *job)
+    : m_mutex(QMutex::Recursive)
+    , m_job(job)
+    , m_delegate(0)
+    , m_error(0)
+    , m_started(false)
+    , m_asyncInitialized(false)
+    , m_weakFactory(this)
+{
+}
+
+URLRequestCustomJobShared::~URLRequestCustomJobShared()
+{
+    Q_ASSERT(!m_job);
+    Q_ASSERT(!m_delegate);
+}
+
+void URLRequestCustomJobShared::killJob()
+{
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    QMutexLocker lock(&m_mutex);
+    m_job = 0;
+    bool doDelete = false;
+    if (m_delegate) {
+        m_delegate->deleteLater();
+    } else {
+        // Do not delete yet if startAsync has not yet run.
+        doDelete = m_asyncInitialized;
+    }
+    if (m_device && m_device->isOpen())
+        m_device->close();
+    m_device = 0;
+    m_weakFactory.InvalidateWeakPtrs();
+    lock.unlock();
+    if (doDelete)
+        delete this;
+}
+
+void URLRequestCustomJobShared::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 URLRequestCustomJobShared::setReplyMimeType(const std::string &mimeType)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    QMutexLocker lock(&m_mutex);
     m_mimeType = mimeType;
 }
 
-void URLRequestCustomJob::setReplyCharset(const std::string &charset)
+void URLRequestCustomJobShared::setReplyCharset(const std::string &charset)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    QMutexLocker lock(&m_mutex);
     m_charset = charset;
 }
 
-void URLRequestCustomJob::setReplyDevice(QIODevice *device)
+void URLRequestCustomJobShared::setReplyDevice(QIODevice *device)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return;
     m_device = device;
     if (m_device && !m_device->isReadable())
         m_device->open(QIODevice::ReadOnly);
 
     if (m_device && m_device->isReadable())
-        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactory.GetWeakPtr()));
+        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJobShared::notifyStarted, m_weakFactory.GetWeakPtr()));
     else
         fail(ERR_INVALID_URL);
 }
 
-bool URLRequestCustomJob::ReadRawData(IOBuffer *buf, int bufSize, int *bytesRead)
-{
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    Q_ASSERT(bytesRead);
-    QMutexLocker lock(&m_mutex);
-    qint64 rv = m_device ? m_device->read(buf->data(), bufSize) : -1;
-    if (rv >= 0) {
-        *bytesRead = static_cast<int>(rv);
-        return true;
-    } else {
-        NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, ERR_FAILED));
-    }
-    return false;
-}
-
-void URLRequestCustomJob::redirect(const GURL &url)
+void URLRequestCustomJobShared::redirect(const GURL &url)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    if (m_device || m_error)
-        return;
 
     QMutexLocker lock(&m_mutex);
+    if (m_device || m_error)
+        return;
+    if (!m_job)
+        return;
     m_redirect = url;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactory.GetWeakPtr()));
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJobShared::notifyStarted, m_weakFactory.GetWeakPtr()));
 }
 
-void URLRequestCustomJob::abort()
+void URLRequestCustomJobShared::abort()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     QMutexLocker lock(&m_mutex);
     if (m_device && m_device->isOpen())
         m_device->close();
     m_device = 0;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyCanceled, m_weakFactory.GetWeakPtr()));
+    if (!m_job)
+        return;
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJobShared::notifyCanceled, m_weakFactory.GetWeakPtr()));
 }
 
-void URLRequestCustomJob::notifyCanceled()
+void URLRequestCustomJobShared::notifyCanceled()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return;
     if (m_started)
-        NotifyDone(URLRequestStatus(URLRequestStatus::CANCELED, ERR_ABORTED));
+        m_job->NotifyDone(URLRequestStatus(URLRequestStatus::CANCELED, ERR_ABORTED));
     else
-        NotifyStartError(URLRequestStatus(URLRequestStatus::CANCELED, ERR_ABORTED));
+        m_job->NotifyStartError(URLRequestStatus(URLRequestStatus::CANCELED, ERR_ABORTED));
 }
 
-void URLRequestCustomJob::notifyStarted()
+void URLRequestCustomJobShared::notifyStarted()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return;
     Q_ASSERT(!m_started);
     m_started = true;
-    NotifyHeadersComplete();
+    m_job->NotifyHeadersComplete();
 }
 
-void URLRequestCustomJob::fail(int error)
+void URLRequestCustomJobShared::fail(int error)
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     QMutexLocker lock(&m_mutex);
     m_error = error;
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyFailure, m_weakFactory.GetWeakPtr()));
+    if (!m_job)
+        return;
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJobShared::notifyFailure, m_weakFactory.GetWeakPtr()));
 }
 
-void URLRequestCustomJob::notifyFailure()
+void URLRequestCustomJobShared::notifyFailure()
 {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    m_mutex.lock();
+    QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return;
     if (m_device)
         m_device->close();
     const URLRequestStatus status(URLRequestStatus::FAILED, m_error);
     const bool started = m_started;
-    m_mutex.unlock();
 
     if (started)
-        NotifyDone(status);
+        m_job->NotifyDone(status);
     else
-        NotifyStartError(status);
+        m_job->NotifyStartError(status);
 }
 
-void URLRequestCustomJob::startAsync()
+GURL URLRequestCustomJobShared::requestUrl()
+{
+    QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return GURL();
+    return m_job->request()->url();
+}
+
+std::string URLRequestCustomJobShared::requestMethod()
+{
+    QMutexLocker lock(&m_mutex);
+    if (!m_job)
+        return std::string();
+    return m_job->request()->method();
+}
+
+void URLRequestCustomJobShared::startAsync()
 {
     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;
+    }
     m_delegate = new URLRequestCustomJobDelegate(this);
-    lock.unlock();
+    m_asyncInitialized = true;
     QWebEngineUrlRequestJob *requestJob = new QWebEngineUrlRequestJob(m_delegate);
-    m_schemeHandler->requestStarted(requestJob);
+    if (m_job)
+        m_job->m_schemeHandler->requestStarted(requestJob);
 }
 
 } // namespace
diff --git a/src/core/url_request_custom_job.h b/src/core/url_request_custom_job.h
index a994c467a..eedb41814 100644
--- a/src/core/url_request_custom_job.h
+++ b/src/core/url_request_custom_job.h
@@ -50,6 +50,7 @@ QT_FORWARD_DECLARE_CLASS(QWebEngineUrlSchemeHandler)
 namespace QtWebEngineCore {
 
 class URLRequestCustomJobDelegate;
+class URLRequestCustomJobShared;
 
 // A request job that handles reading custom URL schemes
 class URLRequestCustomJob : public net::URLRequestJob {
@@ -62,6 +63,25 @@ public:
     virtual bool GetCharset(std::string *charset) Q_DECL_OVERRIDE;
     virtual bool IsRedirectResponse(GURL* location, int* http_status_code) Q_DECL_OVERRIDE;
 
+protected:
+    virtual ~URLRequestCustomJob();
+
+private:
+    QWebEngineUrlSchemeHandler *m_schemeHandler;
+    URLRequestCustomJobShared *m_shared;
+
+    friend class URLRequestCustomJobShared;
+
+    DISALLOW_COPY_AND_ASSIGN(URLRequestCustomJob);
+};
+
+// A shared state between URLRequestCustomJob living on the IO thread
+// and URLRequestCustomJobDelegate living on the UI thread.
+class URLRequestCustomJobShared {
+public:
+    URLRequestCustomJobShared(URLRequestCustomJob *job);
+    ~URLRequestCustomJobShared();
+
     void setReplyMimeType(const std::string &);
     void setReplyCharset(const std::string &);
     void setReplyDevice(QIODevice *);
@@ -70,28 +90,28 @@ public:
     void fail(int);
     void abort();
 
-protected:
-    virtual ~URLRequestCustomJob();
+    void killJob();
+    void unsetJobDelegate();
+
     void startAsync();
     void notifyStarted();
     void notifyFailure();
     void notifyCanceled();
 
-private:
+    GURL requestUrl();
+    std::string requestMethod();
+
     QMutex m_mutex;
     QPointer<QIODevice> m_device;
-    QPointer<URLRequestCustomJobDelegate> m_delegate;
-    QWebEngineUrlSchemeHandler *m_schemeHandler;
+    URLRequestCustomJob *m_job;
+    URLRequestCustomJobDelegate *m_delegate;
     std::string m_mimeType;
     std::string m_charset;
     int m_error;
     GURL m_redirect;
     bool m_started;
-    base::WeakPtrFactory<URLRequestCustomJob> m_weakFactory;
-
-    friend class URLRequestCustomJobDelegate;
-
-    DISALLOW_COPY_AND_ASSIGN(URLRequestCustomJob);
+    bool m_asyncInitialized;
+    base::WeakPtrFactory<URLRequestCustomJobShared> m_weakFactory;
 };
 
 } // namespace QtWebEngineCore
diff --git a/src/core/url_request_custom_job_delegate.cpp b/src/core/url_request_custom_job_delegate.cpp
index 0650242c8..1a3e08e52 100644
--- a/src/core/url_request_custom_job_delegate.cpp
+++ b/src/core/url_request_custom_job_delegate.cpp
@@ -44,39 +44,40 @@
 
 namespace QtWebEngineCore {
 
-URLRequestCustomJobDelegate::URLRequestCustomJobDelegate(URLRequestCustomJob *job)
-    : m_job(job)
+URLRequestCustomJobDelegate::URLRequestCustomJobDelegate(URLRequestCustomJobShared *shared)
+    : m_shared(shared)
 {
 }
 
 URLRequestCustomJobDelegate::~URLRequestCustomJobDelegate()
 {
+    m_shared->unsetJobDelegate();
 }
 
 QUrl URLRequestCustomJobDelegate::url() const
 {
-    return toQt(m_job->request()->url());
+    return toQt(m_shared->requestUrl());
 }
 
 QByteArray URLRequestCustomJobDelegate::method() const
 {
-    return QByteArray::fromStdString(m_job->request()->method());
+    return QByteArray::fromStdString(m_shared->requestMethod());
 }
 
 void URLRequestCustomJobDelegate::setReply(const QByteArray &contentType, QIODevice *device)
 {
-    m_job->setReplyMimeType(contentType.toStdString());
-    m_job->setReplyDevice(device);
+    m_shared->setReplyMimeType(contentType.toStdString());
+    m_shared->setReplyDevice(device);
 }
 
 void URLRequestCustomJobDelegate::abort()
 {
-    m_job->abort();
+    m_shared->abort();
 }
 
 void URLRequestCustomJobDelegate::redirect(const QUrl &url)
 {
-    m_job->redirect(toGurl(url));
+    m_shared->redirect(toGurl(url));
 }
 
 void URLRequestCustomJobDelegate::fail(Error error)
@@ -102,7 +103,7 @@ void URLRequestCustomJobDelegate::fail(Error error)
         break;
     }
     if (net_error)
-        m_job->fail(net_error);
+        m_shared->fail(net_error);
 }
 
 } // namespace
diff --git a/src/core/url_request_custom_job_delegate.h b/src/core/url_request_custom_job_delegate.h
index 5b6137820..e066e85cc 100644
--- a/src/core/url_request_custom_job_delegate.h
+++ b/src/core/url_request_custom_job_delegate.h
@@ -46,7 +46,7 @@ QT_FORWARD_DECLARE_CLASS(QIODevice)
 
 namespace QtWebEngineCore {
 
-class URLRequestCustomJob;
+class URLRequestCustomJobShared;
 
 class QWEBENGINE_EXPORT URLRequestCustomJobDelegate : public QObject {
     Q_OBJECT
@@ -72,10 +72,10 @@ public:
     void fail(Error);
 
 private:
-    URLRequestCustomJobDelegate(URLRequestCustomJob *job);
+    URLRequestCustomJobDelegate(URLRequestCustomJobShared *shared);
 
-    friend class URLRequestCustomJob;
-    URLRequestCustomJob *m_job;
+    friend class URLRequestCustomJobShared;
+    URLRequestCustomJobShared *m_shared;
 };
 
 } // namespace