Proposal: make wxSizerItem, wxMenuItemBase, wxToolBarToolBase inherit wxTrackable, to let bindings detect their destruction (Issue #26789)

[Jorge Moraleda]

jmoraleda created an issue (wxWidgets/wxWidgets#26789)

Proposal: make wxSizerItem, wxMenuItemBase, wxToolBarToolBase inherit wxTrackable, to let bindings detect their destruction

Background

wxPython (Phoenix/SIP) wraps C++ objects with Python proxy objects. SIP can automatically invalidate a Python wrapper on C++ object destruction only when the object was constructed through a special generated "shadow" subclass — which only happens when the object was constructed via a binding-visible constructor call. Invalidating a wrapper manually needs nothing but code that runs at the moment of destruction; the shadow-class destructor is simply the only such hook SIP currently has for objects it didn't construct itself.

wxSizerItem, wxMenuItemBase/wxMenuItem, and wxToolBarToolBase are normally constructed internally by wxWidgets itself (e.g. wxSizer::Add()/Insert(), wxMenu::Append(), wxToolBarBase::AddTool() via CreateTool()), not through their public constructors. Because no shadow instance is ever created on these paths, wxPython currently has no way to learn when the underlying C++ object is destroyed. The Python wrapper is left pointing at freed memory, and using it afterwards is a use-after-free (observed as a segfault, a silently stale object, or corrupted data depending on what has since reused the memory).

Full analysis and the binding-side investigation that led here: wxWidgets/Phoenix#2931

What we're asking

Add wxTrackable as an additional base class to these three classes, the same way wxEvtHandler already does:

// include/wx/event.h:3755-3756, for reference — the existing precedent
class WXDLLIMPEXP_BASE wxEvtHandler : public wxObject
                                     , public wxTrackable

Proposed:

class WXDLLIMPEXP_CORE wxSizerItem       : public wxObject, public wxTrackable { ... };
class WXDLLIMPEXP_CORE wxMenuItemBase    : public wxObject, public wxTrackable { ... };
class WXDLLIMPEXP_CORE wxToolBarToolBase : public wxObject, public wxTrackable { ... };

(wxMenuItemBase is the base every port's concrete wxMenuItem derives from, so this covers it the same way it covers wxGBSizerItem via wxSizerItem and each port's private tool class via wxToolBarToolBase.)

wxTrackable's destructor (include/wx/tracker.h) unconditionally walks its list of registered wxTrackerNodes and calls each one's OnObjectDestroy() when the object is destroyed — independent of how the object was constructed or which code deletes it. That's exactly the notification bindings are missing. wxWeakRef<T> already relies on this same mechanism generically.

On the binding side, SIP already provides everything needed to make use of this, with no further wxWidgets-side change beyond the base-class addition:

• sipEventWrappedInstance fires whenever a C/C++-constructed instance of a registered type is first wrapped, handing back the raw pointer (void *sipCpp). Since registration is per-type (sipRegisterEventHandler(sipEventWrappedInstance, td, handler)), the handler knows the concrete type and can safely convert the pointer to the object's wxTrackable base (adjusting for the multiple-inheritance offset) and call AddNode() on it with a small heap-allocated wxTrackerNode whose OnObjectDestroy() invalidates the Python wrapper.
• sipEventCollectingWrapper fires when a Python wrapper is being collected/removed while its C++ object is still alive, handing back the sipSimpleWrapper*. This is the paired hook needed to RemoveNode() and free the tracker node in that case, so it doesn't outlive the wrapper and later fire OnObjectDestroy() against freed node memory.

Both APIs, and the sipGetPyObject/sipInstanceDestroyedEx calls needed inside the handlers, are already generated and available to binding code (confirmed present in Phoenix's generated sipAPI_core.h). This is entirely Phoenix's/SIP's responsibility to implement, not wxWidgets' — described here only to show the base-class addition is sufficient on its own, with no other wxWidgets-side API needed.

Why this specific mechanism

• It requires no new API and no behavioral change to any existing method — wxTrackable is an established, already-shipping opt-in base class, and this proposal applies an existing idiom (already used by wxEvtHandler) to three more classes.
• It works regardless of which internal code path constructs or destroys the object, which is essential here: each of these three classes is built via multiple different internal call sites (e.g. per-port CreateTool() overrides for toolbar tools), so there is no single construction or destruction chokepoint a binding could otherwise intercept.
• wxTrackable's own destructor is intentionally non-virtual and protected specifically so it adds no vtable overhead and can't be used polymorphically on its own — it's designed to be mixed in exactly this way, as a passive second base.

Cost / impact

• ABI break: adding a base class changes object layout (and, since wxTrackable has member data — a linked-list head pointer — it adds one pointer's worth of size to each instance). Per docs/release.md, the 3.3.x series is the current development branch and explicitly does not guarantee API/ABI compatibility, so this fits there; it cannot be backported to the stable 3.2.x series.
• No change to any public method signature or documented behavior of the three classes.
• Runtime cost when untracked (the common case — most wxSizerItem/wxMenuItem/tool instances are never touched by a binding's tracking hook) is the same as wxEvtHandler already pays today: one extra pointer per instance, and an empty-list check in the destructor.

Scope

This is proposing the wxWidgets-side enabling change only. We are not asking wxWidgets to add any binding-specific code; the registration and invalidation logic is entirely Phoenix/SIP-side and we intend to implement and PR that ourselves once (if) this lands.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789@github.com>

[VZ]

vadz left a comment (wxWidgets/wxWidgets#26789)

I wonder why this couldn't be avoided by just always creating these objects in wxPython instead of letting wx do it? E.g. instead of wrapping wxSizer::Add(...) by calling it directly, rewrite it as

auto* item = new wxSizerItem(...);
wxSizer::Add(item);

This would give wxPython full control over the allocation.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5205058534@github.com>

[Jorge Moraleda]

jmoraleda left a comment (wxWidgets/wxWidgets#26789)

Thanks for the fast response. I checked this against the code: it appears to work cleanly for two of the three classes. But it doesn't reach the third, and there's one gap left in the other two — laid out below.

Where it works, with no wxWidgets change needed

wxSizerItem and wxMenuItem both already have exactly the chokepoint you're describing:

• wxSizer::Add/Insert/Prepend(wxSizerItem *item) — every other overload (Add(window, ...), Add(sizer, ...), Add(w, h, ...), the wxSizerFlags variants, and their Insert/Prepend equivalents) is an inline wrapper in sizer.h that constructs a wxSizerItem and ultimately calls DoInsert(size_t, wxSizerItem*) (sizer.cpp:908).
• wxMenuBase::Append(wxMenuItem *item) (→ DoAppend) — same shape; Append(id, text, ...) builds the item via wxMenuItem::New(...) and calls this.

Phoenix's generated code for the convenience overloads could be rerouted to construct the item via its real (Python-visible) constructor and call the existing pointer-taking overload instead. I tested this end to end against a full build, including through the window-teardown path described in the original issue (frame.Destroy() → ~wxWindowBase → sizer->Detach(window) → deletes the item), not just at construction time:

item = wx.SizerItem(btn)
sizer.Add(item)
panel.SetSizer(sizer)
frame.Destroy()
# sip.isdeleted(item) -> True

Because ~wxSizerItem and ~wxMenuItemBase are virtual, SIP's own shadow-class destructor fires via the vtable regardless of which code calls delete — so this reroute closes the window-teardown gap too, for anything constructed this way, with no other change needed.

Where it doesn't reach

The gap is objects wx constructs internally that Python only observes afterwards through a getter — nothing is ever routed through the pointer-taking overload, so there's no Python-visible construction call to reroute. For example, wxDialog::CreateStdDialogButtonSizer() builds the sizer and its items entirely in C++; Python only ever sees them via GetItem()/GetChildren(). I confirmed the sizer itself, and a spacer item inside it, are both never invalidated after the dialog is destroyed. Same shape applies to wxFileHistory::AddFilesToMenu() appending items Python later retrieves via GetMenuItems(), and to XRC-loaded sizer/menu trees.

wxToolBarToolBase can't use this approach at all, for an unrelated reason: it's abstract, and the concrete implementation is defined inside a .cpp/.mm file rather than a header on every port checked (GTK, MSW, Qt, Cocoa, iOS, Univ) — the one header reference found (msw/toolbar.h) is a forward declaration for a private method signature, not the class definition. There's no bindable type to construct, even for the tools Phoenix does create through AddTool(), so there's no Python-visible constructor to route through. No construction-side fix looks available here.

Where I'd like to land

I'd rather not submit a PR that covers the common case and leaves a use-after-free reachable through the less common one. So I still think the wxTrackable addition is worth doing, but narrower than the original post:

• Needed for wxToolBarToolBase, where the construction-side reroute can't apply.
• Necessary for wxSizerItem/wxMenuItem only for the observation-only paths that the reroute above doesn't reach — not for the common Python-constructed case.

What do you think?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5206602203@github.com>

[VZ]

vadz left a comment (wxWidgets/wxWidgets#26789)

I didn't have time to look at this in details yet (obviously, I'm just a human), but my feeling is that making these classes trackable trackable at wx level is not the right thing to do, wxTrackable is meant to be used for windows, whose lifetime may be difficult to handle otherwise (they can be closed by user), while for all these items it's pretty deterministic. It also feels wrong to do at wx level something that is only needed for wxPython (nobody is going to use wxWeakRef<wxMenuItem> in C++). My feeling could, of course, be wrong, but it would take an effort (not least from myself) in order to convince me that they are.

OTOH if you could make a PR changing wxToolBarTool so that it could be handled in the same way as the other classes, I would have absolutely zero qualms applying it because this would feel natural and I don't see any possible reason not to do it. So this would be definitely the past of least resistance, and by far.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5206722576@github.com>

[Jorge Moraleda]

jmoraleda left a comment (wxWidgets/wxWidgets#26789)

Thank you again. That's a fair objection — I'll drop the wxTrackable request.

I'd be glad to send the toolbar tool PR instead. I checked the concrete tool class on all six ports (GTK, MSW, Cocoa, Qt, iOS, Univ), and they all have the same basic structure: thin wxToolBarTool : public wxToolBarToolBase subclasses whose constructors forward to the base class, with only port-specific data members and (on most ports) destructor cleanup. The same two constructor signatures exist everywhere, with no apparent divergence. Univ already forward-declares wxToolBarTool in its own header (include/wx/univ/toolbar.h:15), so it is partly there already.

The PR I have in mind would be mechanical: move each port's class wxToolBarTool { ... } declaration out of its .cpp/.mm file and into the corresponding public header, while leaving the out-of-line method implementations where they are. No behavior change. I'll send it as a PR on that basis unless you'd rather I handle any of the ports differently.

Once the class declaration is available in the public headers, this would allow Phoenix (and other bindings) to expose it as a real, constructible type — similar to wxSizerItem or wxMenuItem. It is also a straightforward C++ visibility change rather than a new mechanism: no additional per-instance state and no extra constraints on the existing implementation.

One caveat

This covers the cases that originally motivated me to investigate the issue (discussion here). But it does not address cases where wxWidgets creates the object internally and Python only receives it later through a getter — for example wxDialog::CreateStdDialogButtonSizer(), wxFileHistory::AddFilesToMenu(), or XRC-loaded controls. Users relying on those paths could still encounter similar issues, and those cases would likely require a separate solution.

Thank you again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5207236556@github.com>

[Jorge Moraleda]

jmoraleda left a comment (wxWidgets/wxWidgets#26789)

Thanks for the quick and thoughtful feedback on this, and for the time you spent looking at it.

Following up on your suggestion to construct the items on the binding side: it works. wxSizerItem and wxMenuItem are already header-declared and Python-constructible, and their destructors are virtual, so SIP's own shadow-class destructor fires via the vtable no matter which code calls delete — including the case that was actually biting me, where a window's destructor removes itself from its sizer and deletes the item long before the sizer itself is destroyed. So those two need no wxWidgets change at all; the fix is entirely on the wxPython side, and that is where I will make it.

That leaves wxToolBarToolBase, which was the only remaining reason to change anything here. I did prototype exposing wxToolBarTool in the public headers on all six ports, and it does work — but it is a fair amount of churn (the Cocoa and iOS classes need their Objective-C++ members split out behind opaque typedefs to keep the shared header parseable as plain C++), and it would only be complete if wxAuiToolBar got similar treatment, since it has an analogous but distinct problem of its own. Weighed against the fact that this is the one of the three classes I have never actually seen cause a problem in practice, that is more disruption to core code than the benefit justifies.

So I am closing this rather than leaving a proposal open that I no longer think should be acted on. If someone does hit a concrete use-after-free with toolbar tools from a binding, the approach is straightforward enough to revisit then.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5225465459@github.com>

[Jorge Moraleda]

Closed #26789 as not planned.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issue/26789/issue_event/29155268318@github.com>

[Jorge Moraleda]

Reopened #26789.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issue/26789/issue_event/29156961627@github.com>

[VZ]

vadz left a comment (wxWidgets/wxWidgets#26789)

Unfortunately I don't know anything about SIP. With SWIG creation of all proxy objects of the specified type can be customized at its level, does SIP have anything like SWIG typemaps?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/26789/5232542659@github.com>