From 626eac13865a9691c4f079f510caeb2d6709a999 Mon Sep 17 00:00:00 2001 From: nakst <> Date: Mon, 15 Nov 2021 21:05:48 +0000 Subject: [PATCH] fix file manager threaded instance destroy bug --- apps/file_manager/commands.cpp | 10 +++++--- apps/file_manager/folder.cpp | 10 ++++++++ apps/file_manager/main.cpp | 23 ++++++++++++++++-- apps/file_manager/ui.cpp | 6 ++++- desktop/api.cpp | 44 +++++++++++++++++++++------------- desktop/desktop.cpp | 2 +- desktop/os.header | 12 +++++++--- util/api_table.ini | 2 +- 8 files changed, 81 insertions(+), 28 deletions(-) diff --git a/apps/file_manager/commands.cpp b/apps/file_manager/commands.cpp index 9ec074b..6a659d7 100644 --- a/apps/file_manager/commands.cpp +++ b/apps/file_manager/commands.cpp @@ -56,7 +56,9 @@ void CommandRename(Instance *instance, EsElement *, EsCommand *) { }, .then = [] (Instance *instance, Task *task) { - if (task->result != ES_SUCCESS) { + if (instance->closed) { + // Ignore. + } else if (task->result != ES_SUCCESS) { InstanceReportError(instance, ERROR_RENAME_ITEM, task->result); } else { Folder *folder = instance->folder; @@ -105,7 +107,9 @@ void CommandNewFolder(Instance *instance, EsElement *, EsCommand *) { }, .then = [] (Instance *instance, Task *task) { - if (task->result != ES_SUCCESS) { + if (instance->closed) { + // Ignore. + } else if (task->result != ES_SUCCESS) { InstanceReportError(instance, ERROR_NEW_FOLDER, task->result); } else { Folder *folder = instance->folder; @@ -405,7 +409,7 @@ void CommandPasteTask(EsUserTask *userTask, EsGeneric _task) { for (uintptr_t i = 0; i < instances.Length(); i++) { Instance *instance = instances[i]; - if (instance->issuedPasteTask == task) { + if (!instance->closed && instance->issuedPasteTask == task) { instance->issuedPasteTask = nullptr; if (error != ES_SUCCESS) { diff --git a/apps/file_manager/folder.cpp b/apps/file_manager/folder.cpp index e19ec24..b993de9 100644 --- a/apps/file_manager/folder.cpp +++ b/apps/file_manager/folder.cpp @@ -77,6 +77,9 @@ EsError FSDirEnumerate(Folder *folder) { EsDirectoryChild *buffer = nullptr; ptrdiff_t _entryCount = EsDirectoryEnumerateChildren(STRING(folder->path), &buffer); + // TODO. + EsSleep(5000); + if (!ES_CHECK_ERROR(_entryCount)) { for (intptr_t i = 0; i < _entryCount; i++) { FolderAddEntry(folder, buffer[i].name, buffer[i].nameBytes, &buffer[i]); @@ -301,6 +304,7 @@ void BookmarkAdd(String path, bool saveConfiguration = true) { if (saveConfiguration) ConfigurationSave(); for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewInsert(instances[i]->placesView, PLACES_VIEW_GROUP_BOOKMARKS, bookmarks.Length(), 1); } } @@ -311,6 +315,7 @@ void BookmarkRemove(String path) { bookmarks.Delete(i); for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewRemove(instances[i]->placesView, PLACES_VIEW_GROUP_BOOKMARKS, i + 1, 1); } @@ -433,6 +438,7 @@ void FolderAddEntryAndUpdateInstances(Folder *folder, const char *name, size_t n for (uintptr_t i = 0; i < folder->attachedInstances.Length(); i++) { Instance *instance = folder->attachedInstances[i]; + if (instance->closed) continue; listEntry.selected = instance == selectItem; InstanceAddSingle(instance, listEntry); InstanceUpdateStatusString(instance); @@ -445,6 +451,7 @@ uint64_t FolderRemoveEntryAndUpdateInstances(Folder *folder, const char *name, s if (entry) { for (uintptr_t i = 0; i < folder->attachedInstances.Length(); i++) { + if (folder->attachedInstances[i]->closed) continue; InstanceRemoveSingle(folder->attachedInstances[i], entry); } @@ -489,6 +496,7 @@ void FolderPathMoved(String oldPath, String newPath, bool saveConfiguration) { if (PathReplacePrefix(&folder->path, oldPath, newPath)) { for (uintptr_t i = 0; i < folder->attachedInstances.Length(); i++) { + if (folder->attachedInstances[i]->closed) continue; InstanceFolderPathChanged(folder->attachedInstances[i], false); } } @@ -503,6 +511,7 @@ void FolderPathMoved(String oldPath, String newPath, bool saveConfiguration) { } for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewInvalidateAll(instances[i]->placesView); for (uintptr_t j = 0; j < instances[i]->pathBackwardHistory.Length(); j++) { @@ -581,6 +590,7 @@ void FolderRefresh(Folder *folder) { Array instancesToRefresh = {}; for (uintptr_t i = 0; i < folder->attachedInstances.Length(); i++) { + if (folder->attachedInstances[i]->closed) continue; instancesToRefresh.Add(folder->attachedInstances[i]); } diff --git a/apps/file_manager/main.cpp b/apps/file_manager/main.cpp index 792d338..834b237 100644 --- a/apps/file_manager/main.cpp +++ b/apps/file_manager/main.cpp @@ -179,6 +179,8 @@ struct Instance : EsInstance { Task blockingTask; volatile bool blockingTaskInProgress, blockingTaskReachedTimeout; volatile uint32_t blockingTaskID; + + bool closed; // The window has been destroyed (or is about to be). }; struct NamespaceHandler { @@ -205,7 +207,7 @@ struct Folder { HashTable entries; EsArena entryArena; - Array attachedInstances; + Array attachedInstances; // NOTE Check Instance::closed is false before accessing the UI! uintptr_t referenceCount; String path; @@ -245,7 +247,7 @@ void ThumbnailGenerateIfNeeded(Folder *folder, FolderEntry *entry, bool fromFold Array drives; EsMutex drivesMutex; -Array instances; +Array instances; // NOTE Check Instance::closed is false before accessing the UI! Array bookmarks; #define FOLDER_VIEW_SETTINGS_MAXIMUM_ENTRIES (10000) @@ -330,11 +332,15 @@ void BlockingTaskComplete(Instance *instance) { if (task->then) { task->then(instance, task); } + + EsInstanceCloseReference(instance); } void BlockingTaskQueue(Instance *instance, Task task) { EsAssert(!instance->blockingTaskInProgress); // Cannot queue a new blocking task if the previous has not finished. + EsInstanceOpenReference(instance); // Don't destroy the instance while the task is in progress. + instance->blockingTask = task; instance->blockingTaskInProgress = true; @@ -434,6 +440,7 @@ void DriveRemove(const char *prefix, size_t prefixBytes) { found = true; for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewRemove(instances[i]->placesView, PLACES_VIEW_GROUP_DRIVES, index, 1); } @@ -459,6 +466,7 @@ void DriveAdd(const char *prefix, size_t prefixBytes) { drives.Add(drive); for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewInsert(instances[i]->placesView, PLACES_VIEW_GROUP_DRIVES, drives.Length(), 1); } @@ -520,6 +528,16 @@ void _start() { instances.Add(instance); InstanceCreateUI(instance); } + } else if (message->type == ES_MSG_INSTANCE_CLOSE) { + Instance *instance = message->instanceClose.instance; + EsAssert(!instance->closed); + instance->closed = true; + instance->list = nullptr; + instance->placesView = nullptr; + instance->breadcrumbBar = nullptr; + instance->newFolderButton = nullptr; + instance->status = nullptr; + instance->blockingDialog = nullptr; } else if (message->type == ES_MSG_INSTANCE_DESTROY) { Instance *instance = message->instanceDestroy.instance; EsApplicationStartupRequest request = EsInstanceGetStartupRequest(instance); @@ -628,6 +646,7 @@ void _start() { } for (uintptr_t i = 0; i < instances.Length(); i++) { + if (instances[i]->closed) continue; EsListViewInvalidateAll(instances[i]->list); } diff --git a/apps/file_manager/ui.cpp b/apps/file_manager/ui.cpp index 3bb5440..536ec97 100644 --- a/apps/file_manager/ui.cpp +++ b/apps/file_manager/ui.cpp @@ -72,6 +72,10 @@ bool InstanceLoadFolder(Instance *instance, String path /* takes ownership */, i task.then = [] (Instance *instance, Task *task) { // TODO Check if folder was marked for refresh. + if (instance->closed) { + return; + } + int historyMode = task->context.i & 0xFF; int flags = task->context.i; Folder *folder = instance->folder; @@ -627,7 +631,7 @@ void ThumbnailGenerateTaskComplete(Instance *, Task *task) { String parent = PathGetParent(task->string); for (uintptr_t i = 0; i < instances.Length(); i++) { - if (StringEquals(parent, instances[i]->path)) { // TODO Support recursive views. + if (!instances[i]->closed && StringEquals(parent, instances[i]->path)) { // TODO Support recursive views. EsListViewInvalidateAll(instances[i]->list); } } diff --git a/desktop/api.cpp b/desktop/api.cpp index 284d0c8..6187f83 100644 --- a/desktop/api.cpp +++ b/desktop/api.cpp @@ -670,7 +670,7 @@ void InstanceSave(EsInstance *_instance) { void InstanceClose(EsInstance *instance) { if (!EsCommandByID(instance, ES_COMMAND_SAVE)->enabled) { - EsInstanceDestroy(instance); + EsInstanceClose(instance); return; } @@ -717,7 +717,7 @@ void InstanceClose(EsInstance *instance) { EsDialogAddButton(dialog, ES_FLAGS_DEFAULT, ES_STYLE_PUSH_BUTTON_DANGEROUS, INTERFACE_STRING(FileCloseWithModificationsDelete), [] (EsInstance *instance, EsElement *, EsCommand *) { EsDialogClose(instance->window->dialogs.Last()); - EsInstanceDestroy(instance); + EsInstanceClose(instance); }); EsButton *button = EsDialogAddButton(dialog, ES_BUTTON_DEFAULT, 0, INTERFACE_STRING(FileCloseWithModificationsSave), @@ -936,20 +936,12 @@ void EsInstanceCloseReference(EsInstance *_instance) { } } -void EsInstanceDestroy(EsInstance *instance) { +void EsInstanceClose(EsInstance *instance) { EsMessageMutexCheck(); - InspectorWindow **inspector = &((APIInstance *) instance->_private)->attachedInspector; - - if (*inspector) { - EsInstanceDestroy(*inspector); - (*inspector)->window->InternalDestroy(); - *inspector = nullptr; - } - - UndoManagerDestroy(instance->undoManager); - EsAssert(instance->window->instance == instance); - EsElementDestroy(instance->window); - EsInstanceCloseReference(instance); + EsMessage m = {}; + m.type = ES_MSG_INSTANCE_CLOSE; + m.instanceClose.instance = instance; + EsMessagePost(nullptr, &m); } EsWindow *WindowFromWindowID(EsObjectID id) { @@ -1063,6 +1055,24 @@ EsMessage *EsMessageReceive() { if (instance->fileStore) FileStoreCloseHandle(instance->fileStore); EsHeapFree(instance); EsHeapFree(message.message.instanceDestroy.instance); + } else if (message.message.type == ES_MSG_INSTANCE_CLOSE) { + EsInstance *instance = message.message.instanceClose.instance; + InspectorWindow **inspector = &((APIInstance *) instance->_private)->attachedInspector; + + if (*inspector) { + EsInstance *instance2 = *inspector; + UndoManagerDestroy(instance2->undoManager); + EsAssert(instance2->window->instance == instance2); + EsElementDestroy(instance2->window); + EsInstanceCloseReference(instance2); + instance2->window->InternalDestroy(); + *inspector = nullptr; + } + + UndoManagerDestroy(instance->undoManager); + EsAssert(instance->window->instance == instance); + EsElementDestroy(instance->window); + EsInstanceCloseReference(instance); } else if (message.message.type == ES_MSG_UNREGISTER_FILE_SYSTEM) { for (uintptr_t i = 0; i < api.mountPoints.Length(); i++) { if (api.mountPoints[i].information.id == message.message.unregisterFileSystem.id) { @@ -1312,7 +1322,7 @@ EsMessage *EsMessageReceive() { return &message.message; } } - } else if (type == ES_MSG_INSTANCE_DESTROY) { + } else if (type == ES_MSG_INSTANCE_DESTROY || type == ES_MSG_INSTANCE_CLOSE) { APIInstance *instance = (APIInstance *) message.message.instanceDestroy.instance->_private; if (!instance->internalOnly) { @@ -1404,7 +1414,7 @@ void EsInstanceSaveComplete(EsMessage *message, bool success) { EsCommandSetDisabled(&apiInstance->commandShowInFileManager, false); if (apiInstance->closeAfterSaveCompletes) { - EsInstanceDestroy(instance); + EsInstanceClose(instance); } } else { apiInstance->closeAfterSaveCompletes = false; diff --git a/desktop/desktop.cpp b/desktop/desktop.cpp index 648a3c9..d8d6e80 100644 --- a/desktop/desktop.cpp +++ b/desktop/desktop.cpp @@ -1524,7 +1524,7 @@ void InstanceBlankTabCreate(EsMessage *message) { if (ApplicationInstanceStart(((InstalledApplication *) element->userData.p)->id, nullptr, instance)) { WindowTabActivate(instance->tab, true); - EsInstanceDestroy(element->instance); + EsInstanceClose(element->instance); } }); } diff --git a/desktop/os.header b/desktop/os.header index 6be18ab..aec967f 100644 --- a/desktop/os.header +++ b/desktop/os.header @@ -1040,6 +1040,7 @@ enum EsMessageType { ES_MSG_INSTANCE_OPEN = 0x7003 ES_MSG_INSTANCE_SAVE = 0x7004 ES_MSG_INSTANCE_DESTROY = 0x7005 + ES_MSG_INSTANCE_CLOSE = 0x7006 // User messages: ES_MSG_USER_START = 0x8000 @@ -1744,6 +1745,10 @@ struct EsMessageInstanceDestroy { ES_INSTANCE_TYPE *instance; }; +struct EsMessageInstanceClose { + ES_INSTANCE_TYPE *instance; +}; + // Internal system messages. struct EsMessageProcessCrash { @@ -1856,6 +1861,7 @@ struct EsMessage { EsMessageInstanceOpen instanceOpen; EsMessageInstanceSave instanceSave; EsMessageInstanceDestroy instanceDestroy; + EsMessageInstanceClose instanceClose; // Internal messages: void *_argument; @@ -2366,9 +2372,9 @@ function void EsCommandSetCallback(EsCommand *command, EsCommandCallback callbac function void EsCommandSetDisabled(EsCommand *command, bool disabled); function void EsCommandSetCheck(EsCommand *command, EsCheckState check, bool sendUpdatedMessage); -function void EsInstanceOpenReference(EsInstance *_instance); -function void EsInstanceCloseReference(EsInstance *_instance); -function void EsInstanceDestroy(ES_INSTANCE_TYPE *instance); +function void EsInstanceOpenReference(ES_INSTANCE_TYPE *_instance); +function void EsInstanceCloseReference(ES_INSTANCE_TYPE *_instance); // Sends ES_MSG_INSTANCE_DESTROY when all references closed. +function void EsInstanceClose(ES_INSTANCE_TYPE *instance); // Sends ES_MSG_INSTANCE_CLOSE, and closes the window's reference to the instance. function void EsInstanceSetActiveUndoManager(ES_INSTANCE_TYPE *instance, EsUndoManager *manager); function void EsInstanceSetClassEditor(ES_INSTANCE_TYPE *instance, const EsInstanceClassEditorSettings *settings); function void EsInstanceSetClassViewer(ES_INSTANCE_TYPE *instance, const EsInstanceClassViewerSettings *settings); diff --git a/util/api_table.ini b/util/api_table.ini index f8e16f3..b15092f 100644 --- a/util/api_table.ini +++ b/util/api_table.ini @@ -136,6 +136,7 @@ EsBufferFormat=134 EsEventReset=135 EsEventSet=136 EsInstanceCloseReference=137 +EsInstanceClose=138 EsMutexAcquire=140 EsMutexDestroy=141 EsMutexRelease=142 @@ -296,7 +297,6 @@ EsSliderCreate=296 EsSystemConfigurationReadString=297 EsSliderGetValue=298 EsGameControllerStatePoll=299 -EsInstanceDestroy=300 EsRandomU8=301 EsRandomU64=302 EsConnectionPoll=303