Extend life time of wxWebRequest while it is in process

We already did it just before processing the state change event, but
this was too late, as the object could have been already deleted by then
and this actually happened with the example from wxWebRequest
documentation.

Do it earlier now, as soon as the request becomes active, which normally
happens when Start() is called, and keep the reference until the event
is processed after the request reaches one of the final states
(completed, failed or cancelled).

Add a unit test checking that deleting the wxWebRequest object doesn't
prevent the request from running to the completion any more.

include/wx/private/webrequest.h

@@ -106,6 +106,13 @@ public:
 
     wxEvtHandler* GetHandler() const { return m_handler; }
 
+    // Called to notify about the state change in the main thread by SetState()
+    // (which can itself be called from a different one).
+    //
+    // It also releases a reference added when switching to the active state by
+    // SetState() when leaving it.
+    //
+    // TODO-C++11: make private when we don't need StateEventProcessor any more.
     void ProcessStateEvent(wxWebRequest::State state, const wxString& failMsg);
 
 protected:

src/common/webrequest.cpp

@@ -175,15 +175,6 @@ struct StateEventProcessor
                         const wxString& failMsg)
         : m_request(request), m_state(state), m_failMsg(failMsg)
     {
-        // Ensure that the request object stays alive until this event is
-        // processed.
-        m_request.IncRef();
-    }
-
-    StateEventProcessor(const StateEventProcessor& other)
-        : m_request(other.m_request), m_state(other.m_state), m_failMsg(other.m_failMsg)
-    {
-        m_request.IncRef();
     }
 
     void operator()()
@@ -191,11 +182,6 @@ struct StateEventProcessor
         m_request.ProcessStateEvent(m_state, m_failMsg);
     }
 
-    ~StateEventProcessor()
-    {
-        m_request.DecRef();
-    }
-
     wxWebRequestImpl& m_request;
     const wxWebRequest::State m_state;
     const wxString m_failMsg;
@@ -230,6 +216,17 @@ void wxWebRequestImpl::SetState(wxWebRequest::State state, const wxString & fail
     wxLogTrace(wxTRACE_WEBREQUEST, "Request %p: state %s => %s",
                this, StateName(m_state), StateName(state));
 
+    if ( state == wxWebRequest::State_Active )
+    {
+        // The request is now in progress (maybe not for the first time if the
+        // old state was State_Unauthorized and not State_Idle), ensure that it
+        // stays alive until it terminates, even if wxWebRequest object itself
+        // is deleted.
+        //
+        // Note that matching DecRef() is done by ProcessStateEvent() later.
+        IncRef();
+    }
+
     m_state = state;
 
     // Trigger the event in the main thread except when switching to the active
@@ -343,6 +340,7 @@ void wxWebRequestImpl::ProcessStateEvent(wxWebRequest::State state, const wxStri
     wxWebRequestEvent evt(wxEVT_WEBREQUEST_STATE, GetId(), state,
                           wxWebResponse(response), failMsg);
 
+    bool release = false;
     switch ( state )
     {
         case wxWebRequest::State_Idle:
@@ -350,7 +348,17 @@ void wxWebRequestImpl::ProcessStateEvent(wxWebRequest::State state, const wxStri
             break;
 
         case wxWebRequest::State_Active:
+            break;
+
         case wxWebRequest::State_Unauthorized:
+            // This one is tricky: we might not be done with the request yet,
+            // but we don't know if this is the case or not, i.e. if the
+            // application will call wxWebAuthChallenge::SetCredentials() or
+            // not later. So we release it now, as we assume that it still
+            // keeps a reference to the original request if it intends to do it
+            // anyhow, i.e. this won't actually destroy the request object in
+            // this case.
+            release = true;
             break;
 
         case wxWebRequest::State_Completed:
@@ -365,6 +373,8 @@ void wxWebRequestImpl::ProcessStateEvent(wxWebRequest::State state, const wxStri
         case wxWebRequest::State_Cancelled:
             if ( response )
                 response->Finalize();
+
+            release = true;
             break;
     }
 
@@ -374,6 +384,10 @@ void wxWebRequestImpl::ProcessStateEvent(wxWebRequest::State state, const wxStri
     // could have been deleted or moved away by the event handler.
     if ( !dataFile.empty() && wxFileExists(dataFile) )
         wxRemoveFile(dataFile);
+
+    // This may destroy this object if it's not used from elsewhere any longer.
+    if ( release )
+        DecRef();
 }
 
 //

tests/net/webrequest.cpp

@@ -104,7 +104,7 @@ public:
             break;
 
         case wxWebRequest::State_Completed:
-            if ( request.GetStorage() == wxWebRequest::Storage_File )
+            if ( request.IsOk() && request.GetStorage() == wxWebRequest::Storage_File )
             {
                 wxFileName fn(evt.GetDataFile());
                 CHECK( fn.GetSize() == expectedFileSize );
@@ -438,6 +438,26 @@ TEST_CASE_METHOD(RequestFixture,
     REQUIRE( request.GetState() == wxWebRequest::State_Cancelled );
 }
 
+TEST_CASE_METHOD(RequestFixture,
+                 "WebRequest::Destroy", "[net][webrequest]")
+{
+    if ( !InitBaseURL() )
+        return;
+
+    Create("/base64/U3RpbGwgYWxpdmUh");
+    request.Start();
+
+    // Destroy the original request: this shouldn't prevent it from running to
+    // the completion!
+    request = wxWebRequest();
+
+    RunLoopWithTimeout();
+
+    CHECK( stateFromEvent == wxWebRequest::State_Completed );
+    CHECK( statusFromEvent == 200 );
+    CHECK( responseStringFromEvent == "Still alive!" );
+}
+
 // This test is not run by default and has to be explicitly selected to run.
 TEST_CASE_METHOD(RequestFixture,
                  "WebRequest::Manual", "[.]")