From e62173c4798a7e4f23181ef3325e7ed2f5cc3cf6 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 9 Jul 2015 20:59:38 +0200 Subject: [PATCH] Fix using wxHTTP and wxFTP from worker thread in Unix ports. This backports e18c8fd29a10b1b87ea5cff7c5b3a16fe5b32690, d421373c2eee777fd1300fcfb56971f9248ba382 and 6c43aa90b6e2cf117c226e80dc6e28c3d3f974af from master to avoid crashes when using wxHTTP or wxFTP from threads other than main. See #17031. --- docs/changes.txt | 2 ++ include/wx/private/socket.h | 3 ++ samples/sockets/client.cpp | 66 ++++++++++++++++++++++++++++--------- src/common/ftp.cpp | 4 --- src/common/http.cpp | 22 ++----------- src/common/protocol.cpp | 6 +++- src/unix/sockunix.cpp | 5 +++ 7 files changed, 68 insertions(+), 40 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index 78ebaf40e1..d266d58964 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -608,6 +608,7 @@ wxGTK: - Fix crashes in wxGTK3 when running with non-X11 backend (Marco Trevisan). - Fix coordinates of wxSetCursorEvent propagated to parent windows. - Fix GTK+ warnings when refreshing wxListCtrl items (Scott Talbert). +- Fix using wxHTTP and wxFTP from worker thread. wxMSW: @@ -632,6 +633,7 @@ wxOSX: - Compilation fix for wxWebView under 10.10. - Fix conversion of wxBitmap to wxImage in 64 bit builds. +- Fix using wxHTTP and wxFTP from worker thread. - Fix wxFileDialog::GetFilterIndex() for file open dialogs (phsilva). - Fix custom paper support (tijsv). diff --git a/include/wx/private/socket.h b/include/wx/private/socket.h index 973b764f7d..362aa61982 100644 --- a/include/wx/private/socket.h +++ b/include/wx/private/socket.h @@ -308,6 +308,9 @@ public: protected: wxSocketImpl(wxSocketBase& wxsocket); + // get the associated socket flags + wxSocketFlags GetSocketFlags() const { return m_wxsocket->GetFlags(); } + // true if we're a listening stream socket bool m_server; diff --git a/samples/sockets/client.cpp b/samples/sockets/client.cpp index e0bd39e16e..6159fe19ea 100644 --- a/samples/sockets/client.cpp +++ b/samples/sockets/client.cpp @@ -31,6 +31,7 @@ #include "wx/socket.h" #include "wx/url.h" #include "wx/sstream.h" +#include "wx/thread.h" #include // -------------------------------------------------------------------------- @@ -575,23 +576,13 @@ void MyFrame::OnDatagram(wxCommandEvent& WXUNUSED(event)) #if wxUSE_URL -void MyFrame::OnTestURL(wxCommandEvent& WXUNUSED(event)) +void DoDownload(const wxString& urlname) { - // Ask for the URL - static wxString s_urlname("http://www.google.com/"); - wxString urlname = wxGetTextFromUser - ( - _("Enter an URL to get"), - _("URL:"), - s_urlname - ); - if ( urlname.empty() ) - return; // cancelled by user + wxString testname("URL"); + if ( !wxIsMainThread() ) + testname += " in worker thread"; - s_urlname = urlname; - - - TestLogger logtest("URL"); + TestLogger logtest(testname); // Parse the URL wxURL url(urlname); @@ -626,6 +617,51 @@ void MyFrame::OnTestURL(wxCommandEvent& WXUNUSED(event)) urlname, sout.GetString()); } +void MyFrame::OnTestURL(wxCommandEvent& WXUNUSED(event)) +{ + // Ask for the URL + static wxString s_urlname("http://www.google.com/"); + wxString urlname = wxGetTextFromUser + ( + _("Enter an URL to get"), + _("URL:"), + s_urlname + ); + if ( urlname.empty() ) + return; // cancelled by user + + s_urlname = urlname; + + // First do it in this thread. + DoDownload(urlname); + + // And then also in a worker thread. +#if wxUSE_THREADS + class DownloadThread : public wxThread + { + public: + explicit DownloadThread(const wxString& url): m_url(url) + { + Run(); + } + + virtual void* Entry() wxOVERRIDE + { + DoDownload(m_url); + + return NULL; + } + + private: + const wxString m_url; + }; + + // NB: there is a race condition here, we don't check for this thread + // termination before exiting the application, don't do this in real code! + new DownloadThread(urlname); +#endif // wxUSE_THREADS +} + #endif // wxUSE_URL void MyFrame::OnSocketEvent(wxSocketEvent& event) diff --git a/src/common/ftp.cpp b/src/common/ftp.cpp index cfb59e63d4..14e1947a07 100644 --- a/src/common/ftp.cpp +++ b/src/common/ftp.cpp @@ -79,8 +79,6 @@ wxFTP::wxFTP() m_username = wxT("anonymous"); m_password << wxGetUserId() << wxT('@') << wxGetFullHostName(); - SetNotify(0); - SetFlags(wxSOCKET_NOWAIT); m_bPassive = true; m_bEncounteredError = false; } @@ -782,8 +780,6 @@ wxInputStream *wxFTP::GetInputStream(const wxString& path) return NULL; } - sock->SetFlags(wxSOCKET_WAITALL); - m_streaming = true; wxInputFTPStream *in_stream = new wxInputFTPStream(this, sock); diff --git a/src/common/http.cpp b/src/common/http.cpp index 0ecde88c77..cd58f0d9d4 100644 --- a/src/common/http.cpp +++ b/src/common/http.cpp @@ -22,7 +22,6 @@ #ifndef WX_PRECOMP #include "wx/string.h" - #include "wx/app.h" #endif #include "wx/tokenzr.h" @@ -48,8 +47,6 @@ wxHTTP::wxHTTP() m_read = false; m_proxy_mode = false; m_http_response = 0; - - SetNotify(wxSOCKET_LOST_FLAG); } wxHTTP::~wxHTTP() @@ -370,16 +367,6 @@ bool wxHTTP::BuildRequest(const wxString& path, const wxString& method) SetHeader(wxT("Authorization"), GenerateAuthString(m_username, m_password)); } - SaveState(); - - // we may use non blocking sockets only if we can dispatch events from them - int flags = wxIsMainThread() && wxApp::IsMainLoopRunning() ? wxSOCKET_NONE - : wxSOCKET_BLOCK; - // and we must use wxSOCKET_WAITALL to ensure that all data is sent - flags |= wxSOCKET_WAITALL; - SetFlags(flags); - Notify(false); - wxString buf; buf.Printf(wxT("%s %s HTTP/1.0\r\n"), method, path); const wxWX2MBbuf pathbuf = buf.mb_str(); @@ -395,10 +382,8 @@ bool wxHTTP::BuildRequest(const wxString& path, const wxString& method) wxString tmp_str; m_lastError = ReadLine(this, tmp_str); - if (m_lastError != wxPROTO_NOERR) { - RestoreState(); + if (m_lastError != wxPROTO_NOERR) return false; - } if (!tmp_str.Contains(wxT("HTTP/"))) { // TODO: support HTTP v0.9 which can have no header. @@ -441,7 +426,7 @@ bool wxHTTP::BuildRequest(const wxString& path, const wxString& method) m_lastError = wxPROTO_NOERR; ret_value = ParseHeaders(); - RestoreState(); + return ret_value; } @@ -540,9 +525,6 @@ wxInputStream *wxHTTP::GetInputStream(const wxString& path) inp_stream->m_read_bytes = 0; - Notify(false); - SetFlags(wxSOCKET_BLOCK | wxSOCKET_WAITALL); - // no error; reset m_lastError m_lastError = wxPROTO_NOERR; return inp_stream; diff --git a/src/common/protocol.cpp b/src/common/protocol.cpp index 74662adb8a..0560cf9088 100644 --- a/src/common/protocol.cpp +++ b/src/common/protocol.cpp @@ -21,6 +21,7 @@ #include "wx/protocol/log.h" #ifndef WX_PRECOMP + #include "wx/app.h" #include "wx/module.h" #endif @@ -63,7 +64,10 @@ IMPLEMENT_ABSTRACT_CLASS(wxProtocol, wxObject) wxProtocol::wxProtocol() #if wxUSE_SOCKETS - : wxSocketClient() + // Only use non blocking sockets if we can dispatch events. + : wxSocketClient((wxIsMainThread() && wxApp::IsMainLoopRunning() + ? wxSOCKET_NONE + : wxSOCKET_BLOCK) | wxSOCKET_WAITALL) #endif { m_lastError = wxPROTO_NOERR; diff --git a/src/unix/sockunix.cpp b/src/unix/sockunix.cpp index 7d47f18e95..74f1f41bf4 100644 --- a/src/unix/sockunix.cpp +++ b/src/unix/sockunix.cpp @@ -98,6 +98,11 @@ wxSocketError wxSocketImplUnix::GetLastError() const void wxSocketImplUnix::DoEnableEvents(int flags, bool enable) { + // No events for blocking sockets, they should be usable from the other + // threads and the events only work for the sockets used by the main one. + if ( GetSocketFlags() & wxSOCKET_BLOCK ) + return; + wxSocketManager * const manager = wxSocketManager::Get(); if (!manager) return;