View Issue Details

IDProjectCategoryView StatusLast Update
0005508DarkRadiantSaving and loadingpublic05.09.2021 18:21
Reporterjonri Assigned Togreebo  
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionfixed 
PlatformArch Linux 
Product Version2.11.0 
Target Version2.11.0Fixed in Version2.11.0 
Summary0005508: Crash in master when loading a map
DescriptionThis one has been there since at least 0005493 was fixed in commit 40e534bf74b0b5f1decc012f49db332c0751ce79. It may have been introduced earlier but wasn't getting hit due to the previous crash.

This happens when loading a more fully featured map, but not when loading any of my dedicated map files that just contain my modular components. A brand new map with nothing in it won't crash either. I'm still in the process of adding things one-by-one until I create a map that crashes, but figured I'd post the bug now in case the location and stack trace are more familiar to you.

The crash happens immediately after the loading is complete and the map gets rendered to the window for the first time.
Steps To Reproduce1. Open DarkRadiant.
2. Open a fully-featured map. My own caused a crash, and I also confirmed the crash with Perilous Refuge (peril2).
Additional InformationScreenshot with debug information is attached, "owningNode" appears to be invalid. Stack trace isn't super helpful, it's all WX so the actual caller probably posted an event.

Let me know if you'd like me to help narrow down which map feature is causing the crash.
TagsNo tags attached.

Relationships

related to 0005513 confirmed Change wxDataViewItem ID from TreeModel::Node* to something safer 

Activities

jonri

jonri

25.01.2021 20:42

developer  

jonri

jonri

25.01.2021 20:58

developer   ~0013537

P.S the latest master currently needs the usual round of trivial Linux fixes for a couple missing includes, but didn't want to toss it into the middle of what you were working on :-)
greebo

greebo

26.01.2021 03:18

administrator   ~0013538

Hm, this stracktrace looks familiar, I think already fixed the same or very similar issue. It's caused by the optimistic approach of the wxGTK implementation posting idle events to expand ancestors, ignoring the possibility that the wxDataViewItem might actually be invalid by the time the idle event kicks in. There's no way to clear that event or clear the item unless somebody calls EnsureVisible(wxDataViewItem(nullptr)) after changing the model.
jonri

jonri

26.01.2021 23:30

developer   ~0013550

I've done some more digging and I believe you are correct. I'm 100% certain I'm still experiencing 0005498, even after your fix was applied. I can prevent the crash by removing the "LastShaderClipboardMaterial" key/value from the map.darkradiant file.

I see your `EnsureVisible(wxDataViewItem(nullptr))` that you added to mitigate this issue getting called early on, but if I put a breakpoint on that line and also on both `EnsureVisible(item)` lines in ResourceTreeView.cpp, I see the the latter getting called last, after the map is loaded and rendered but before the crash. I assume this would lead to that idle event getting reactivated and causing the crash.

If you can't reproduce 0005498 in the latest master when opening a map that contains a "LastShaderClipboardMaterial", let me know and I can send you the stack trace showing where `EnsureVisible(item)` is getting called.
greebo

greebo

28.01.2021 18:07

administrator   ~0013555

Last edited: 28.01.2021 18:07

There must be a code path I missed when placing that fix calling EnsureVisible(null). If ResourceTreeView.cpp:EnsureVisible is called last, there must be something happening in between that removes the model from the view (or clears the items). Can you maybe figure out if the AssociateModel() method is invoked after the item is scheduled by EnsureVisible(item)?

Thing is, the MediaBrowser is reloaded once a map is finished loading. The ShaderClipboard notifier (LastShaderClipboardMaterial) schedules an item selection in the MediaBrowser, which is cleared almost immediately afterwards by the repopulator (at least I think that's the trigger) and the scheduled item is no longer valid. We need to figure out why the fix in AssociateModel is not enough in this case.
jonri

jonri

28.01.2021 18:33

developer   ~0013556

If I load a map without the LastShaderClipboardMaterial, I do see AssociateModel() getting called, presumably as you described. However, it happens as a result of ResourceTreeView::_onTreeStorePopulationFinished and that is a callback for a WX event. I imagine the the expand ancestors event is somehow getting scheduled ahead of the population finished event.

I'll keep playing with this and let you know what else I find.
greebo

greebo

28.01.2021 18:58

administrator   ~0013557

Last edited: 28.01.2021 18:59

I found the reason - it's the Clear() call that is happening before the populator thread is starting. I'll have to think about how to rearrange that whole thing to make it safe.
To be honest, I don't like the wxGTK implementation of scheduling an idle event with a dataviewitem that might be out of date by the time it's processed. There's no wxDataViewModelNotifier listening for item removed events to cancel the callback if the item is stale.

The real solution would be to not embed the actual pointers to our Node structures in the wxDataViewItem, since there's no way to know whether the pointer is valid or stale. Some other ID that can be checked for validity would be nice, but this is of course some more work. The cheap solution of placing an EnsureVisible(null) in ResourceTreeView::Clear() would fix this particular crash, but it's not even close to be waterproof.

Related Changesets

DarkRadiant: master 8024e813

29.01.2021 04:02

greebo


Details Diff
0005508: Fix crash in Linux due to stale wxDataViewItems kept by the wxDataViewCtrl implementation Affected Issues
0005508
mod - libs/wxutil/dataview/ResourceTreeView.cpp Diff File

Issue History

Date Modified Username Field Change
25.01.2021 20:42 jonri New Issue
25.01.2021 20:42 jonri File Added: Screenshot_20210125_150817.png
25.01.2021 20:58 jonri Note Added: 0013537
26.01.2021 03:19 greebo Note Added: 0013538
26.01.2021 03:19 greebo Status new => acknowledged
26.01.2021 23:30 jonri Note Added: 0013550
28.01.2021 18:07 greebo Status acknowledged => feedback
28.01.2021 18:07 greebo Note Added: 0013555
28.01.2021 18:07 greebo Note Edited: 0013555
28.01.2021 18:33 jonri Note Added: 0013556
28.01.2021 18:33 jonri Status feedback => new
28.01.2021 18:58 greebo Note Added: 0013557
28.01.2021 18:59 greebo Note Edited: 0013557
28.01.2021 19:04 greebo Assigned To => greebo
28.01.2021 19:04 greebo Status new => assigned
29.01.2021 03:54 greebo Relationship added related to 0005513
29.01.2021 04:03 greebo Changeset attached => DarkRadiant master 8024e813
29.01.2021 04:03 greebo Target Version => 2.11.0
29.01.2021 04:03 greebo Status assigned => resolved
29.01.2021 04:03 greebo Resolution open => fixed
29.01.2021 04:03 greebo Fixed in Version => 2.11.0
05.09.2021 18:21 greebo Status resolved => closed