Merge branch 'webrequest-keep-alive'

Ensure that wxWebRequest objects stay alive as long as the request is in
progress.

See https://github.com/wxWidgets/wxWidgets/pull/2292

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:

interface/wx/webrequest.h

@@ -45,8 +45,11 @@
                 case wxWebRequest::State_Completed:
                 {
                     wxImage logoImage(*evt.GetResponse().GetStream());
-                    if (logoImage.IsOK())
+                    if (logoImage.IsOk())
-                        wxLogInfo("Image loaded");
+                        wxLogInfo("Image successfully downloaded");
+
+                    ... do something with it ...
+
                     break;
                 }
                 // Request failed

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,23 +182,50 @@ 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;
 };
 
+#if wxUSE_LOG_TRACE
+
+// Tiny helper to log states as strings rather than meaningless numbers.
+wxString StateName(wxWebRequest::State state)
+{
+    switch ( state )
+    {
+        case wxWebRequest::State_Idle:            return wxS("IDLE");
+        case wxWebRequest::State_Unauthorized:    return wxS("UNAUTHORIZED");
+        case wxWebRequest::State_Active:          return wxS("ACTIVE");
+        case wxWebRequest::State_Completed:       return wxS("COMPLETED");
+        case wxWebRequest::State_Failed:          return wxS("FAILED");
+        case wxWebRequest::State_Cancelled:       return wxS("CANCELLED");
+    }
+
+    return wxString::Format("invalid state %d", state);
+}
+
+#endif // wxUSE_LOG_TRACE
+
 } // anonymous namespace
 
 void wxWebRequestImpl::SetState(wxWebRequest::State state, const wxString & failMsg)
 {
-    wxASSERT_MSG( state != m_state, "shouldn't switch to the same state" );
+    wxCHECK_RET( state != m_state, "shouldn't switch to the same state" );
 
-    wxLogTrace(wxTRACE_WEBREQUEST, "Request %p: state %d => %d", this, m_state, state);
+    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;
 
@@ -322,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:
@@ -329,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:
@@ -344,6 +373,8 @@ void wxWebRequestImpl::ProcessStateEvent(wxWebRequest::State state, const wxStri
         case wxWebRequest::State_Cancelled:
             if ( response )
                 response->Finalize();
+
+            release = true;
             break;
     }
 
@@ -353,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

@@ -44,6 +44,8 @@ public:
     {
         expectedFileSize = 0;
         dataSize = 0;
+        stateFromEvent = wxWebRequest::State_Idle;
+        statusFromEvent = 0;
     }
 
     // All tests should call this function first and skip the test entirely if
@@ -81,7 +83,17 @@ public:
 
     void OnRequestState(wxWebRequestEvent& evt)
     {
-        switch (evt.GetState())
+        stateFromEvent = evt.GetState();
+        const wxWebResponse& response = evt.GetResponse();
+        if ( response.IsOk() )
+        {
+            // Note that the response object itself may be deleted if request
+            // using it is, so we need to copy its data to use it later.
+            statusFromEvent = response.GetStatus();
+            responseStringFromEvent = response.AsString();
+        }
+
+        switch ( stateFromEvent )
         {
         case wxWebRequest::State_Idle:
             FAIL("should never get events with State_Idle");
@@ -92,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 );
@@ -134,16 +146,22 @@ public:
         request.Start();
         RunLoopWithTimeout();
 
-        if ( request.GetState() != requiredState )
+        if ( stateFromEvent != requiredState )
         {
             errorDescription.Trim();
             if ( !errorDescription.empty() )
                 WARN("Error: " << errorDescription);
         }
 
-        REQUIRE( request.GetState() == requiredState );
+        REQUIRE( stateFromEvent == requiredState );
+
+        CHECK( request.GetState() == stateFromEvent );
+
         if (requiredStatus)
+        {
+            CHECK( statusFromEvent == requiredStatus );
             CHECK( request.GetResponse().GetStatus() == requiredStatus );
+        }
     }
 
     // Precondition: we must have an auth challenge.
@@ -156,6 +174,9 @@ public:
     wxString baseURL;
     wxEventLoop loop;
     wxWebRequest request;
+    wxWebRequest::State stateFromEvent;
+    int statusFromEvent;
+    wxString responseStringFromEvent;
     wxInt64 expectedFileSize;
     wxInt64 dataSize;
     wxString errorDescription;
@@ -417,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", "[.]")