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

@@ -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;

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;

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);
 };
 

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;

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();
 

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();
 }
 

src/common/webrequest_curl.cpp

@@ -263,7 +263,7 @@ bool wxWebRequestCURL::StartRequest()
     return true;
 }
 
-void wxWebRequestCURL::Cancel()
+void wxWebRequestCURL::DoCancel()
 {
     m_sessionImpl.CancelRequest(this);
 }

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;
 }
 
 //

src/osx/webrequest_urlsession.mm

@@ -220,7 +220,7 @@ void wxWebRequestURLSession::Start()
     [m_task resume];
 }
 
-void wxWebRequestURLSession::Cancel()
+void wxWebRequestURLSession::DoCancel()
 {
     [m_task cancel];
 }