From 989cafe53592453e1250eeb26d17c67f424f9083 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 4 Jan 2021 01:57:36 +0100 Subject: [PATCH] Take raw pointer and not wxSharedPtr<> in SetData() Using shared pointer seems to be ill-advised here, the stream shouldn't be shared as it's going to be used by wxWebRequest itself and can't be used by the application code in parallel, so the ownership transfer semantics is more appropriate. We could take a wxScopedPtr<> instead, but wx API takes ownership of raw pointers everywhere else, so do it here too. Incidentally fix a bug with calling IsOk() on a possibly null pointer. --- include/wx/private/webrequest.h | 4 ++-- include/wx/webrequest.h | 2 +- interface/wx/webrequest.h | 19 ++++++++++++++++--- src/common/webrequest.cpp | 25 +++++++++++++++++-------- tests/net/webrequest.cpp | 4 ++-- 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/include/wx/private/webrequest.h b/include/wx/private/webrequest.h index e77858ef57..b74bbd566b 100644 --- a/include/wx/private/webrequest.h +++ b/include/wx/private/webrequest.h @@ -56,7 +56,7 @@ public: void SetData(const wxString& text, const wxString& contentType, const wxMBConv& conv = wxConvUTF8); - bool SetData(const wxSharedPtr& dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); + bool SetData(wxScopedPtr& dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); void SetIgnoreServerErrorStatus(bool ignore) { m_ignoreServerErrorStatus = ignore; } @@ -99,7 +99,7 @@ protected: wxWebRequest::Storage m_storage; wxWebRequestHeaderMap m_headers; wxFileOffset m_dataSize; - wxSharedPtr m_dataStream; + wxScopedPtr m_dataStream; wxWebRequestImpl(wxWebSession& session, wxEvtHandler* handler, int id); diff --git a/include/wx/webrequest.h b/include/wx/webrequest.h index 0f9da9aa3f..2582021d1d 100644 --- a/include/wx/webrequest.h +++ b/include/wx/webrequest.h @@ -134,7 +134,7 @@ public: void SetData(const wxString& text, const wxString& contentType, const wxMBConv& conv = wxConvUTF8); - bool SetData(const wxSharedPtr& dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); + bool SetData(wxInputStream* dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); void SetIgnoreServerErrorStatus(bool ignore); diff --git a/interface/wx/webrequest.h b/interface/wx/webrequest.h index 1eea20b305..dcde845b5a 100644 --- a/interface/wx/webrequest.h +++ b/interface/wx/webrequest.h @@ -269,10 +269,23 @@ public: @c GET and the given @a dataStream will be posted as the body of this request. + Example of use: + @code + std::unique_ptr stream(new wxFileInputStream("some_file.dat")); + if ( !stream->IsOk() ) { + // Handle error (due to e.g. file not found) here. + ... + return; + } + request.SetData(stream.release(), "application/octet-stream") + @endcode + @param dataStream The data in this stream will be posted as the request body. The - stream may be empty, which will result in sending 0 bytes of data, - but if not empty, should be valid. + pointer may be @NULL, which will result in sending 0 bytes of data, + but if not empty, should be valid, i.e. wxInputStream::IsOk() must + return @true. This object takes ownership of the passed in pointer + and will delete it, i.e. the pointer must be heap-allocated. @param contentType The value of HTTP "Content-Type" header, e.g. "application/octet-stream". @@ -284,7 +297,7 @@ public: dataSize is not specified and the attempt to determine stream size failed; @true in all the other cases. */ - bool SetData(const wxSharedPtr& dataStream, + bool SetData(wxInputStream* dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); /** diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index 2f82d9d5fa..0c7134274b 100644 --- a/src/common/webrequest.cpp +++ b/src/common/webrequest.cpp @@ -97,17 +97,23 @@ bool wxWebRequestImpl::IsActiveState(wxWebRequest::State state) void wxWebRequestImpl::SetData(const wxString& text, const wxString& contentType, const wxMBConv& conv) { m_dataText = text.mb_str(conv); - SetData(wxSharedPtr(new wxMemoryInputStream(m_dataText, m_dataText.length())), contentType); + + wxScopedPtr + stream(new wxMemoryInputStream(m_dataText, m_dataText.length())); + SetData(stream, contentType); } -bool wxWebRequestImpl::SetData(const wxSharedPtr& dataStream, const wxString& contentType, wxFileOffset dataSize) +bool +wxWebRequestImpl::SetData(wxScopedPtr& dataStream, + const wxString& contentType, + wxFileOffset dataSize) { - if ( !dataStream->IsOk() ) - return false; + m_dataStream.reset(dataStream.release()); - m_dataStream = dataStream; - if ( m_dataStream.get() ) + if ( m_dataStream ) { + wxCHECK_MSG( m_dataStream->IsOk(), false, "can't use invalid stream" ); + if ( dataSize == wxInvalidOffset ) { // Determine data size @@ -342,13 +348,16 @@ void wxWebRequest::SetData(const wxString& text, const wxString& contentType, co } bool -wxWebRequest::SetData(const wxSharedPtr& dataStream, +wxWebRequest::SetData(wxInputStream* dataStream, const wxString& contentType, wxFileOffset dataSize) { + // Ensure that the stream is destroyed even we return below. + wxScopedPtr streamPtr(dataStream); + wxCHECK_IMPL( false ); - return m_impl->SetData(dataStream, contentType, dataSize); + return m_impl->SetData(streamPtr, contentType, dataSize); } void wxWebRequest::SetIgnoreServerErrorStatus(bool ignore) diff --git a/tests/net/webrequest.cpp b/tests/net/webrequest.cpp index 488fc1a0d2..cb8caede1e 100644 --- a/tests/net/webrequest.cpp +++ b/tests/net/webrequest.cpp @@ -178,10 +178,10 @@ TEST_CASE_METHOD(RequestFixture, "WebRequest", "[net][webrequest]") SECTION("PUT file data") { Create("/put"); - wxSharedPtr is(new wxFileInputStream("horse.png")); + wxScopedPtr is(new wxFileInputStream("horse.png")); REQUIRE( is->IsOk() ); - request.SetData(is, "image/png"); + request.SetData(is.release(), "image/png"); request.SetMethod("PUT"); Run(); }