From 9c794fc12f3b3af2d5e1e818d9a7740da7fc2a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Lucas=20Golini?= Date: Thu, 20 Aug 2026 00:24:16 -0300 Subject: [PATCH] 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 --- include/eepp/ui/models/filesystemmodel.hpp | 30 ++- include/eepp/ui/models/model.hpp | 2 +- src/eepp/ui/abstract/uiabstractview.cpp | 6 +- src/eepp/ui/models/filesystemmodel.cpp | 214 +++++++++++++----- src/eepp/ui/uifiledialog.cpp | 4 +- .../unit_tests/modeloperations_tests.cpp | 137 +++++++++++ src/tools/ecode/filesystemlistener.cpp | 48 +++- src/tools/ecode/filesystemlistener.hpp | 7 +- src/tools/ecode/uitreeviewfs.cpp | 19 +- 9 files changed, 392 insertions(+), 75 deletions(-) diff --git a/include/eepp/ui/models/filesystemmodel.hpp b/include/eepp/ui/models/filesystemmodel.hpp index 4a39ba4e5..972735f2d 100644 --- a/include/eepp/ui/models/filesystemmodel.hpp +++ b/include/eepp/ui/models/filesystemmodel.hpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -82,7 +83,7 @@ class EE_API FileSystemModel : public Model { Node( const std::string& rootPath, FileSystemModel& model, const std::shared_ptr& 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 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 mAliveNodes; + mutable Uint64 mNextNodeId{ 1 }; FileSystemModel( const std::string& rootPath, const Mode& mode, const DisplayConfig& displayConfig, Translator* translator, diff --git a/include/eepp/ui/models/model.hpp b/include/eepp/ui/models/model.hpp index af95e640c..707d4b128 100644 --- a/include/eepp/ui/models/model.hpp +++ b/include/eepp/ui/models/model.hpp @@ -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 ); diff --git a/src/eepp/ui/abstract/uiabstractview.cpp b/src/eepp/ui/abstract/uiabstractview.cpp index bd15c1fba..cceef642e 100644 --- a/src/eepp/ui/abstract/uiabstractview.cpp +++ b/src/eepp/ui/abstract/uiabstractview.cpp @@ -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 { diff --git a/src/eepp/ui/models/filesystemmodel.cpp b/src/eepp/ui/models/filesystemmodel.cpp index 9a6bc5eef..6ad77fcb5 100644 --- a/src/eepp/ui/models/filesystemmodel.cpp +++ b/src/eepp/ui/models/filesystemmodel.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #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 ) : - 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( this ) ); + return model.createIndex( row, column, const_cast( 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( 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( 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( index.internalData() ); + if ( mAliveNodes.find( node ) == mAliveNodes.end() || + node->mId != static_cast( index.internalId() ) ) + return false; + return Model::isValid( index ); +} + size_t FileSystemModel::rowCount( const ModelIndex& index ) const { - Node& node = const_cast( 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( 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 ).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( row ) >= node.mChildren.size() ) + if ( static_cast( 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 newIndexes; view->getSelection().forEachIndex( [&]( const ModelIndex& selectedIndex ) { Node* curNode = static_cast( 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( index.internalData() ); Node* nodeSelected = static_cast( 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( 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( 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; { diff --git a/src/eepp/ui/uifiledialog.cpp b/src/eepp/ui/uifiledialog.cpp index b137dbda5..f814a4061 100644 --- a/src/eepp/ui/uifiledialog.cpp +++ b/src/eepp/ui/uifiledialog.cpp @@ -476,8 +476,8 @@ std::vector UIFileDialog::getSelectionNodes() cons std::vector 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; } diff --git a/src/tests/unit_tests/modeloperations_tests.cpp b/src/tests/unit_tests/modeloperations_tests.cpp index f1d42f172..ae9fa3d9c 100644 --- a/src/tests/unit_tests/modeloperations_tests.cpp +++ b/src/tests/unit_tests/modeloperations_tests.cpp @@ -1,12 +1,24 @@ #include "utest.hpp" +#include +#include #include +#include #include #include +#include +#include +#include +#include #include +#include 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() ); +} diff --git a/src/tools/ecode/filesystemlistener.cpp b/src/tools/ecode/filesystemlistener.cpp index 326f2029d..ef95301d0 100644 --- a/src/tools/ecode/filesystemlistener.cpp +++ b/src/tools/ecode/filesystemlistener.cpp @@ -1,6 +1,10 @@ #include "filesystemlistener.hpp" +#include +#include #include #include +#include +#include namespace ecode { @@ -22,7 +26,28 @@ std::string getFileSystemEventTypeName( FileSystemEventType action ) { FileSystemListener::FileSystemListener( UICodeEditorSplitter* splitter, std::shared_ptr fileSystemModel, const std::vector& 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 nextEventActionTag{ 0 }; + mEventActionTag = + ( static_cast( EE::String::hash( "ecode::FileSystemListener" ) ) << 32 ) | + ( ++nextEventActionTag & 0xFFFFFFFFULL ); + mLifetime = std::make_shared>( 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 ) { diff --git a/src/tools/ecode/filesystemlistener.hpp b/src/tools/ecode/filesystemlistener.hpp index f71be38d4..c8ef8dc24 100644 --- a/src/tools/ecode/filesystemlistener.hpp +++ b/src/tools/ecode/filesystemlistener.hpp @@ -25,7 +25,7 @@ class FileSystemListener : public efsw::FileWatchListener { std::shared_ptr fileSystemModel, const std::vector& 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 mCbs; std::vector 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> mLifetime; bool isFileOpen( const FileInfo& file ); diff --git a/src/tools/ecode/uitreeviewfs.cpp b/src/tools/ecode/uitreeviewfs.cpp index 230ebdd8b..28018e459 100644 --- a/src/tools/ecode/uitreeviewfs.cpp +++ b/src/tools/ecode/uitreeviewfs.cpp @@ -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& paths, const std:: } std::string UITreeViewFS::getSelectionPath() const { - return static_cast( getModel() ) - ->node( getSelection().first() ) - .fullPath(); + const auto* node = + static_cast( 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( index ) >= static_cast( selection.size() ) ) return ""; auto indexVec = selection.indexes(); - return static_cast( getModel() ) - ->node( indexVec[static_cast( index )] ) - .fullPath(); + const auto* node = static_cast( getModel() ) + ->nodePtr( indexVec[static_cast( index )] ); + return node ? node->fullPath() : ""; } std::vector UITreeViewFS::getSelectionsFileInfo() const { @@ -363,7 +365,8 @@ std::vector UITreeViewFS::getSelectionsFileInfo() const { auto indexVec = getSelection().indexes(); auto model = static_cast( 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; }