From 4fa6a7a7c7e15a560bd8503fb5c0dc2e6d1024c1 Mon Sep 17 00:00:00 2001 From: Joerg Bornemann Date: Thu, 21 Jan 2016 18:38:47 +0100 Subject: Fix deadlock on QWebEngineUrlRequestJob::fail URLRequestCustomJob::notifyFailure calls NotifyStartError(status), which in turn will result in a call to URLRequestCustomJob::Kill. We must release the lock of m_mutex before calling NotifyStartError, otherwise m_mutex.lock() will wait forever in Kill. Change-Id: I319e45049766c2192dfc46a91b352b92ec677bc6 Task-number: QTBUG-50160 Reviewed-by: Allan Sandfeld Jensen --- src/core/url_request_custom_job.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'src/core/url_request_custom_job.cpp') diff --git a/src/core/url_request_custom_job.cpp b/src/core/url_request_custom_job.cpp index 1b6f1c767..907f71c2e 100644 --- a/src/core/url_request_custom_job.cpp +++ b/src/core/url_request_custom_job.cpp @@ -222,13 +222,17 @@ void URLRequestCustomJob::fail(int error) void URLRequestCustomJob::notifyFailure() { DCHECK_CURRENTLY_ON(content::BrowserThread::IO); - QMutexLocker lock(&m_mutex); + m_mutex.lock(); if (m_device) m_device->close(); - if (m_started) - NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, m_error)); + const URLRequestStatus status(URLRequestStatus::FAILED, m_error); + const bool started = m_started; + m_mutex.unlock(); + + if (started) + NotifyDone(status); else - NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, m_error)); + NotifyStartError(status); } void URLRequestCustomJob::startAsync() -- cgit v1.2.3 From eeb18149fa6d796dd5202f080b828099a9187046 Mon Sep 17 00:00:00 2001 From: Joerg Bornemann Date: Tue, 26 Jan 2016 17:35:06 +0100 Subject: Fix occasional "WeakPtrs must be checked on the same sequenced thread." Revert 7c7ee9a9, and fix the issue by passing a raw this pointer to startAsync. Bind will take care of calling AddRef/Release for us. Otherwise the WeakPtr would get assigned to the UI thread and must be invalidated in the same. Change-Id: Iee741dde521cf085a086e397a8154fa1384d58d1 Task-number: QTBUG-49670 Reviewed-by: Allan Sandfeld Jensen --- src/core/url_request_custom_job.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) (limited to 'src/core/url_request_custom_job.cpp') diff --git a/src/core/url_request_custom_job.cpp b/src/core/url_request_custom_job.cpp index 907f71c2e..84c898c3a 100644 --- a/src/core/url_request_custom_job.cpp +++ b/src/core/url_request_custom_job.cpp @@ -60,8 +60,7 @@ URLRequestCustomJob::URLRequestCustomJob(URLRequest *request, NetworkDelegate *n , m_schemeHandler(schemeHandler) , m_error(0) , m_started(false) - , m_weakFactoryIO(this) - , m_weakFactoryUI(this) + , m_weakFactory(this) { } @@ -79,7 +78,8 @@ URLRequestCustomJob::~URLRequestCustomJob() void URLRequestCustomJob::Start() { DCHECK_CURRENTLY_ON(content::BrowserThread::IO); - content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, base::Bind(&URLRequestCustomJob::startAsync, m_weakFactoryIO.GetWeakPtr())); + content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, + base::Bind(&URLRequestCustomJob::startAsync, this)); } void URLRequestCustomJob::Kill() @@ -94,8 +94,7 @@ void URLRequestCustomJob::Kill() if (m_device && m_device->isOpen()) m_device->close(); m_device = 0; - m_weakFactoryIO.InvalidateWeakPtrs(); - m_weakFactoryUI.InvalidateWeakPtrs(); + m_weakFactory.InvalidateWeakPtrs(); URLRequestJob::Kill(); } @@ -153,7 +152,7 @@ void URLRequestCustomJob::setReplyDevice(QIODevice *device) m_device->open(QIODevice::ReadOnly); if (m_device && m_device->isReadable()) - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactoryUI.GetWeakPtr())); + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactory.GetWeakPtr())); else fail(ERR_INVALID_URL); } @@ -181,7 +180,7 @@ void URLRequestCustomJob::redirect(const GURL &url) QMutexLocker lock(&m_mutex); m_redirect = url; - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactoryUI.GetWeakPtr())); + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyStarted, m_weakFactory.GetWeakPtr())); } void URLRequestCustomJob::abort() @@ -191,7 +190,7 @@ void URLRequestCustomJob::abort() 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_weakFactoryUI.GetWeakPtr())); + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyCanceled, m_weakFactory.GetWeakPtr())); } void URLRequestCustomJob::notifyCanceled() @@ -216,7 +215,7 @@ void URLRequestCustomJob::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_weakFactoryUI.GetWeakPtr())); + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestCustomJob::notifyFailure, m_weakFactory.GetWeakPtr())); } void URLRequestCustomJob::notifyFailure() -- cgit v1.2.3 From fd7f2367515bd243a171ae1234b3443d6f24955d Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Wed, 27 Jan 2016 11:21:53 +0100 Subject: Fix multi-thread protection in custom url handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Joerg Bornemann --- src/core/url_request_custom_job.cpp | 231 +++++++++++++++++++++++++----------- 1 file changed, 160 insertions(+), 71 deletions(-) (limited to 'src/core/url_request_custom_job.cpp') 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(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(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 -- cgit v1.2.3