From 508a4f6ca8e8ee9e3104ad9139b81ddc7cc0a63c Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 16 Jan 2021 13:49:22 +0100 Subject: [PATCH] Fix Cancel() semantics under MSW and other improvements to it Under MSW, don't set the state to State_Cancelled as soon as Cancel() was called, as the request was still used from the other threads afterwards, resulting in race conditions and crashes. Fix this by just removing the SetState(State_Cancelled) call from the main thread, as it was redundant anyhow. This also makes the behaviour correspond to the documentation, which indicates that Cancel() works asynchronously. Also ensure, for all backends, that we actually cancel the request only once, even if public Cancel() is called multiple times. This required renaming the existing wxWebRequestImpl::Cancel() to DoCancel(). --- include/wx/msw/private/webrequest_winhttp.h | 4 ++-- .../wx/osx/private/webrequest_urlsession.h | 4 ++-- include/wx/private/webrequest.h | 12 ++++++++++- include/wx/private/webrequest_curl.h | 4 ++-- interface/wx/webrequest.h | 5 +++++ src/common/webrequest.cpp | 21 ++++++++++++++++++- src/common/webrequest_curl.cpp | 2 +- src/msw/webrequest_winhttp.cpp | 16 +++++--------- src/osx/webrequest_urlsession.mm | 2 +- 9 files changed, 49 insertions(+), 21 deletions(-) diff --git a/include/wx/msw/private/webrequest_winhttp.h b/include/wx/msw/private/webrequest_winhttp.h index 3851e7f209..c4767a7395 100644 --- a/include/wx/msw/private/webrequest_winhttp.h +++ b/include/wx/msw/private/webrequest_winhttp.h @@ -76,8 +76,6 @@ public: void Start() wxOVERRIDE; - void Cancel() wxOVERRIDE; - wxWebResponseImplPtr GetResponse() const wxOVERRIDE { return m_response; } @@ -99,6 +97,8 @@ public: } private: + void DoCancel() wxOVERRIDE; + wxWebSessionWinHTTP& m_sessionImpl; wxString m_url; HINTERNET m_connect; diff --git a/include/wx/osx/private/webrequest_urlsession.h b/include/wx/osx/private/webrequest_urlsession.h index 68a1210c9a..dad169703e 100644 --- a/include/wx/osx/private/webrequest_urlsession.h +++ b/include/wx/osx/private/webrequest_urlsession.h @@ -86,8 +86,6 @@ public: void Start() wxOVERRIDE; - void Cancel() wxOVERRIDE; - wxWebResponseImplPtr GetResponse() const wxOVERRIDE { return m_response; } @@ -120,6 +118,8 @@ public: { return m_authChallenge.get(); } private: + void DoCancel() wxOVERRIDE; + wxWebSessionURLSession& m_sessionImpl; wxString m_url; WX_NSURLSessionTask m_task; diff --git a/include/wx/private/webrequest.h b/include/wx/private/webrequest.h index 95588feba3..c4ad8ade05 100644 --- a/include/wx/private/webrequest.h +++ b/include/wx/private/webrequest.h @@ -72,7 +72,9 @@ public: // Precondition for this method checked by caller: current state is idle. virtual void Start() = 0; - virtual void Cancel() = 0; + // Precondition for this method checked by caller: not idle and not already + // cancelled. + void Cancel(); virtual wxWebResponseImplPtr GetResponse() const = 0; @@ -114,6 +116,8 @@ protected: wxEvtHandler* handler, int id); + bool WasCancelled() const { return m_cancelled; } + // Call SetState() with either State_Failed or State_Completed appropriate // for the response status. void SetFinalStateFromStatus(); @@ -121,6 +125,9 @@ protected: static bool IsActiveState(wxWebRequest::State state); private: + // Called from public Cancel() at most once per object. + virtual void DoCancel() = 0; + wxWebSession& m_session; wxEvtHandler* const m_handler; const int m_id; @@ -128,6 +135,9 @@ private: wxFileOffset m_bytesReceived; wxCharBuffer m_dataText; + // Initially false, set to true after the first call to Cancel(). + bool m_cancelled; + wxDECLARE_NO_COPY_CLASS(wxWebRequestImpl); }; diff --git a/include/wx/private/webrequest_curl.h b/include/wx/private/webrequest_curl.h index 5229db7bc2..ec38041785 100644 --- a/include/wx/private/webrequest_curl.h +++ b/include/wx/private/webrequest_curl.h @@ -50,8 +50,6 @@ public: void Start() wxOVERRIDE; - void Cancel() wxOVERRIDE; - wxWebResponseImplPtr GetResponse() const wxOVERRIDE { return m_response; } @@ -79,6 +77,8 @@ public: size_t CURLOnRead(char* buffer, size_t size); private: + void DoCancel() wxOVERRIDE; + wxWebSessionCURL& m_sessionImpl; CURL* m_handle; diff --git a/interface/wx/webrequest.h b/interface/wx/webrequest.h index 10c4ef2e16..66e8ed41cd 100644 --- a/interface/wx/webrequest.h +++ b/interface/wx/webrequest.h @@ -244,6 +244,11 @@ public: Note that cancelling is asynchronous, so the application needs to wait until the request state becomes @c State_Cancelled to know when the request was really cancelled. + + Request must be active when Cancel() is called, i.e. the current state + can't be @c State_Idle. However, because it can be difficult to avoid + doing it in some circumstances, Cancel() may be called multiple times + and only a single wxWebRequestEvent will be sent even in this case. */ void Cancel(); diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index 7d77112a82..1c46b0012e 100644 --- a/src/common/webrequest.cpp +++ b/src/common/webrequest.cpp @@ -66,10 +66,26 @@ wxWebRequestImpl::wxWebRequestImpl(wxWebSession& session, m_handler(handler), m_id(id), m_state(wxWebRequest::State_Idle), - m_bytesReceived(0) + m_bytesReceived(0), + m_cancelled(false) { } +void wxWebRequestImpl::Cancel() +{ + if ( m_cancelled ) + { + // Nothing to do, don't even assert -- it's ok to call Cancel() + // multiple times, but calling DoCancel() once is enough. + return; + } + + wxLogTrace(wxTRACE_WEBREQUEST, "Request %p: cancelling", this); + + m_cancelled = true; + DoCancel(); +} + void wxWebRequestImpl::SetFinalStateFromStatus() { const wxWebResponseImplPtr& resp = GetResponse(); @@ -397,6 +413,9 @@ void wxWebRequest::Cancel() { wxCHECK_IMPL_VOID(); + wxCHECK_RET( m_impl->GetState() != wxWebRequest::State_Idle, + "Not yet started requests can't be cancelled" ); + m_impl->Cancel(); } diff --git a/src/common/webrequest_curl.cpp b/src/common/webrequest_curl.cpp index 3d42e5f08c..47264ff12b 100644 --- a/src/common/webrequest_curl.cpp +++ b/src/common/webrequest_curl.cpp @@ -263,7 +263,7 @@ bool wxWebRequestCURL::StartRequest() return true; } -void wxWebRequestCURL::Cancel() +void wxWebRequestCURL::DoCancel() { m_sessionImpl.CancelRequest(this); } diff --git a/src/msw/webrequest_winhttp.cpp b/src/msw/webrequest_winhttp.cpp index 102fc7305a..65c3a5ef18 100644 --- a/src/msw/webrequest_winhttp.cpp +++ b/src/msw/webrequest_winhttp.cpp @@ -174,7 +174,7 @@ wxWebRequestWinHTTP::HandleCallback(DWORD dwInternetStatus, if ( dwStatusInformationLength > 0 ) { if ( !m_response->ReportAvailableData(dwStatusInformationLength) - && GetState() != wxWebRequest::State_Cancelled ) + && !WasCancelled() ) SetFailedWithLastError(); } else @@ -195,7 +195,7 @@ wxWebRequestWinHTTP::HandleCallback(DWORD dwInternetStatus, // "Failing" with "cancelled" error is not actually an error if // we're expecting it, i.e. if our Cancel() had been called. if ( asyncResult->dwError == ERROR_WINHTTP_OPERATION_CANCELLED && - GetState() == wxWebRequest::State_Cancelled ) + WasCancelled() ) SetState(wxWebRequest::State_Cancelled); else SetFailed(asyncResult->dwError); @@ -398,16 +398,10 @@ void wxWebRequestWinHTTP::SendRequest() SetState(wxWebRequest::State_Active); } -void wxWebRequestWinHTTP::Cancel() +void wxWebRequestWinHTTP::DoCancel() { - wxLogTrace(wxTRACE_WEBREQUEST, "Request %p: cancelling", this); - - SetState(wxWebRequest::State_Cancelled); - if ( m_request != NULL ) - { - wxWinHTTPCloseHandle(m_request); - m_request = NULL; - } + wxWinHTTPCloseHandle(m_request); + m_request = NULL; } // diff --git a/src/osx/webrequest_urlsession.mm b/src/osx/webrequest_urlsession.mm index 2bf4500b89..43b1dcdcee 100644 --- a/src/osx/webrequest_urlsession.mm +++ b/src/osx/webrequest_urlsession.mm @@ -220,7 +220,7 @@ void wxWebRequestURLSession::Start() [m_task resume]; } -void wxWebRequestURLSession::Cancel() +void wxWebRequestURLSession::DoCancel() { [m_task cancel]; }