Fix FileSystemModel crash on stale filesystem-model indexes

- dispatch filesystem events on the main thread in FileSystemListener, preserving model, tree, document, and callback ordering
- add address + generation node identity to close the allocator-reuse hole
- return null/empty results for stale indexes instead of root data
- assert the main-thread contract and token-guard queued listener actions
- add attached-view and stale-selection crash regression tests

Fixes crash reported in discussion SpartanJ/ecode#952
This commit is contained in:
Martín Lucas Golini
2026-08-20 00:24:16 -03:00
parent 8448d579ab
commit 9c794fc12f
9 changed files with 392 additions and 75 deletions

View File

@@ -5,6 +5,7 @@
#include <atomic>
#include <memory>
#include <eepp/core/containers.hpp>
#include <eepp/system/fileinfo.hpp>
#include <eepp/system/threadpool.hpp>
#include <eepp/system/translator.hpp>
@@ -82,7 +83,7 @@ class EE_API FileSystemModel : public Model {
Node( const std::string& rootPath, FileSystemModel& model,
const std::shared_ptr<ThreadPool>& threadPool = {} );
Node( FileInfo&& info, Node* parent );
Node( FileInfo&& info, Node* parent, const FileSystemModel& model );
const std::string& getName() const { return mName; }
@@ -137,6 +138,8 @@ class EE_API FileSystemModel : public Model {
String mDisplayName;
std::string mMimeType;
Node* mParent{ nullptr };
const FileSystemModel* mModel{ nullptr };
Uint64 mId{ 0 };
FileInfo mInfo;
std::vector<Node*> mChildren;
bool mHasTraversed{ false };
@@ -181,6 +184,14 @@ class EE_API FileSystemModel : public Model {
void update();
const Node& node( const ModelIndex& index ) const;
/**
* @brief Returns the node for a valid index, or nullptr for a stale index
* (node deleted by a background refresh). An invalid index resolves to the
* root.
*/
const Node* nodePtr( const ModelIndex& index ) const;
virtual size_t treeColumn() const { return Column::Name; }
virtual size_t rowCount( const ModelIndex& = ModelIndex() ) const;
virtual size_t columnCount( const ModelIndex& = ModelIndex() ) const;
@@ -203,8 +214,18 @@ class EE_API FileSystemModel : public Model {
void setPreviouslySelectedIndex( const ModelIndex& previouslySelectedIndex );
/**
* @brief Processes a filesystem event (add/delete/move/modified).
*
* Must be called from the main thread when views are attached: the method
* mutates view selections and metadata. ecode's FileSystemListener
* dispatches watcher events to the main thread before calling this; direct
* callers are responsible for the same requirement.
*/
bool handleFileEvent( const FileEvent& event );
virtual bool isValid( const ModelIndex& index ) const override;
virtual bool classModelRoleEnabled() { return true; }
~FileSystemModel();
@@ -222,7 +243,12 @@ class EE_API FileSystemModel : public Model {
ModelIndex mPreviouslySelectedIndex{};
Node& nodeRef( const ModelIndex& index ) const;
Node* nodeRef( const ModelIndex& index ) const;
bool isNodeAlive( const Node* node, Uint64 id ) const;
mutable UnorderedSet<const Node*> mAliveNodes;
mutable Uint64 mNextNodeId{ 1 };
FileSystemModel( const std::string& rootPath, const Mode& mode,
const DisplayConfig& displayConfig, Translator* translator,

View File

@@ -100,7 +100,7 @@ class EE_API Model {
virtual bool isEditable( const ModelIndex& ) const { return false; }
bool isValid( const ModelIndex& index ) const {
virtual bool isValid( const ModelIndex& index ) const {
auto parentIndex = this->parentIndex( index );
return index.row() >= 0 && index.row() < (Int64)rowCount( parentIndex ) &&
index.column() >= 0 && index.column() < (Int64)columnCount( parentIndex );

View File

@@ -12,9 +12,13 @@ UIAbstractView::UIAbstractView( const std::string& tag ) :
UIScrollableWidget( tag ), mSelection( this ) {}
UIAbstractView::~UIAbstractView() {
eeSAFE_DELETE( mEditingDelegate );
// Unregister first so the view leaves the model's view set before its
// members are torn down. This is defense-in-depth only: view callbacks are
// safe because model events run on the main thread, not because of this
// ordering.
if ( mModel )
mModel->unregisterView( this );
eeSAFE_DELETE( mEditingDelegate );
}
UIAbstractView::SelectionKind UIAbstractView::getSelectionKind() const {

View File

@@ -6,6 +6,7 @@
#include <eepp/ui/models/filesystemmodel.hpp>
#include <eepp/ui/uiiconthememanager.hpp>
#include <eepp/ui/uiscenenode.hpp>
#include <eepp/window/engine.hpp>
#ifndef INDEX_ALREADY_EXISTS
#define INDEX_ALREADY_EXISTS eeINDEX_NOT_FOUND
@@ -17,12 +18,17 @@ namespace EE { namespace UI { namespace Models {
FileSystemModel::Node::Node( const std::string& rootPath, FileSystemModel& model,
const std::shared_ptr<ThreadPool>& threadPool ) :
mInfo( FileSystem::getRealPath( rootPath ) ) {
mModel( &model ), mInfo( FileSystem::getRealPath( rootPath ) ) {
mInfoDirty = false;
mName = FileSystem::fileNameFromPath( mInfo.getFilepath() );
mMimeType = "";
mHash = String::hash( mName );
mDisplayName = mName;
{
Lock l( model.mResourceLock );
mId = model.mNextNodeId++;
model.mAliveNodes.insert( this );
}
if ( threadPool ) {
mQueuedForTraversal = true;
threadPool->run( [this, &model]() {
@@ -35,13 +41,19 @@ FileSystemModel::Node::Node( const std::string& rootPath, FileSystemModel& model
}
}
FileSystemModel::Node::Node( FileInfo&& info, FileSystemModel::Node* parent ) :
mParent( parent ), mInfo( info ) {
FileSystemModel::Node::Node( FileInfo&& info, FileSystemModel::Node* parent,
const FileSystemModel& model ) :
mParent( parent ), mModel( &model ), mInfo( info ) {
mInfoDirty = false;
mName = FileSystem::fileNameFromPath( mInfo.getFilepath() );
mHash = String::hash( mName );
mDisplayName = mName;
updateMimeType();
{
Lock l( model.mResourceLock );
mId = model.mNextNodeId++;
model.mAliveNodes.insert( this );
}
}
const std::string& FileSystemModel::Node::fullPath() const {
@@ -110,6 +122,10 @@ FileSystemModel::Node::~Node() {
while ( mIsTraversing )
Sys::sleep( Milliseconds( 1 ) );
cleanChildren();
if ( mModel ) {
Lock l( mModel->mResourceLock );
mModel->mAliveNodes.erase( this );
}
}
FileSystemModel::Node* FileSystemModel::Node::createChild( const std::string& childName,
@@ -129,7 +145,7 @@ FileSystemModel::Node* FileSystemModel::Node::createChild( const std::string& ch
if ( node->mParent == this && node->mHash == hash )
return nullptr;
return eeNew( Node, ( std::move( file ), this ) );
return eeNew( Node, ( std::move( file ), this, model ) );
}
void FileSystemModel::Node::rename( const FileInfo& file ) {
@@ -159,7 +175,7 @@ ModelIndex FileSystemModel::Node::index( const FileSystemModel& model, int colum
return {};
for ( size_t row = 0; row < mParent->mChildren.size(); ++row ) {
if ( mParent->mChildren[row] == this )
return model.createIndex( row, column, const_cast<Node*>( this ) );
return model.createIndex( row, column, const_cast<Node*>( this ), mId );
}
eeASSERT( false );
return {};
@@ -227,7 +243,7 @@ bool FileSystemModel::Node::refresh( const FileSystemModel& model ) {
if ( node->info().isDirectory() && node->mHasTraversed )
node->refresh( model );
} else {
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this ) ) );
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this, model ) ) );
}
}
@@ -276,7 +292,7 @@ bool FileSystemModel::Node::traverseIfNeeded( const FileSystemModel& model ) {
if ( displayCfg.fileIsVisibleFn &&
!displayCfg.fileIsVisibleFn( file.getFilepath() ) )
continue;
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this ) ) );
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this, model ) ) );
} else {
accepted = false;
size_t psize = patterns.size();
@@ -298,7 +314,7 @@ bool FileSystemModel::Node::traverseIfNeeded( const FileSystemModel& model ) {
}
if ( accepted )
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this ) ) );
newChildren.emplace_back( eeNew( Node, ( std::move( file ), this, model ) ) );
}
}
}
@@ -452,6 +468,13 @@ void FileSystemModel::reload() {
}
void FileSystemModel::refresh() {
// NOTE: refresh() mutates the node tree (deletes stale nodes) on the
// calling thread, which may be a worker. The resource lock protects the
// mutation itself, but readers that dereference nodes without holding the
// lock (data(), rowCount(), index(), ...) can race with the deletion: the
// live-node registry is crash hardening, not full thread safety. Fully
// closing this race requires applying structural changes on the main
// thread or locking every access.
{
Lock l( resourceMutex() );
mRoot->refresh( *this );
@@ -465,26 +488,65 @@ void FileSystemModel::update() {
}
const FileSystemModel::Node& FileSystemModel::node( const ModelIndex& index ) const {
return nodeRef( index );
}
FileSystemModel::Node& FileSystemModel::nodeRef( const ModelIndex& index ) const {
if ( !index.isValid() )
return *mRoot;
Node* node = static_cast<Node*>( index.internalData() );
// Unchecked accessor: the index must be valid (or the root {}), and its
// node must still be alive. Callers that cannot guarantee this must use
// nodePtr() and handle null. Violating the precondition is undefined
// behavior (the debug assert fires; release dereferences the pointer).
Node* node = nodeRef( index );
eeASSERT( node != nullptr );
return *node;
}
const FileSystemModel::Node* FileSystemModel::nodePtr( const ModelIndex& index ) const {
return nodeRef( index );
}
FileSystemModel::Node* FileSystemModel::nodeRef( const ModelIndex& index ) const {
// An invalid index is the root; only a valid-looking index whose node is
// gone (deleted by a background refresh, or its address reused by a newer
// node) resolves to null.
if ( !index.isValid() )
return mRoot.get();
Node* node = static_cast<Node*>( index.internalData() );
// A stale index (node already deleted by a background refresh, or its
// address reused by a newer node) must not be dereferenced. The address +
// generation check rejects both cases; the next model update drops the
// stale index from the views. The check runs under the resource lock, but
// the returned pointer is used after the lock is released: a concurrent
// refresh() can still delete the node in between (see refresh()).
if ( !isNodeAlive( node, index.internalId() ) )
return nullptr;
return node;
}
bool FileSystemModel::isNodeAlive( const Node* node, Uint64 id ) const {
Lock l( mResourceLock );
return mAliveNodes.find( node ) != mAliveNodes.end() && node->mId == id;
}
bool FileSystemModel::isValid( const ModelIndex& index ) const {
if ( !index.isValid() )
return false;
Lock l( mResourceLock );
const Node* node = static_cast<const Node*>( index.internalData() );
if ( mAliveNodes.find( node ) == mAliveNodes.end() ||
node->mId != static_cast<Uint64>( index.internalId() ) )
return false;
return Model::isValid( index );
}
size_t FileSystemModel::rowCount( const ModelIndex& index ) const {
Node& node = const_cast<Node&>( this->node( index ) );
if ( node.mIsTraversing )
Node* node = nodeRef( index );
if ( !node )
return 0;
bool isThreaded = mThreadPool && &node == mRoot.get();
bool res = node.refreshIfNeeded( *this, isThreaded ? mThreadPool : nullptr );
if ( node->mIsTraversing )
return 0;
bool isThreaded = mThreadPool && node == mRoot.get();
bool res = node->refreshIfNeeded( *this, isThreaded ? mThreadPool : nullptr );
if ( isThreaded && res )
return 0;
if ( node.info().isDirectory() )
return node.mChildren.size();
if ( node->info().isDirectory() )
return node->mChildren.size();
return 0;
}
@@ -493,10 +555,12 @@ size_t FileSystemModel::columnCount( const ModelIndex& ) const {
}
bool FileSystemModel::hasChildren( const ModelIndex& index ) const {
Node& node = const_cast<Node&>( this->node( index ) );
if ( node.mInfoDirty )
node.fetchData( node.fullPath() );
return node.mInfo.isDirectory();
Node* node = nodeRef( index );
if ( !node )
return false;
if ( node->mInfoDirty )
node->fetchData( node->fullPath() );
return node->mInfo.isDirectory();
}
std::string FileSystemModel::columnName( const size_t& column ) const {
@@ -522,34 +586,37 @@ static std::string permissionString( const FileInfo& info ) {
Variant FileSystemModel::data( const ModelIndex& index, ModelRole role ) const {
eeASSERT( index.isValid() );
auto& node = this->nodeRef( index );
Node* node = nodeRef( index );
if ( !node )
return {};
switch ( role ) {
case ModelRole::Custom: {
return Variant( node.info().getFilepath().c_str() );
return Variant( node->info().getFilepath().c_str() );
}
case ModelRole::Sort: {
switch ( index.column() ) {
case Column::Icon:
return node.info().isDirectory() ? 0 : 1;
return node->info().isDirectory() ? 0 : 1;
case Column::Name:
return Variant( node.getName().c_str() );
return Variant( node->getName().c_str() );
case Column::Size:
return node.info().getSize();
return node->info().getSize();
case Column::Owner:
return node.info().getOwnerId();
return node->info().getOwnerId();
case Column::Group:
return node.info().getGroupId();
return node->info().getGroupId();
case Column::Permissions:
return Variant( permissionString( node.info() ) );
return Variant( permissionString( node->info() ) );
case Column::ModificationTime:
return node.info().getModificationTime();
return node->info().getModificationTime();
case Column::Inode:
return node.info().getInode();
return node->info().getInode();
case Column::Path:
return Variant( node.info().getFilepath().c_str() );
return Variant( node->info().getFilepath().c_str() );
case Column::SymlinkTarget:
return node.info().isLink() ? Variant( node.info().linksTo() ) : Variant( "" );
return node->info().isLink() ? Variant( node->info().linksTo() )
: Variant( "" );
default:
eeASSERT( false );
}
@@ -558,33 +625,34 @@ Variant FileSystemModel::data( const ModelIndex& index, ModelRole role ) const {
case ModelRole::Display: {
switch ( index.column() ) {
case Column::Icon:
return iconFor( node, index );
return iconFor( *node, index );
case Column::Name:
return Variant( &node.getDisplayName() );
return Variant( &node->getDisplayName() );
case Column::Size:
return Variant( FileSystem::sizeToString( node.info().getSize() ) );
return Variant( FileSystem::sizeToString( node->info().getSize() ) );
case Column::Owner:
return Variant( String::toString( node.info().getOwnerId() ) );
return Variant( String::toString( node->info().getOwnerId() ) );
case Column::Group:
return Variant( String::toString( node.info().getGroupId() ) );
return Variant( String::toString( node->info().getGroupId() ) );
case Column::Permissions:
return Variant( permissionString( node.info() ) );
return Variant( permissionString( node->info() ) );
case Column::ModificationTime:
return Variant( Sys::epochToString( node.info().getModificationTime() ) );
return Variant( Sys::epochToString( node->info().getModificationTime() ) );
case Column::Inode:
return Variant( String::toString( node.info().getInode() ) );
return Variant( String::toString( node->info().getInode() ) );
case Column::Path:
return Variant( node.info().getFilepath().c_str() );
return Variant( node->info().getFilepath().c_str() );
case Column::SymlinkTarget:
return node.info().isLink() ? Variant( node.info().linksTo() ) : Variant( "" );
return node->info().isLink() ? Variant( node->info().linksTo() )
: Variant( "" );
}
break;
}
case ModelRole::Icon: {
return iconFor( node, index );
return iconFor( *node, index );
}
case ModelRole::Class: {
return stylizeModel( index, &node );
return stylizeModel( index, node );
}
default: {
}
@@ -596,26 +664,29 @@ Variant FileSystemModel::data( const ModelIndex& index, ModelRole role ) const {
ModelIndex FileSystemModel::parentIndex( const ModelIndex& index ) const {
if ( !index.isValid() )
return {};
auto& node = this->node( index );
if ( !node.getParent() ) {
eeASSERT( &node == mRoot.get() );
Node* node = nodeRef( index );
if ( !node )
return {};
if ( !node->getParent() ) {
eeASSERT( node == mRoot.get() );
return {};
}
return node.getParent()->index( *this, index.column() );
return node->getParent()->index( *this, index.column() );
}
ModelIndex FileSystemModel::index( int row, int column, const ModelIndex& parent ) const {
if ( row < 0 || column < 0 )
return {};
auto& node = this->node( parent );
bool isThreaded = mThreadPool && &node == mRoot.get();
bool res =
const_cast<Node&>( node ).refreshIfNeeded( *this, isThreaded ? mThreadPool : nullptr );
Node* node = nodeRef( parent );
if ( !node )
return {};
bool isThreaded = mThreadPool && node == mRoot.get();
bool res = node->refreshIfNeeded( *this, isThreaded ? mThreadPool : nullptr );
if ( isThreaded && res )
return {};
if ( static_cast<size_t>( row ) >= node.mChildren.size() )
if ( static_cast<size_t>( row ) >= node->mChildren.size() )
return {};
return createIndex( row, column, node.mChildren[row] );
return createIndex( row, column, node->mChildren[row], node->mChildren[row]->mId );
}
UIIcon* FileSystemModel::iconFor( const Node& node, const ModelIndex& index ) const {
@@ -750,6 +821,13 @@ bool FileSystemModel::handleFileEventLocked( const FileEvent& event ) {
std::vector<ModelIndex> newIndexes;
view->getSelection().forEachIndex( [&]( const ModelIndex& selectedIndex ) {
Node* curNode = static_cast<Node*>( selectedIndex.internalData() );
if ( !isNodeAlive( curNode, selectedIndex.internalId() ) ) {
// Stale selection entry (node deleted by a background
// refresh): keep it untouched, the next model update
// drops it. Never dereference freed memory.
newIndexes.emplace_back( selectedIndex );
return;
}
if ( curNode->getParent() == parent ) {
if ( selectedIndex.row() >= (Int64)pos ) {
newIndexes.emplace_back( this->index( selectedIndex.row() + 1,
@@ -788,6 +866,10 @@ bool FileSystemModel::handleFileEventLocked( const FileEvent& event ) {
view->getSelection().removeAllMatching( [&]( auto& selectionIndex ) {
Node* node = static_cast<Node*>( index.internalData() );
Node* nodeSelected = static_cast<Node*>( selectionIndex.internalData() );
// A stale selection entry points at freed memory: drop it
// instead of dereferencing it.
if ( !isNodeAlive( nodeSelected, selectionIndex.internalId() ) )
return true;
return selectionIndex.internalData() == index.internalData() ||
( node->childCount() > 0 && nodeSelected->inParentTree( node ) );
} );
@@ -815,6 +897,10 @@ bool FileSystemModel::handleFileEventLocked( const FileEvent& event ) {
if ( !selectedIndex.isValid() )
return;
Node* curNode = static_cast<Node*>( selectedIndex.internalData() );
if ( !isNodeAlive( curNode, selectedIndex.internalId() ) ) {
newIndexes.emplace_back( selectedIndex );
return;
}
if ( curNode->getParent() == parent ) {
if ( selectedIndex.row() >= (Int64)pos ) {
auto newIndex =
@@ -925,6 +1011,10 @@ bool FileSystemModel::handleFileEventLocked( const FileEvent& event ) {
newIndexes.reserve( selections[view].size() );
for ( const ModelIndex& selectedIndex : selections[view] ) {
Node* selectedNode = static_cast<Node*>( selectedIndex.internalData() );
if ( !isNodeAlive( selectedNode, selectedIndex.internalId() ) ) {
newIndexes.emplace_back( selectedIndex );
continue;
}
ModelIndex newIndex = selectedNode->index( *this, selectedIndex.column() );
if ( newIndex.isValid() )
newIndexes.emplace_back( std::move( newIndex ) );
@@ -963,6 +1053,12 @@ bool FileSystemModel::handleFileEvent( const FileEvent& event ) {
if ( !mInitOK )
return false;
// Views are UI objects: this must run on the main thread. ecode's
// FileSystemListener dispatches watcher events to the main thread; direct
// callers are responsible for the same requirement. Without an Engine no
// view can exist, so the check is skipped.
eeASSERT( !Engine::existsSingleton() || Engine::isMainThread() );
bool ret;
{

View File

@@ -476,8 +476,8 @@ std::vector<const FileSystemModel::Node*> UIFileDialog::getSelectionNodes() cons
std::vector<const FileSystemModel::Node*> nodes;
nodes.reserve( localIndexes.size() );
for ( const auto& localIndex : localIndexes ) {
const FileSystemModel::Node& node = mModel->node( localIndex );
nodes.push_back( &node );
if ( const auto* node = mModel->nodePtr( localIndex ) )
nodes.push_back( node );
}
return nodes;
}

View File

@@ -1,12 +1,24 @@
#include "utest.hpp"
#include <eepp/graphics/fonttruetype.hpp>
#include <eepp/scene/scenemanager.hpp>
#include <eepp/system/filesystem.hpp>
#include <eepp/system/sys.hpp>
#include <eepp/ui/models/filesystemmodel.hpp>
#include <eepp/ui/models/persistentmodelindex.hpp>
#include <eepp/ui/uiscenenode.hpp>
#include <eepp/ui/uithememanager.hpp>
#include <eepp/ui/uitreeview.hpp>
#include <eepp/window/engine.hpp>
#include <filesystem>
#include <thread>
using namespace EE::UI::Models;
using namespace EE::System;
using namespace EE::Scene;
using namespace EE::UI;
using namespace EE::Window;
using namespace EE::Graphics;
namespace {
@@ -141,6 +153,20 @@ struct TempTree {
std::filesystem::path path;
};
static UISceneNode* initTreeViewTestScene() {
FileSystem::changeWorkingDirectory( Sys::getProcessPath() );
// Use a unique font name and skip applyDefaultTheme(): the default theme
// and the "NotoSans-Regular" resource-scope entry are shared globals that
// later tests (e.g. UIDiffView's UIApplication) replace and free.
FontTrueType* font = FontTrueType::New( "eepp-fsm-test-font" ).get();
font->loadFromFile( "../assets/fonts/NotoSans-Regular.ttf" );
UISceneNode* sceneNode = UISceneNode::New();
SceneManager::instance()->add( sceneNode );
SceneManager::instance()->setCurrentUISceneNode( sceneNode );
sceneNode->getUIThemeManager()->setDefaultFont( font );
return sceneNode;
}
} // namespace
UTEST( ModelMove, crossParentPersistentIndexesFollowNodesAndShiftSiblings ) {
@@ -271,3 +297,114 @@ UTEST( FileSystemModelMove, keepsUnopenedDestinationBranchLazy ) {
ASSERT_TRUE( model->getNodeFromPath( ( destination / "file.txt" ).string(), false, true ) !=
nullptr );
}
static ModelIndex indexOfNode( const FileSystemModel& model, const void* node,
const ModelIndex& parent = {} ) {
for ( Int64 row = 0; row < (Int64)model.rowCount( parent ); ++row ) {
ModelIndex idx = model.index( row, 0, parent );
if ( idx.isValid() && idx.internalData() == node )
return idx;
}
return {};
}
UTEST( FileSystemModelMove, deleteFallbackWithAttachedViewAndSelection ) {
Engine::instance()->createWindow( WindowSettings( 800, 600, "FileSystemModel test",
WindowStyle::Default, WindowBackend::Default,
32, {}, 1, false, true ),
ContextSettings( false, 0, 0, GLv_default, true, false ) );
initTreeViewTestScene();
TempTree tree;
auto source = tree.path / "source";
auto destination = tree.path / "outer" / "destination";
std::filesystem::create_directories( source / "folder" );
std::filesystem::create_directories( destination );
std::FILE* file = std::fopen( ( source / "folder" / "file.txt" ).string().c_str(), "wb" );
ASSERT_TRUE( file != nullptr );
std::fclose( file );
auto model = FileSystemModel::New( tree.path.string() );
auto* sourceNode = model->getNodeFromPath( ( source ).string(), true );
auto* folderNode = model->getNodeFromPath( ( source / "folder" ).string(), true );
auto* fileNode = model->getNodeFromPath( ( source / "folder" / "file.txt" ).string() );
ASSERT_TRUE( sourceNode != nullptr );
ASSERT_TRUE( folderNode != nullptr );
ASSERT_TRUE( fileNode != nullptr );
UITreeView* treeView = UITreeView::New();
treeView->setModel( model );
// Select the file inside the folder that will be moved away.
ModelIndex sourceIndex = indexOfNode( *model, sourceNode );
ModelIndex folderIndex = indexOfNode( *model, folderNode, sourceIndex );
ModelIndex fileIndex = indexOfNode( *model, fileNode, folderIndex );
ASSERT_TRUE( fileIndex.isValid() );
treeView->getSelection().set( fileIndex );
ASSERT_EQ( treeView->getSelection().size(), 1 );
// Moving the folder to an unopened destination falls back to deleting the
// materialized source node; the attached view and its selection must
// survive without crashing.
std::filesystem::rename( source / "folder", destination / "renamed" );
ASSERT_TRUE( model->handleFileEvent(
{ FileSystemEventType::Moved, destination.string() + EE::System::FileSystem::getOSSlash(),
"renamed", ( source / "folder" ).string() } ) );
ASSERT_TRUE( model->getNodeFromPath( ( source / "folder" ).string(), false, false ) ==
nullptr );
ASSERT_TRUE( treeView->getSelection().isEmpty() );
}
UTEST( FileSystemModelMove, deleteFallbackWithStaleSelectionDoesNotCrash ) {
Engine::instance()->createWindow( WindowSettings( 800, 600, "FileSystemModel test",
WindowStyle::Default, WindowBackend::Default,
32, {}, 1, false, true ),
ContextSettings( false, 0, 0, GLv_default, true, false ) );
initTreeViewTestScene();
TempTree tree;
auto source = tree.path / "source";
auto destination = tree.path / "outer" / "destination";
std::filesystem::create_directories( source / "folder" );
std::filesystem::create_directories( destination );
std::FILE* file = std::fopen( ( source / "folder" / "file.txt" ).string().c_str(), "wb" );
ASSERT_TRUE( file != nullptr );
std::fclose( file );
auto model = FileSystemModel::New( tree.path.string() );
auto* sourceNode = model->getNodeFromPath( ( source ).string(), true );
auto* folderNode = model->getNodeFromPath( ( source / "folder" ).string(), true );
auto* fileNode = model->getNodeFromPath( ( source / "folder" / "file.txt" ).string() );
ASSERT_TRUE( sourceNode != nullptr );
ASSERT_TRUE( folderNode != nullptr );
ASSERT_TRUE( fileNode != nullptr );
UITreeView* treeView = UITreeView::New();
treeView->setModel( model );
ModelIndex sourceIndex = indexOfNode( *model, sourceNode );
ModelIndex folderIndex = indexOfNode( *model, folderNode, sourceIndex );
ModelIndex fileIndex = indexOfNode( *model, fileNode, folderIndex );
ASSERT_TRUE( fileIndex.isValid() );
treeView->getSelection().set( fileIndex );
// Delete the file from disk and refresh from a background thread: the
// model drops the node without touching the view selection (the debounced
// selection cleanup is queued but has not run yet), leaving a stale
// selection entry pointing at freed memory.
std::filesystem::remove( source / "folder" / "file.txt" );
std::thread refresher( [&model]() { model->refresh(); } );
refresher.join();
ASSERT_EQ( treeView->getSelection().size(), 1 );
// Moving the folder to an unopened destination triggers the delete
// fallback, which used to dereference the stale selection entry.
std::filesystem::rename( source / "folder", destination / "renamed" );
ASSERT_TRUE( model->handleFileEvent(
{ FileSystemEventType::Moved, destination.string() + EE::System::FileSystem::getOSSlash(),
"renamed", ( source / "folder" ).string() } ) );
// The stale entry must have been dropped without crashing.
ASSERT_TRUE( treeView->getSelection().isEmpty() );
}

View File

@@ -1,6 +1,10 @@
#include "filesystemlistener.hpp"
#include <eepp/scene/actionmanager.hpp>
#include <eepp/scene/scenemanager.hpp>
#include <eepp/system/filesystem.hpp>
#include <eepp/system/md5.hpp>
#include <eepp/system/time.hpp>
#include <eepp/window/engine.hpp>
namespace ecode {
@@ -22,7 +26,28 @@ std::string getFileSystemEventTypeName( FileSystemEventType action ) {
FileSystemListener::FileSystemListener( UICodeEditorSplitter* splitter,
std::shared_ptr<FileSystemModel> fileSystemModel,
const std::vector<std::string>& ignoreFiles ) :
mSplitter( splitter ), mFileSystemModel( fileSystemModel ), mIgnoredFiles( ignoreFiles ) {}
mSplitter( splitter ), mFileSystemModel( fileSystemModel ), mIgnoredFiles( ignoreFiles ) {
// Namespaced 64-bit tag: a stable class-name prefix plus an atomic instance
// counter, which minimizes (but cannot mathematically eliminate) collision
// with unrelated scene action tags.
static std::atomic<Uint64> nextEventActionTag{ 0 };
mEventActionTag =
( static_cast<Uint64>( EE::String::hash( "ecode::FileSystemListener" ) ) << 32 ) |
( ++nextEventActionTag & 0xFFFFFFFFULL );
mLifetime = std::make_shared<std::atomic<bool>>( true );
}
FileSystemListener::~FileSystemListener() {
// The lifetime token is complete only because destruction runs on the main
// thread, where queued actions also execute: a runnable cannot be
// interleaved between its token check and its use of `this`. Assert that
// invariant; cross-thread destruction would need a synchronized control
// block instead of an atomic flag.
eeASSERT( !Engine::existsSingleton() || Engine::isMainThread() );
*mLifetime = false;
if ( auto* scene = SceneManager::instance()->getUISceneNode() )
scene->getActionManager()->removeActionsByTagFromTarget( scene, mEventActionTag );
}
static inline bool endsWithSlash( const std::string& dir ) {
return !dir.empty() && ( dir.back() == '\\' || dir.back() == '/' );
@@ -31,6 +56,27 @@ static inline bool endsWithSlash( const std::string& dir ) {
void FileSystemListener::handleFileAction( efsw::WatchID, const std::string& dir,
const std::string& filename, efsw::Action action,
const std::string& oldFilename ) {
// The whole logical event (model, directory tree, open documents,
// callbacks) must run on the main thread in its original order: the model
// update used to complete synchronously before the other consequences, and
// all of them touch UI-owned state. The destructor cancels queued actions
// (tagged with mEventActionTag), so a runnable can never outlive the
// listener.
if ( !Engine::isMainThread() ) {
if ( auto* scene = SceneManager::instance()->getUISceneNode() ) {
scene->runOnMainThread(
[lifetime = mLifetime, this, dir, filename, action, oldFilename]() {
if ( !lifetime->load( std::memory_order_acquire ) )
return;
handleFileAction( 0, dir, filename, action, oldFilename );
},
Time::Zero, mEventActionTag );
return;
}
// No scene node: cannot safely touch UI state from this thread.
return;
}
FileInfo file( ( endsWithSlash( dir ) ? dir : ( dir + FileSystem::getOSSlash() ) ) + filename );
switch ( action ) {

View File

@@ -25,7 +25,7 @@ class FileSystemListener : public efsw::FileWatchListener {
std::shared_ptr<FileSystemModel> fileSystemModel,
const std::vector<std::string>& ignoreFiles );
virtual ~FileSystemListener() {}
virtual ~FileSystemListener();
void handleFileAction( efsw::WatchID, const std::string& dir, const std::string& filename,
efsw::Action action, const std::string& oldFilename );
@@ -46,6 +46,11 @@ class FileSystemListener : public efsw::FileWatchListener {
std::unordered_map<Uint64, FileEventFn> mCbs;
std::vector<std::string> mIgnoredFiles;
Mutex mCbsMutex;
// Tag of the queued main-thread file-event actions; the destructor cancels
// them and marks the lifetime token dead so a queued runnable can never
// dereference the destroyed listener.
Uint64 mEventActionTag{ 0 };
std::shared_ptr<std::atomic<bool>> mLifetime;
bool isFileOpen( const FileInfo& file );

View File

@@ -137,7 +137,9 @@ class UITreeViewCellFS : public UITreeViewCell {
}
const std::string& getCurrentPath() const {
return getModel()->node( getCurIndex() ).fullPath();
static const std::string empty;
const auto* node = getModel()->nodePtr( getCurIndex() );
return node ? node->fullPath() : empty;
}
protected:
@@ -343,9 +345,9 @@ void UITreeViewFS::copyFiles( const std::vector<std::string>& paths, const std::
}
std::string UITreeViewFS::getSelectionPath() const {
return static_cast<const FileSystemModel*>( getModel() )
->node( getSelection().first() )
.fullPath();
const auto* node =
static_cast<const FileSystemModel*>( getModel() )->nodePtr( getSelection().first() );
return node ? node->fullPath() : "";
}
std::string UITreeViewFS::getSelectionPathAtIndex( int index ) const {
@@ -353,9 +355,9 @@ std::string UITreeViewFS::getSelectionPathAtIndex( int index ) const {
if ( index < 0 || static_cast<size_t>( index ) >= static_cast<size_t>( selection.size() ) )
return "";
auto indexVec = selection.indexes();
return static_cast<const FileSystemModel*>( getModel() )
->node( indexVec[static_cast<size_t>( index )] )
.fullPath();
const auto* node = static_cast<const FileSystemModel*>( getModel() )
->nodePtr( indexVec[static_cast<size_t>( index )] );
return node ? node->fullPath() : "";
}
std::vector<FileInfo> UITreeViewFS::getSelectionsFileInfo() const {
@@ -363,7 +365,8 @@ std::vector<FileInfo> UITreeViewFS::getSelectionsFileInfo() const {
auto indexVec = getSelection().indexes();
auto model = static_cast<const FileSystemModel*>( getModel() );
for ( const auto& index : indexVec ) {
ret.emplace_back( FileInfo( model->node( index ).fullPath() ) );
if ( const auto* node = model->nodePtr( index ) )
ret.emplace_back( FileInfo( node->fullPath() ) );
}
return ret;
}