From ce91a5d0ffe5ff990ab0b423c4c6529264f45808 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 22 Mar 2021 18:22:28 +0100 Subject: [PATCH 1/5] Fix a typo in wxWebRequest use example s/IsOK/IsOk/ and also make the example slightly more verbose. --- interface/wx/webrequest.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 From c0f8c0e0f2082c527cff3ac320e1f80eddbbda14 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Mar 2021 16:04:02 +0100 Subject: [PATCH 2/5] Be stricter about changing to the same state in wxWebRequest This shouldn't happen, and doesn't, but if it ever does we'd better return immediately rather than doing something that will almost surely be wrong. --- src/common/webrequest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index 5baca038be..a604c89eb7 100644 --- a/src/common/webrequest.cpp +++ b/src/common/webrequest.cpp @@ -205,7 +205,7 @@ struct StateEventProcessor 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); From 0548a06905f8b95218b624773bbc6ac479a38550 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Mar 2021 16:24:34 +0100 Subject: [PATCH 3/5] Check the values from wxWebRequestEvent in the unit test too In addition to checking that wxWebRequest itself is filled with the expected values, verify that the same values are returned via wxWebRequestEvent. --- tests/net/webrequest.cpp | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/net/webrequest.cpp b/tests/net/webrequest.cpp index b8c8de79a9..5051b66386 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"); @@ -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; From d7235ebb051593c83f280b5f4185cd6034647374 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Mar 2021 17:29:30 +0100 Subject: [PATCH 4/5] Show states using names, not ordinals, in trace message This is much more readable. --- src/common/webrequest.cpp | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index a604c89eb7..189b6d1c16 100644 --- a/src/common/webrequest.cpp +++ b/src/common/webrequest.cpp @@ -201,13 +201,34 @@ struct StateEventProcessor 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) { 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)); m_state = state; From 360268ee25dae36e3e61083c14738d236d319c07 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Mar 2021 16:04:54 +0100 Subject: [PATCH 5/5] 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", "[.]")