Non-wx thread memory leak (PR #23535)

83 views
Skip to first unread message

Antti Nietosvaara

unread,
May 9, 2023, 9:41:45 AM5/9/23
to wx-...@googlegroups.com, Subscribed

Using certain parts of wx from a thread other than wxThread results in a memory leak.

For example, using wxSemaphore causes wxLogTrace calls that create a wxThreadSpecificInfo object that then never gets freed. This was tested using wxGTK 3.2.2.1:
`#include <wx/wx.h>

#include

int main(int argc, char **argv) {
wxInitializer initializer;
wxSemaphore sema;
for (;;) {
std::async(std::launch::async, &sema { sema.Post(); });
sema.Wait();
}
return 0;
}
`

This commit tries to work around the problem by only creating the wxThreadSpecificInfo from wxThreads, and otherwise reverting to using common log objects.

Notably in master this would probably be a trivial thing to work around by just creating the wxThreadSpecificInfo thread_local, but I wanted a workaround for 3.2.

Another place which this will affect is wxTranslations::GetUntranslatedString. I switched it to use a common critical section protected hashmap, which should work just find in my understanding. Although I am not 100% sure if the hash map created by WX_DECLARE_HASH_SET keeps the addresses of its items intact when inserting new items. std::unordered_set should be safe and I am assuming this works the same way.

You can view, comment on, or merge this pull request online at:

  https://github.com/wxWidgets/wxWidgets/pull/23535

Commit Summary

  • 120aa61 Make generic wxListCtrl lines more visible
  • 960dde9 Change wxImage::ResampleBox() to avoid MSVS 2022 optimizer bug
  • 5f0be15 Fix the date of 3.2.1 release in the change log
  • 6c7a14f Allow specifying release date in the pre-release script
  • 0a60849 Fix typo in glib name in a change log entry
  • 8df3517 Update documentation for 3.2.1 release
  • 42573c6 Make wxWindow::GetTextExtent() result match wxDC::GetTextExtent() on OSX
  • 109ae40 Merge branch 'cmake-gtk-printing' of https://github.com/MaartenBent/wxWidgets
  • ac18f51 Add a GitHub Actions workflow for creating releases
  • f0cfdab Merge branch 'release-workflow'
  • 10a7ee9 Fix bug when creating a new release in our gh-release action fork
  • fb1d884 Restore check for radius <= 0 when drawing ellipse with Cairo
  • 1c2185e Add wxDC::DrawCircle() 0 radius fix in wxGTK to the change log
  • 7352e5a Improve wxBitmapBundle documentation, notably mention SVG support
  • 97e9970 Update checksums of 3.2.1 release files
  • f2937b6 Fix including "wx/sckaddr.h" as first header, see #22791
  • 00b5dd6 Update version to 3.2.2
  • d77c9ad Improve wxJSScriptWrapper performance
  • 86af31a Fix data race when using wxTheApp::m_handlersWithPendingEvents
  • 25c7319 Update the instructions for creating releases on GitHub
  • 418c84d CMake: Restore wx-config variables
  • 3967bd9 Fix more double negatives used with 'neither'
  • cb172e3 Add wxDEPRECATED_EXPORT_CORE() macro for wxTransformMatrix
  • a6d8598 Remove or comment out unused variables in wxOSX code
  • 522d1da Use WXUNUSED() for unused parameters in wxOSX wxToggleButton code
  • 9de789d Fix for missing X11 libraries when linking wxGTK2 statically
  • b4cc350 Add an example of using wxrc docs with CMake add_subdirectory()
  • ec7120a CMake: use output directory under wx directory in the build tree
  • 7438f82 Fix compilation of wxGTK 3 with 3.22.z and z < 25
  • c036b9d Fix return value of wxZoomGestureEvent::GetZoomFactor() in wxMSW
  • ddefb07 Fix resizing wxGLCanvas in wxGTK with EGL and Wayland
  • 61a0473 Handle Aero Snap when saving top level window geometry
  • 401bab8 Improve wxEVT_GRID_SELECT_CELL documentation
  • d723a1b Improve formatting of wxStandardPaths::UseAppInfo() documentation
  • 12248d3 Define GDK_KEY_LaunchX to fix build with GTK < 2.21.8
  • caf2060 Document third wxBitmapBundle::FromSVG() variant
  • 8c9cdb2 Update TLW icon when the DPI changes in wxMSW
  • b6d9948 Implement wxEVT_DATAVIEW_COLUMN_HEADER_RIGHT_CLICK in wxOSX
  • e440bdd Handle header and scrolling in wxOSX wxDataViewCtrl::HitTest()
  • f7c54bb Remove "Printing" prefix from title of printout document
  • dc08b9a Fix memory leak when deleting wxThreads
  • 9d47560 Add serial number to wxwin.m4
  • 0543352 Apply fix for CVE-2022-40674 to Expat submodule
  • 80aa7cc Fix wxEVT_TEXT_ENTER and other events from wxMSW wxBitmapComboBox
  • e342239 Fix native coordinate system within wxStaticBox on macOS
  • 7c6b529 Fix compilation when including wx/propgrid/editors.h early
  • 8dd1375 Generic wxTreeCtrl icons high DPI improvements
  • 47a7041 Correct wxBitmapBundle best match choice description in its docs
  • 7835005 Fix wxTaskBarButton availability in the documentation
  • f0ee875 Fix MSW "Save" dialog overwrite prompt for files without ext
  • d53bbfc Add support for wxUSE_TASKBARBUTTON option to configure
  • bf2b837 Update comments in setup_inc.h and various wx/setup.h files
  • d109e79 Update strftime() link in wxDateTime::Format() docs
  • 92a63f0 Always use vendor in CMake-built DLLs regardless of version
  • 317a133 Do not trigger non-CMake CI builds with CMake-only changes
  • b597d39 Fix the name of EVT_WEBVIEW_FULLSCREEN_CHANGED macro in the docs
  • 4cd34b1 Fix position of "Window" menu in wxOSX
  • 5c39d04 Fix drawing of right wxStaticBox border when using custom label
  • 720cff6 Document that wxSearchCtrl derives only from wxTextEntry
  • 22e339a Fix wxMSW menu items using custom fonts in high DPI
  • b45d470 Add macros for event tables for three wxWebView events
  • 6d0ad72 Update Windows Theme support page
  • 173c8f9 Update Multithreading Overview programming guide
  • 8ad526c Update Backwards Compatibility programming guide.
  • 55c1ad8 Use wxChoice for month selection in wxGenericCalendarCtrl
  • a0b7450 Fix implicit declaration configuration errors with Xcode 12
  • ea6769f Avoid deprecation warnings in ABI check CI job
  • af3db51 Fix typo in ccache-action version in the parent commit
  • 1b12103 Fix wxOSX compilation with Xcode 7.3 and macOS 11 SDK
  • 92a970c Handle 'T' separator in wxDateTime::ParseDateTime()
  • feea01b Use wxTaskBarIcon::GetPopupMenu() for menu icons under Mac too
  • 1450e5e Define HAVE_SSIZE_T as 1 in CMake builds for consistency
  • b2c37a7 Improve documentation of events sent by wxTreeCtrl::EditLabel()
  • 90179ee Queue wxWebViewEdge events to avoid difficult to debug bugs
  • 563df08 Add Serbian translation
  • b490048 Fix editor max length
  • 60ad4aa Add wxPropertyGrid editor max length fix to the change log
  • 0c4f026 Fix wxSysColourChangedEvent brief description in Doxygen
  • 353d793 Initialize m_value field of wxGrid editors too
  • 81ad04e Avoid -Wunused-result in the except sample
  • 032a71b Update GitHub actions to avoid relying on deprecated versions
  • d17f35d Use wxLogNull to temporarily disable logging in wxZipInputStream
  • 39c2a48 Fix type of values passed to TIFFSetField()
  • 2efb6e7 Fix retrieving printer information in wxMSW print dialog code
  • 4705cb3 Simplify defining manifest in wx.rc
  • d979a7f Fix memory leak of command line arguments in wxMSW
  • e934e0e Move wxTextCtrl constructors out of line in wxMSW
  • 9207368 Fix scaling of standard MSW platform icons in high DPI
  • 2892794 Consistently include trailing NUL in wxMSW wxTextDataObject size
  • 25c5f57 Initialize atoms used in wxGTK wxDataObject on demand
  • fcf6ff8 Don't append extra NUL to URLs in wxGTK wxURLDataObject
  • 6c2c922 Restore private wxDataFormat::PrepareFormats() to preserve ABI
  • 2ca3d25 Avoid warnings about signed/unsigned comparison in wxGrid code
  • 68f6dc2 Update copyright years to 2023 in 3.2 branch too
  • 974b596 Rebake after the copyright year update
  • c3f34bf Switch to using detectportal.firefox.com for wxURL HTTP test
  • e1a6bb2 Disable test for loading images using wxURL
  • 203e501 Fix peeking into an already closed socket
  • b8c4d54 Improve comments in samples/sample.rc
  • 4cef952 Include wxrc.exe in the 64-bit wxMSW MSVC binary package
  • 3c18113 Only build memcheck sample when debugging is enabled
  • 1d32868 Fix typo in wxDEFINE_EVENT macro name in the documentation
  • ac29c30 Fix wrongly recreating wxComboBox in disabled state in wxMSW
  • 3a621d7 Don't use C++11 features in wxGdkAtom backported from master
  • 37d5bf3 Disable using [[deprecated]] attribute when not using C++11
  • e3c869e Use C++98 for some CI builds in 3.2 branch
  • ece2326 Merge branch 'c++98-fixes' into 3.2
  • 40885eb Fix painting artifacts with AUI and wxGTK3 in some environments
  • 49c14fa Use margins around the text in wxGenericListCtrl on all platforms
  • 2bad14d Fix using pkg-config when cross-compiling to Linux targets
  • 05f67df Fix initializing wxFont from wxNativeFontInfo in wxQt
  • e350ebb Fix determining joystick product name
  • f2b8938 Avoid expanding the window size by too much on DPI change
  • c1b27f3 Fix size of native icons in MSW wxArtProvider in high DPI
  • f16387b Fix support for high DPI icons in generic wxListCtrl
  • af2f7f2 Fix drawing wxTaskBarIcon under macOS in high DPI
  • 7163630 Fix wxUILocale compilation with musl
  • 42f3fb4 Use named links in Markdown instead of numbered ones
  • 0fd9546 Fix a possible crash in Mac menu item event handling
  • e2470b2 Allow selecting and copying text in wxMessageDialog on GTK
  • 54cecd5 Update bundled zlib to 1.2.13.1
  • ac625b1 Fix keyboard shortcuts handling in wxWebView under macOS
  • cd6499a Fix for routing ordinary selectors in Mac wxWebView
  • 9b4ec19 Work around MSVS 2022 optimizer bug in wxImage::ShrinkBy()
  • edbd909 Work around wxPropertyGrid state loss on modules reinitialization
  • 03ccc88 Support Caps/Num/Scroll Lock in wxGetKeyState() in wxGTK
  • 0a8a624 Ensure that TLW size is updated when decoration size changes on Wayland
  • 3619d34 Update TLW size before showing on Wayland when decoration size becomes known
  • fdf43f8 Fix build with wxUSE_SEARCHCTRL==0
  • 43994dd Fix compilation with C++98 compilers after recent change
  • dc9baf6 Add automatic check for not using nullptr etc in 3.2 branch
  • e575b22 Fix the branch compared with in the parent commit
  • 0a3a332 Work around placement new breakage by wx/msw/msvcrt.h
  • ec2e4f7 Explain why including wx/msw/msvcrt.h is a bad idea
  • 1434756 Mention Wayland TLW size fix
  • f55ed82 Document wxBookCtrlBase::GetControlSizer
  • 1fa1963 Merge branch '3.2-new-redef-fix' into 3.2
  • f51a04f Work around link errors when using wxSimplebook in DLL wx build
  • 25c805e Revert "Use new CoreGraphics API to reset clipping region"
  • 60e67de Return English names for wxLOCALE_FORM_ENGLISH under Mac
  • 7bc1177 use properly translated string when creating default window menu
  • 8f8f34b Use fractional point sizes correctly when creating fonts in wxOSX
  • 4e272cc Map wxFONTFAMILY_DEFAULT to wxFONTFAMILY_SWISS in wxOSX too
  • 94997a4 Round the value returned from wxFont::GetPixelSize() in wxOSX
  • 087f0c7 Improve size and handling of in-place editor in wxGenericTreeCtrl
  • ee2a2cc Fix return value of wxToolBar::GetToolBitmapSize() under GTK/Mac
  • 65729d7 Update French translations of the internat sample
  • 7714c32 Make comment in wxLocale::GetLanguageInfo() even more detailed
  • 2b1da43 Don't look for C++11 keywords in the comments
  • 86cf1bb Add a pseudo test to show system locale and language
  • dc1710b Recommend using wxUILocale::GetSystemLanguage() in wxLocale docs
  • 8bff4bc Handle wxLANGUAGE_DEFAULT specially in wxLocale::IsAvailable()
  • cacb97f Give more information if wxLocale::Default unit test fails
  • 6f8f030 Don't assume that wxLocale::Init(wxLANGUAGE_DEFAULT) succeeds
  • 2b02c03 Don't ignore LC_XXX in wxUILocaleImpl::GetPreferredUILanguages()
  • d273cec Fix wxUILocale::FindLanguageInfo() to work for mixed locales
  • 17b90ee Return wxLANGUAGE_UNKNOWN from GetSystemLocale() in mixed locales
  • 1d1b132 Add wxUILocale::GetSystemLocaleId()
  • da3ff24 Create valid (if empty) "C" locale under macOS
  • 7db14d4 Remove outdated special case for Mac from wxSetlocale()
  • 00977fd Fail if environment variables define unknown locale under Unix
  • 194cc44 Fix GetSystemLocale() return value for "C" locale
  • a6b9432 Don't use LC_MESSAGES for determining locale on non-glibc systems
  • 0d69104 Merge branch '3.2-locale-improve' into 3.2
  • 5b89b4a Update release instructions for 3.2 branch
  • 01bda92 Update release documents for 3.2.2
  • 4d2bd7c Fix typo in the README and announcement text
  • 3f6e2ca Update the version of action-gh-release used
  • c98f9c9 Update MSYS2 MinGW version to 12.2.0
  • 0574a81 Update version to 3.2.3
  • 13451f7 Add a reminder to update the version after adding new APIs
  • 8353a47 Add a header for 3.2.3 changes section to the change log
  • 5b0518e Update version of action-gh-release in another place
  • 0f6dfb6 Add missing new line before the checksums
  • 2922fa0 Fix drawing of icons for non-root wxTreeCtrl items
  • 74512db Add move ctor and assignment operator to wxString
  • 6e38c80 Show run-time wxWidgets version in the minimal sample "About"
  • 393b575 Respect composition mode in wxOSX wxGraphicsContext::DrawBitmap()
  • 17d7cf3 Add wxSUBRELEASE_NUMBER to wxGetLibraryVersionInfo() if non-zero
  • 4b57475 Fix wxFLEX_GROWMODE_ALL in wxFlexGridSizer with proportions
  • 44870f2 Minor fixes to wxWebRequestCURL
  • fda2ff6 Fix updating value of item in wxPGArrayEditorDialog
  • acd030c Fix setting locale to wxLANGUAGE_UKRAINIAN
  • 35adde2 Fix losing underline/strike-through in wxFont in wxOSX sometimes
  • 026b2c5 Remove wxSYS_COLOUR_MAX from documentation
  • ff680eb Update Georgian translations
  • 0687e2d Fix background of shaped frames under macOS
  • f13f561 Fix initial width of wxTextCtrl in wxOSX
  • db1beaf Fix clang warnings about truncating 64 bit ints to 32 bits
  • cb4ebdc Document wxSplitterWindow::SetSashPosition() argument better
  • 1074cc8 Run Ubuntu 18.04 builds in a container on GitHub CI
  • 24d696c Switch CMake wxGTK build to Ubuntu 22.04
  • 72e3825 Merge branch '3.2-ci-containers' into 3.2
  • 70b31c6 Improve date validation in wxDatePickerCtrlGeneric
  • c08e48d Use wxCOL_WIDTH_DEFAULT instead of -1 in wxDataViewCtrl functions
  • fe3dff8 Add forgotten wxUSE_VALIDATORS checks
  • ba6e78e CMake: Enable large file support
  • b5ef8b7 CMake: Update documentation of wxWidgets_USE_FILE
  • 6f0ecd4 CMake: Apply no-RTTI compile options directly to targets
  • 9627664 CMake: silence shorten-64-to-32 warnings on macOS
  • 4d3b158 fixup! Improve date validation in wxDatePickerCtrlGeneric
  • 36d359c fixup! Improve date validation in wxDatePickerCtrlGeneric
  • 98261d4 Fix handling of duplicate -arch or -framework options
  • 7ad5b88 Enable wxWebView in ASAN Unix CI build
  • 6db3dba Don't reset wxToolBar colour on system colours change in wxMSW
  • 1a34b76 Fix memory leak with extra controls in wxFileDialog on OSX
  • 9d9aa58 Update Brazilian Portuguese translations for 3.2.2
  • e1d8bc8 CMake: Add libraries of imported targets to wx-config
  • f9befcc Fix using Wine in wxMSW cross-CI workflow
  • aae0540 Extend wxWebView test hack to all the builds
  • 365bea9 Don't try using -mthreads with clang
  • 2aac6d6 Fix dereferencing invalid iterator in wxString in UTF-8 build
  • 90b42b9 Fix wxRegKey compilation in UTF-8 build
  • 270c086 Fix using wxStringOutputStream with surrogates in UTF-8 build
  • d0dd8f2 Fix another surrogate-related bug in UTF-8 build in wxString
  • 759331e Don't assert when using "%s" with invalid char in UTF-8 build
  • 2c2d9fd Fix recognizing locales using UTF-8 charset
  • ecc0b2f Fix handling non-ASCII format strings in UTF-8 build
  • ca4db68 CI: ignore CircleCI config files from all other CIs
  • 148cafd Fix memory leak in the art provider sample
  • 5150c1a Fix wxUILocale::GetPreferredUILanguages() under Windows < 10
  • 4d584bd Fix information for some languages in the language database
  • 1f0d84b Replace gtk_combo_box_text_insert_text with direct model insertion
  • 8094ed5 Correct wxDC::GetContentScaleFactor() documentation
  • ac2d5fd Fix wrong codepoint in rc file with llvm-rc
  • ee3f10c Fix runtime exception when running application compiled with clang on Windows
  • 8296145 Fix building wxScintilla with LLVM clang under MSW
  • 867c720 Improve and document wxGLCanvas::CreateSurface()
  • d19acd5 Fix for wxLocale breaking Unicode string formatting under macOS
  • a34e550 Fix discrepancy between using GDI and Direct2D in high DPI
  • 1713692 Miscellaneous fixes for WebKit2 wxWebView backend
  • 87ae8d5 Fix wxKeyEvent::GetKeyCode() for non-US keyboard layouts
  • 9ce295f Improve code in gtk_window_key_press_callback()
  • 6fdc9e4 Simplify wxKeyEvent generation in wxGTK a bit
  • 5d0a77c Disable bogus gcc 13 warnings in Scintilla SplitVector
  • 9ce34a1 Fix showing hint in wxUniv wxTextCtrl
  • 38c4b48 Test non-Unicode wxMSW build in CI workflow
  • cc46425 Fix non-Unicode wxMSW build after command line arguments fixes
  • 5c239e1 Fix wxRegKey compilation in ANSI build too
  • f5cccfe Avoid ambiguity with the deprecated wxToolBar::AddTool() overload
  • 601b4f4 Suppress -Wsuggest-attribute=format in non-Unicode build
  • 5279c62 Avoid -Wsuggest-override warnings for 2.8-compatible functions
  • 9c1fa39 Make wxGCC_ONLY_WARNING_SUPPRESS really gcc-specific
  • 0c23908 Avoid harmless unused parameter warning in non-Unicode build
  • 7a8331c Skip tests failing without Unicode in string unit test
  • e9a0f96 Don't run regex unit tests in non-Unicode build
  • 29f55f4 Disable wxMenu unit test requiring Unicode in non-Unicode build
  • a5cfd0e Skip wxListCtrl column drag test in wxMSW non-Unicode build
  • 89dc204 Merge branch '3.2-fix-ansi' into 3.2
  • 6af61da Fix a memory leak when using non-wx threads.

File Changes

(300 files)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535@github.com>

VZ

unread,
May 9, 2023, 10:57:43 AM5/9/23
to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.

Thanks for looking into this!

This is definitely a problem which needs to be fixed, but while I've started
writing a reply saying that I didn't see a way to do it in any other way, I've finally deleted it because I think I do see a way, or even a couple of ways, to do it better.

Basically, we just need to ensure that wxThreadSpecificInfo::ThreadCleanUp() is called on any thread exit, not just when wxThread terminates. And it should be possible to do it:

  1. In C++11 I think we could just create a global thread_local object whose dtor would be executed -- if my understanding is correct, I didn't test it -- whenever a thread terminates. Then it would be simply a matter of calling ThreadCleanUp() from it. And I think the ordering should be correct, i.e. it should happen after all the rest of the thread code ends running.
  2. In C++98, if we/you care about fixing it for it too, although this seems much less important considering that it doesn't have std::thread and so people using it are much more likely to use wxThread, we could still do it in platform-specific way by registering a function to call on thread exit when creating wxThreadSpecificInfo:

Doing this would be much preferable to this PR because it wouldn't (silently!) change the behaviour of the existing code, which is really problematic, especially in 3.2 branch.

It would be great if you could please try re-fixing this problem in this way, so as to preserve the current behaviour and let wxLog etc work as it does now.

TIA!

In src/common/log.cpp:

>  
-    return oldLogger;
+    wxASSERT_MSG( false, "SetThreadActiveTarget() only works when called from a wxThread" );

This should be a wxCHECK_MSG( wxThreadInfo, ...) above (would also preserve the indent of the existing code).

In src/common/log.cpp:

>  }
 
 /* static */
 bool wxLog::EnableThreadLogging(bool enable)
 {
-    const bool wasEnabled = !wxThreadInfo.loggingDisabled;
-    wxThreadInfo.loggingDisabled = !enable;
+    const bool wasEnabled = IsThreadLoggingEnabled();
+    if ( wxThreadInfo )
+        wxThreadInfo->loggingDisabled = !enable;
+    else
+        wxASSERT_MSG(enable, "Disabling thread logging only works for wxThreads");

Same remark as above.

In src/common/translation.cpp:

>  
 /* static */
 const wxString& wxTranslations::GetUntranslatedString(const wxString& str)
 {
-    wxLocaleUntranslatedStrings& strings = wxThreadInfo.untranslatedStrings;
-
-    wxLocaleUntranslatedStrings::iterator i = strings.find(str);
-    if ( i == strings.end() )
-        return *strings.insert(str).first;
-
-    return *i;
+    static wxCriticalSection s_cs;

This is only MT-safe in C++11, in C++98 we'd need to make it global. Not sure if it's worth changing this...

In include/wx/private/threadinfo.h:

> +    // Initialize the wxThreadSpecificInfo.
+    // If this is not called from a thread, the wxThreadInfo will remain NULL
+    // in that thread's context. Use ThreadCleanUp to free when thread ends.
+    static void ThreadInitialize();

It would make sense to have a wxThreadInfoInitializer class calling this in its ctor and CleanUp() in its dtor now that we have both functions.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/review/1418807907@github.com>

Antti Nietosvaara

unread,
May 9, 2023, 2:10:44 PM5/9/23
to wx-...@googlegroups.com, Subscribed

Alright, thanks for the feedback. I'll see if I can get the ThreadCleanUp call to work on all threads. I actually have an FLS-based TLS implementation somewhere which might be usable here...

I believe with C++11 thread_local would make the entire ThreadCleanUp obsolete: you could just implement the wxThreadSpecificInfo::Get() like:
thread_local wxThreadSpecificInfo info;
return info;

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1540640182@github.com>

VZ

unread,
May 9, 2023, 4:48:00 PM5/9/23
to wx-...@googlegroups.com, Subscribed

Alright, thanks for the feedback. I'll see if I can get the ThreadCleanUp call to work on all threads. I actually have an FLS-based TLS implementation somewhere which might be usable here...

Note that this would only be needed to support non-wxThread uses from C++98 programs. I think this should be quite rare...

I believe with C++11 thread_local would make the entire ThreadCleanUp obsolete: you could just implement the wxThreadSpecificInfo::Get() like: thread_local wxThreadSpecificInfo info; return info;

Hmm, yes, it could actually indeed be as simple as this. And if so, we should do it -- please don't hesitate making a PR against master simplifying this. TIA!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1540869619@github.com>

Antti Nietosvaara

unread,
May 10, 2023, 2:57:08 AM5/10/23
to wx-...@googlegroups.com, Subscribed

Note that this would only be needed to support non-wxThread uses from C++98 programs. I think this should be quite rare...

Well, the problem is there regardless of the C++ version in main program as long as wxWidgets itself uses C++98, is it not? Once wxWidgets switches to C++11 (as it has in master?) the workaround becomes obsolete, but meanwhile I'd like to have a workaround for stable 3.2.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1541450335@github.com>

Antti Nietosvaara

unread,
May 10, 2023, 7:41:04 AM5/10/23
to wx-...@googlegroups.com, Push

@nietsu pushed 1 commit.

  • f815455 Fix a memory leak when using non-wx threads.

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/push/13586810413@github.com>

Antti Nietosvaara

unread,
May 10, 2023, 8:25:57 AM5/10/23
to wx-...@googlegroups.com, Subscribed

Does this look more like what you had in mind? (overlooking the stray #include <wx/tls.h> I seem to have left in thread.cpp...)

I'm looking at master now as well. I think the entire threadinfo.h/cpp files might be obsolete. You could just declare thread_local logger and loggingdisabled in log.cpp and thread_local strings in wxTranslations::GetUntranslatedString. There's a comment about wxThreadSpecificInfo being about preserving TLS slots, and I don't think that's really a concern anymore with thread_local?

Alternatively I could just alter the wxThreadSpecificInfo::Get to use thread_local, remove the ThreadCleanUp, and keep the rest unchanged.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1542118044@github.com>

Antti Nietosvaara

unread,
May 10, 2023, 8:36:03 AM5/10/23
to wx-...@googlegroups.com, Push

@nietsu pushed 1 commit.

  • 24d29db Fix a memory leak when using non-wx threads.

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/push/13587504540@github.com>

VZ

unread,
May 10, 2023, 12:33:56 PM5/10/23
to wx-...@googlegroups.com, Subscribed

Just a quick note: apparently old MSVC and MinGW don't have the header, see AppVeyor build logs, e.g.

..\..\src\msw\thread.cpp(44) : fatal error C1083: Cannot open include file: 'fibersapi.h': No such file or directory

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1542501811@github.com>

VZ

unread,
May 10, 2023, 12:43:52 PM5/10/23
to wx-...@googlegroups.com, Subscribed

Does this look more like what you had in mind?

Yes, this looks good, thanks! We just need to declare the function and load it dynamically as we can't rely on having it in the headers/import library :-(

I'm looking at master now as well. I think the entire threadinfo.h/cpp files might be obsolete.

Yes, I think you're right.

You could just declare thread_local logger and loggingdisabled in log.cpp and thread_local strings in wxTranslations::GetUntranslatedString. There's a comment about wxThreadSpecificInfo being about preserving TLS slots, and I don't think that's really a concern anymore with thread_local?

I don't know how is thread_local implemented internally. I suspect that they already do the equivalent of what wx code does, i.e. use a single TLS slot to store all thread local variables as otherwise I don't see how could it work. But I'm not 100% sure.

Alternatively I could just alter the wxThreadSpecificInfo::Get to use thread_local, remove the ThreadCleanUp, and keep the rest unchanged.

This would be the simplest/fastest change, so if you don't have time to do the rest, doing just this would be already nice. But I'd welcome getting rid of it entirely too.

Thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1542513094@github.com>

Antti Nietosvaara

unread,
May 11, 2023, 5:33:38 AM5/11/23
to wx-...@googlegroups.com, Push

@nietsu pushed 1 commit.

  • b2c7714 Load FLS functions dynamically.

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/push/13598903565@github.com>

Antti Nietosvaara

unread,
May 11, 2023, 5:38:09 AM5/11/23
to wx-...@googlegroups.com, Subscribed

Another day, another version.

There was a note on Fls function documentation that they are not supported until Vista. I added Tls fallbacks as workaround, but they require the cleanup mechanic again, meaning that the leak is still there on Windows XP. Still an improvement I guess.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1543671254@github.com>

Antti Nietosvaara

unread,
May 11, 2023, 5:41:03 AM5/11/23
to wx-...@googlegroups.com, Push

@nietsu pushed 1 commit.

  • f8dcdf2 Load FLS functions dynamically.

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/push/13598998100@github.com>

Antti Nietosvaara

unread,
May 12, 2023, 1:57:15 AM5/12/23
to wx-...@googlegroups.com, Push

@nietsu pushed 1 commit.

  • 34fe2c9 Load FLS functions dynamically.

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/push/13610404765@github.com>

VZ

unread,
May 15, 2023, 8:23:59 PM5/15/23
to wx-...@googlegroups.com, Subscribed

Thanks for all this work, looks good to me now! I'm going to merge it soon with just some minor reformatting (to better conform to wx style) and rewording.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1548753558@github.com>

VZ

unread,
Jun 3, 2023, 5:15:03 PM6/3/23
to wx-...@googlegroups.com, Subscribed

Merged now, thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/c1575198052@github.com>

VZ

unread,
Jun 3, 2023, 5:15:03 PM6/3/23
to wx-...@googlegroups.com, Subscribed

Closed #23535.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/23535/issue_event/9422695788@github.com>

Reply all
Reply to author
Forward
0 new messages