From 360268ee25dae36e3e61083c14738d236d319c07 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Mar 2021 16:04:54 +0100 Subject: [PATCH] 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 | 7 ++++++ src/common/webrequest.cpp | 42 ++++++++++++++++++++++----------- tests/net/webrequest.cpp | 22 ++++++++++++++++- 3 files changed, 56 insertions(+), 15 deletions(-) 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/src/common/webrequest.cpp b/src/common/webrequest.cpp index 189b6d1c16..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,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(); } // diff --git a/tests/net/webrequest.cpp b/tests/net/webrequest.cpp index 5051b66386..32c2f8b981 100644 --- a/tests/net/webrequest.cpp +++ b/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", "[.]")