Skip to content

DirectoryTreeCtrl: avoid wxNullFont crash on Windows revert path - #830

Merged
mrjimenez merged 2 commits into
amule-project:masterfrom
got3nks:fix/dir-tree-revert-font-crash
Jun 3, 2026
Merged

DirectoryTreeCtrl: avoid wxNullFont crash on Windows revert path#830
mrjimenez merged 2 commits into
amule-project:masterfrom
got3nks:fix/dir-tree-revert-font-crash

Conversation

@got3nks

@got3nks got3nks commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #827.

Two related fixes

1. ApplyRecursiveMark: avoid wxNullFont crash on Windows

ApplyRecursiveMark(hItem, false) passed wxNullFont to wxTreeCtrl::SetItemFont to clear the bold-italic recursive-share marker. On wxMSW 3.2 this crashes: wxTreeCtrl::SetItemFont calls wxFont::WXAdjustToPPI() on the font for DPI adjustment, and WXAdjustToPPI dereferences the font's refdata without a null check. wxNullFont has no refdata → access violation:

wxFont::WXAdjustToPPI + 0x13   mulsd xmm0, [rsi+78h] with rsi = NULL
wxTreeCtrl::SetItemFont + 0xaa
CDirectoryTreeCtrl::ApplyRecursiveMark
CDirectoryTreeCtrl::OnRButtonDown

The wxNullFont path is null-safe on wxGTK / wxOSX, which is why we didn't see it locally. Fix: pass GetFont() (the tree's own font, guaranteed valid refdata) instead.

2. CheckChanged / OnRButtonDown: keep per-item font and bold flag in sync

Once a per-item font is set on wxMSW, it wins over TVIS_BOLD at render time. The fix above leaves a plain GetFont() sticky on the item after a recursive→plain toggle, which makes a subsequent double-click's SetItemBold(true) render unbold:

  1. Right-click X → bold-italic (recursive marker)
  2. Right-click X → plain
  3. Double-click X → SetItemBold(true) runs, but the plain per-item font from step 2 still wins → displayed unbold (was the regression)

Two changes:

  • CheckChanged mirrors the new bold state into the per-item font, so wxMSW renders consistently.
  • OnRButtonDown reorders: MarkChildren runs first (CheckChanged resets the per-item font), then ApplyRecursiveMark(true) overlays the bold-italic marker on the recursive-share path. The redundant ApplyRecursiveMark(false) call on the unshare path is dropped.

wxGTK / wxOSX are unaffected (they layer TVIS_BOLD over the per-item font).

Test plan

  • macOS: compiles, no behavioral change.
  • Windows: full cycle verified on an ARM64 Windows 11 VM — right-click → bold-italic, right-click → plain, double-click → plain bold, double-click → plain. Original crash repro from aMule crashes selecting a shared directory #827 (double-click + right-click on bold) no longer crashes.

got3nks added 2 commits June 3, 2026 10:39
ApplyRecursiveMark(hItem, false) passed wxNullFont to wxTreeCtrl::SetItemFont
to clear the bold-italic recursive marker and revert to the tree's default
font.

On wxMSW 3.2 this crashes. wxTreeCtrl::SetItemFont calls
wxFont::WXAdjustToPPI() on the supplied font for DPI adjustment, and
WXAdjustToPPI dereferences the font's refdata without a null check.
wxNullFont has no refdata, so the deref segfaults:

  wxFont::WXAdjustToPPI + 0x13   mulsd xmm0, [rsi+78h] with rsi = NULL
  wxTreeCtrl::SetItemFont + 0xaa
  CDirectoryTreeCtrl::ApplyRecursiveMark
  CDirectoryTreeCtrl::OnRButtonDown

