diff --git a/include/wx/private/webrequest.h b/include/wx/private/webrequest.h index 1b18d583c6..4f502b9af5 100644 --- a/include/wx/private/webrequest.h +++ b/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: diff --git a/interface/wx/webrequest.h b/interface/wx/webrequest.h index dc90ae6670..56f045c713 100644 --- a/interface/wx/webrequest.h +++ b/interface/wx/webrequest.h @@ -45,8 +45,11 @@ case wxWebRequest::State_Completed: { wxImage logoImage(*evt.GetResponse().GetStream()); - if (logoImage.IsOK()) - wxLogInfo("Image loaded"); + if (logoImage.IsOk()) + wxLogInfo("Image successfully downloaded"); + + ... do something with it ... + break; } // Request failed diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index 5baca038be..20579d29c5 100644 --- a/src/common/webrequest.cpp +++ b/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(); } // diff --git a/tests/net/webrequest.cpp b/tests/net/webrequest.cpp index b8c8de79a9..32c2f8b981 100644 --- a/tests/net/webrequest.cpp +++ b/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", "[.]")