Reproduction (amule-project#827, Windows 11):
  1. Preferences -> Directories
  2. Double-click a folder (it becomes bold via OnItemActivated /
     SetItemBold).
  3. Right-click the now-bold item -> OnRButtonDown takes the
     "unshare" branch -> ApplyRecursiveMark(hItem, false) -> crash.

The wxNullFont path is null-safe on wxGTK / wxOSX, which is why the
crash only manifests on Windows and we didn't see it locally.

Pass GetFont() (the tree's own font, guaranteed to have valid refdata)
instead of the wxNullFont sentinel. Functionally identical -- both mean
"no custom item-specific font" -- but the wxMSW PPI-adjust path now has
something real to read.
The wxNullFont -> GetFont() workaround in d02ef74 introduced a
state-machine regression on wxMSW: once a per-item font is set on
a tree item, wxMSW honors it over TVIS_BOLD, so a later
SetItemBold(true) no longer renders bold.

User-visible bug after the previous commit:

  1. Right-click folder X       -> bold-italic (recursive marker)
  2. Right-click X again        -> per-item font cleared to plain
  3. Double-click X             -> SetItemBold(true) runs, but the
                                   plain per-item font from step 2
                                   still wins -> displayed unbold.

Two changes keep the per-item font and the bold flag in sync:

  - CheckChanged() now also calls SetItemFont() to mirror the new
    bold state into the per-item font. wxGTK / wxOSX are unaffected
    (they layer TVIS_BOLD over the per-item font).

  - OnRButtonDown() reorders so MarkChildren runs first (whose
    CheckChanged on the right-clicked item resets the per-item
    font to plain on unshare, or to plain-bold on share); the
    bold-italic ApplyRecursiveMark(true) is then overlaid LAST on
    the share path so CheckChanged doesn't clobber it. The
    ApplyRecursiveMark(false) call on the unshare path is now
    redundant (CheckChanged handles it) and dropped.

After these changes the user's step 3 displays bold as expected.
@mrjimenez
mrjimenez merged commit abe9d5e into amule-project:master Jun 3, 2026
7 checks passed
@got3nks
got3nks deleted the fix/dir-tree-revert-font-crash branch June 3, 2026 14:16
got3nks added a commit to got3nks/amule that referenced this pull request Jun 4, 2026
…ndex

Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(amule-project#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg
  amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796
  (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths).

- Bug Fixes & Stability: post-amule-project#744 fixes including EC notification
  leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728),
  amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760),
  PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788,
  amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756),
  Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833,
  amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR
  (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for
  date+version drift (amule-project#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
got3nks added a commit to got3nks/amule that referenced this pull request Jun 4, 2026
…ndex

Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(amule-project#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg
  amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796
  (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths).

- Bug Fixes & Stability: post-amule-project#744 fixes including EC notification
  leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728),
  amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760),
  PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788,
  amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756),
  Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833,
  amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR
  (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for
  date+version drift (amule-project#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
mrjimenez pushed a commit that referenced this pull request Jun 4, 2026
Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for #785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), #794 (.dmg
  amuleweb path), #789 (<OS>-<arch> artifact naming), #780 / #796
  (Windows DPI + comctl32 manifest), #784 (FHS share/amule paths).

- Bug Fixes & Stability: post-#744 fixes including EC notification
  leak (#797), big-library scaling (#736, #840 superseding #728),
  amulegui ghost entries (#810, #819, #841, #824, #830, #760),
  PartFile early hash (#762), server protocol fixes (#835, #788,
  #721, #787), crypto stream UB (#779), UAF prevention (#756),
  Kad rotation (#795, #799/#805), GTK warning silencing (#833,
  #826/#836), and the clang-tidy worklist (#770, #772-#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (#753/#754/#776), Galician (#763), Slovenian (#771), pt-BR
  (#768/#775/#812), French (#811), plus man-page tooling for
  date+version drift (#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (#817/#818/#821/#828/#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with #746-#845 + #841.
got3nks added a commit to got3nks/amule that referenced this pull request Aug 7, 2026
…e-project#180, amule-project#801) (amule-project#839)

* feat(gui): port CFileDetailListCtrl to CMuleVirtualDataViewCtrl (amule-project#180, amule-project#801)

Continues the wxListCtrl -> wxDataViewCtrl migration (amule-project#180/amule-project#801) onto
the shared base from amule-project#811, after Search (amule-project#796), Servers (amule-project#807/amule-project#811)
and Friends (amule-project#830). CFileDetailListCtrl was next because it has no
CBarShader rendering (unlike Downloads/SharedFiles/Sources-Peers,
which additionally need a still-nonexistent wxDataViewCustomRenderer).

Its caller, CFileDetailDialog::FillSourcenameList(), drove the old
list with raw position-indexed CRUD (FindItem by name, SetItemPtrData,
SetItem by column index, DeleteItem) rather than the pointer-identity
style CServerListCtrl/CFriendListCtrl already use, so porting the
control cleanly meant refactoring the caller too:

- CFileDetailDialog gained a std::map<wxString, SourcenameItem *>
  m_sourcenames member, replacing "search the list widget by name"
  with a map lookup. FillSourcenameList()'s reset/update/prune shape
  is otherwise unchanged, just re-keyed to the map and driven through
  the control's new AddSource/RefreshSource/RemoveSource API.
- OnBnClickedTakeOver()/OnListClickedTakeOver() resolve the selected
  row through GetSelectedItemData() (inherited from
  CMuleVirtualDataViewCtrl) instead of GetNextItem+GetItemText.
- The dialog's EVT_LIST_ITEM_ACTIVATED binding on IDC_LISTCTRLFILENAMES
  becomes EVT_DATAVIEW_ITEM_ACTIVATED; wxDataViewEvent is a
  wxNotifyEvent/wxCommandEvent descendant so it still propagates from
  the child control to the dialog-level handler the same way.
- Fixed a latent leak: nothing in ~CFileDetailDialog() freed the
  SourcenameItem objects still referenced by open rows -- only the
  "not a partfile" and "pruned to zero" paths in FillSourcenameList()
  ever deleted them. Now freed in the destructor via m_sourcenames.

CMuleDataViewCtrl always ORs in wxDV_MULTIPLE (no single-selection
mode exists on the shared base), but this list's "take over filename"
actions assume exactly one selection, same as the old list did (it
never actually requested wxLC_SINGLE_SEL either -- checked
muuli_wdr.cpp -- it enforced single-selection itself via OnSelect()
deselecting every other row). CFileDetailListCtrl::OnSelectionChanged()
does the equivalent: collapses to the just-clicked row whenever more
than one ends up selected.

Also deleted a vestigial nested SourcenameItem struct in
FileDetailListCtrl.h that duplicated (by accident of matching layout)
the real one in PartFile.h, and dropped a per-row background-colour
set that reproduced the default and carried its own "do we still need
this?" comment.

Verified: builds clean; clang-format v18 (pinned Docker image) applied;
clang-tidy Tier-1 (whole-tree) and Tier-2 (changed-lines,
.clang-tidy-new-code) both clean via the ~/aMuleTest/ci-local replica.
Grepped for other CFileDetailListCtrl/IDC_LISTCTRLFILENAMES references
to confirm muuli_wdr.cpp's construction call needed no changes.
Interactive verification (source list populate/re-tally, take-over via
button and double-click, single-selection enforcement, sort, VoiceOver)
left for manual testing per project convention.

* fix(gui): keep the source-name list sorted, and drop rows before freeing them

Review follow-up to the port.

FillSourcenameList() zeroes every count and then rewrites them in place, so
while it runs the list is not ordered by the column it is sorted on -- and
AddSource() places a new row with a binary search, which needs that ordering
to hold. An insertion therefore leaves rows in arbitrary positions, and the
repair has to happen here, exactly as the pre-port code did.

Only on insertion, though. A count that merely changed is what the live-sort
preference governs: RefreshSource() re-sorts when it is on, and when it is
off the row is meant to stay put rather than move under the user. Sorting
unconditionally would quietly override that setting for this list.

The destructor and the not-a-partfile path also freed the SourcenameItem
objects while the list control still held pointers to them. Nothing can
paint or sort in either window today -- both dialog call sites are stack
temporaries, so destruction is synchronous -- but that rests on wx's teardown
order rather than on anything guaranteed, and the base states the rule
plainly: it has to be told before the caller frees the item data. Both now
clear the rows first, as the prune loop already did.

* fix(gui): decide the source list's live re-sort per column

IsLiveSortColumn() answered true for every column, so a refresh tick
scheduled a re-sort even when the list was sorted by File Name -- a value
that never changes, since the name is the key each row was created under.

Answer per column instead, the shape CServerListCtrl already uses, so only a
sources-sorted list re-sorts on its own. The header comment already
described it this way; only the implementation did not.

* chore: drop a duplicated paragraph from the file header

FileDetailDialog.cpp carried the "Any parts of this program derived from
the xMule, lMule or eMule project" paragraph twice. It is the only file in
src/ that does, and the file is already being touched here.

---------

Co-authored-by: got3nks <[email protected]>
mrjimenez pushed a commit to mrjimenez/amule that referenced this pull request Aug 8, 2026
…oject#180, amule-project#801) (amule-project#830)

* feat(gui): port CFriendListCtrl to CMuleVirtualDataViewCtrl (amule-project#180, amule-project#801)

Continues the wxListCtrl -> wxDataViewCtrl migration (amule-project#180/amule-project#801) onto
the shared base PR amule-project#811 extracted after the Servers port (amule-project#807, now
absorbed into amule-project#811). CFriendListCtrl was picked next after reading all
the remaining candidates directly rather than going by line count
alone: it has no CBarShader rendering (unlike Downloads/SharedFiles/
Sources-Peers, which additionally need a still-nonexistent
wxDataViewCustomRenderer), it's already pointer-identity addressed
(UpdateFriend(CFriend*)/RemoveFriend(CFriend*) map directly onto
AddItemData/RefreshItemData/RemoveItemData), and its blast radius is
tiny (ChatWnd.cpp, muuli_wdr.cpp construction only).

CFileDetailListCtrl looked smaller by line count but its caller
(FileDetailDialog.cpp) drives it with raw position-indexed CRUD
(FindItem by name, SetItem by column index, DeleteItem) plus a
pre-existing duplicate-type quirk (two unrelated SourcenameItem
structs relying on compatible layout) -- porting it cleanly means
refactoring the caller too, which is a separate, more invasive piece
of work than this one.

CServerListCtrl (current, post-amule-project#811) is the template mirrored here:
same AppendTextColumn/AppendSpacerColumn/AssociateVirtualModel/
LoadColumnSettings/InitColumnState ctor sequence, same GetItemColumnText/
GetItemAttr/CompareItemData/OnListKey hook shape. Public API
(UpdateFriend, RemoveFriend) and the constructor signature are
unchanged, so ChatWnd.cpp and muuli_wdr.cpp needed no edits.

Notable deltas from the pre-port behaviour: added a real CompareItemData
so header-click sort now works (the old list never called SetSortFunc
at all); GetItemAttr replaces the old SetItemTextColour call with the
same visible result (blue for linked friends, default text colour
otherwise).

Verified: builds clean; clang-format v18 (pinned Docker image) applied;
clang-tidy Tier-1 (whole-tree) and Tier-2 (changed-lines,
.clang-tidy-new-code) both clean via the ~/aMuleTest/ci-local replica
-- Tier-2 caught one real modernize-use-nullptr hit, fixed. Grepped
ChatWnd.cpp/.h and muuli_wdr.cpp to confirm no other call sites exist.
Interactive verification (sort, right-click menu states, Delete-key
removal, chat-session activation, VoiceOver) left for manual testing
per project convention, same as PR amule-project#796/amule-project#807.

* fix(gui): resolve activated friend through selection, not row-as-pointer

got3nks's review on amule-project#830 found a real crash: OnItemActivated() cast
event.GetItem()'s ID directly to CFriend* on the assumption it was the
item's data pointer, but CMuleVirtualDataViewCtrl's row-addressed
model returns the row index (+1) as that ID -- the "item identity is
not row identity" case MuleVirtualDataViewCtrl.h itself documents.
Every double-click/Enter on a friend dereferenced a bogus pointer.
Fixed by selecting the activated row and resolving it through
GetSelectedItemData(), matching CServerListCtrl::OnItemActivated.

Also from the same review: IsLiveSortColumn() now returns true, since
the name (the only sortable column) can change after a friend is
already listed and the UpdateFriend() comment claimed a re-sort that
the base's default-false hook never actually triggered; and dropped a
no-op static_cast<int>() around a call that already returns int.

Verified: builds clean, clang-format v18 applied, clang-tidy Tier-2
(changed lines, .clang-tidy-new-code) clean via the local CI replica.

* chore: retrigger CI

The previous run hit a GitHub Actions infrastructure outage (job not
acquired by any runner, "Failed to resolve action download info" /
"Service Unavailable" on clang-format, Translation checks, mingw-w64
Debug and clang-tidy Tier-1) unrelated to this branch's code -- every
job that did run passed. No admin rights to rerun the failed jobs
directly, so retriggering with an empty commit instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aMule crashes selecting a shared directory

2 participants