From fd353843d19fe7c5d6a024e4f03f777706d2c8fa Mon Sep 17 00:00:00 2001 From: Alex Plate Date: Thu, 6 Jul 2023 18:01:14 +0300 Subject: [PATCH] [Workspace Model] [IDEA-324533] Clean up usages of workspace model listener that may lead to control-flow issues If we create a subscription between before and after events, this may cause issues in code logic. Also, such issues will be really hard to debug. I've reviewed the listeners of the workspace model in order to get rid of such problem. See IDEA-324532 for details. GitOrigin-RevId: c5b8c12691fa930d3f5f85af32052e0a22984d73 --- .../backwardRefs/DirtyScopeHolder.java | 36 ++++++++----------- .../repositoryLibrarySynchronizers.kt | 10 ++++++ .../facet/FacetEntityChangeListener.kt | 26 +++++++------- .../src/META-INF/LangExtensions.xml | 2 ++ .../library/ProjectLibraryTableBridgeImpl.kt | 10 ++++++ .../LegacyProjectModelListenersBridge.kt | 12 ++++++- 6 files changed, 59 insertions(+), 37 deletions(-) diff --git a/java/compiler/impl/src/com/intellij/compiler/backwardRefs/DirtyScopeHolder.java b/java/compiler/impl/src/com/intellij/compiler/backwardRefs/DirtyScopeHolder.java index bed14d2242e4..98a93568aae3 100644 --- a/java/compiler/impl/src/com/intellij/compiler/backwardRefs/DirtyScopeHolder.java +++ b/java/compiler/impl/src/com/intellij/compiler/backwardRefs/DirtyScopeHolder.java @@ -25,6 +25,12 @@ import com.intellij.openapi.vfs.AsyncFileListener; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.openapi.vfs.VirtualFileManager; import com.intellij.openapi.vfs.newvfs.events.*; +import com.intellij.platform.backend.workspace.WorkspaceModelChangeListener; +import com.intellij.platform.backend.workspace.WorkspaceModelTopics; +import com.intellij.platform.workspace.jps.entities.ContentRootEntity; +import com.intellij.platform.workspace.jps.entities.ModuleEntity; +import com.intellij.platform.workspace.storage.EntityChange; +import com.intellij.platform.workspace.storage.VersionedStorageChange; import com.intellij.psi.PsiDocumentManager; import com.intellij.psi.PsiFile; import com.intellij.psi.search.GlobalSearchScope; @@ -34,13 +40,7 @@ import com.intellij.psi.util.PsiModificationTracker; import com.intellij.util.SmartList; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.messages.MessageBusConnection; -import com.intellij.platform.backend.workspace.WorkspaceModelChangeListener; -import com.intellij.platform.backend.workspace.WorkspaceModelTopics; import com.intellij.workspaceModel.ide.impl.legacyBridge.module.ModuleEntityUtils; -import com.intellij.platform.workspace.storage.EntityChange; -import com.intellij.platform.workspace.storage.VersionedStorageChange; -import com.intellij.platform.workspace.jps.entities.ContentRootEntity; -import com.intellij.platform.workspace.jps.entities.ModuleEntity; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -96,22 +96,6 @@ public final class DirtyScopeHolder extends UserDataHolderBase implements AsyncF compilationAffectedModulesSubscription.accept(connect, myCompilationAffectedModules); connect.subscribe(WorkspaceModelTopics.CHANGED, new WorkspaceModelChangeListener() { - @Override - public void beforeChanged(@NotNull VersionedStorageChange event) { - for (EntityChange change : event.getChanges(ModuleEntity.class)) { - ModuleEntity oldEntity = change.getOldEntity(); - if (oldEntity != null) { - addToDirtyModules(ModuleEntityUtils.findModule(oldEntity, event.getStorageBefore())); - } - } - for (EntityChange change : event.getChanges(ContentRootEntity.class)) { - ContentRootEntity oldEntity = change.getOldEntity(); - if (oldEntity != null) { - addToDirtyModules(ModuleEntityUtils.findModule(oldEntity.getModule(), event.getStorageBefore())); - } - } - } - @Override public void changed(@NotNull VersionedStorageChange event) { for (EntityChange change : event.getChanges(ModuleEntity.class)) { @@ -119,12 +103,20 @@ public final class DirtyScopeHolder extends UserDataHolderBase implements AsyncF if (newEntity != null) { addToDirtyModules(ModuleEntityUtils.findModule(newEntity, event.getStorageAfter())); } + ModuleEntity oldEntity = change.getOldEntity(); + if (oldEntity != null) { + addToDirtyModules(ModuleEntityUtils.findModule(oldEntity, event.getStorageBefore())); + } } for (EntityChange change : event.getChanges(ContentRootEntity.class)) { ContentRootEntity newEntity = change.getNewEntity(); if (newEntity != null) { addToDirtyModules(ModuleEntityUtils.findModule(newEntity.getModule(), event.getStorageAfter())); } + ContentRootEntity oldEntity = change.getOldEntity(); + if (oldEntity != null) { + addToDirtyModules(ModuleEntityUtils.findModule(oldEntity.getModule(), event.getStorageBefore())); + } } clearDisposedModules(); } diff --git a/java/idea-ui/src/com/intellij/jarRepository/repositoryLibrarySynchronizers.kt b/java/idea-ui/src/com/intellij/jarRepository/repositoryLibrarySynchronizers.kt index 50ac9c2fd46e..e75fecffbe3c 100644 --- a/java/idea-ui/src/com/intellij/jarRepository/repositoryLibrarySynchronizers.kt +++ b/java/idea-ui/src/com/intellij/jarRepository/repositoryLibrarySynchronizers.kt @@ -58,7 +58,15 @@ internal class GlobalChangedRepositoryLibrarySynchronizer(private val queue: Lib internal class ChangedRepositoryLibrarySynchronizer(private val project: Project, private val queue: LibrarySynchronizationQueue) : WorkspaceModelChangeListener { + /** + * This is a flag indicating that the [beforeChanged] method was called. Due to the fact that we subscribe using the code, this + * may lead to IDEA-324532. + * With this flag we skip the "after" event if the before event wasn't called. + */ + private var beforeCalled = false + override fun beforeChanged(event: VersionedStorageChange) { + beforeCalled = true for (change in event.getChanges(LibraryEntity::class.java)) { val removed = change as? EntityChange.Removed ?: continue val library = findLibrary(removed.entity.symbolicId, event.storageBefore) @@ -82,6 +90,8 @@ internal class ChangedRepositoryLibrarySynchronizer(private val project: Project } override fun changed(event: VersionedStorageChange) { + if (!beforeCalled) return + beforeCalled = false var libraryReloadRequested = false for (change in event.getChanges(LibraryEntity::class.java)) { diff --git a/platform/lang-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/facet/FacetEntityChangeListener.kt b/platform/lang-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/facet/FacetEntityChangeListener.kt index 7ffa4acae954..3d844721679f 100644 --- a/platform/lang-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/facet/FacetEntityChangeListener.kt +++ b/platform/lang-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/facet/FacetEntityChangeListener.kt @@ -68,21 +68,19 @@ internal class FacetEntityChangeListener(private val project: Project, coroutine initializeFacetBridgeTimeMs.addElapsedTimeMs(start) } - init { - if (!project.isDefault) { - project.messageBus.connect(coroutineScope).subscribe(WorkspaceModelTopics.CHANGED, object : WorkspaceModelChangeListener { - override fun beforeChanged(event: VersionedStorageChange) { - WorkspaceFacetContributor.EP_NAME.extensions.forEach { facetBridgeContributor -> - processBeforeChangeEvents(event, facetBridgeContributor) - } - } + class WorkspaceModelListener(project: Project) : WorkspaceModelChangeListener { + private val facetEntityChangeListener = getInstance(project) - override fun changed(event: VersionedStorageChange) { - WorkspaceFacetContributor.EP_NAME.extensions.forEach { facetBridgeContributor -> - processChangeEvents(event, facetBridgeContributor) - } - } - }) + override fun beforeChanged(event: VersionedStorageChange) { + WorkspaceFacetContributor.EP_NAME.extensions.forEach { facetBridgeContributor -> + facetEntityChangeListener.processBeforeChangeEvents(event, facetBridgeContributor) + } + } + + override fun changed(event: VersionedStorageChange) { + WorkspaceFacetContributor.EP_NAME.extensions.forEach { facetBridgeContributor -> + facetEntityChangeListener.processChangeEvents(event, facetBridgeContributor) + } } } diff --git a/platform/platform-resources/src/META-INF/LangExtensions.xml b/platform/platform-resources/src/META-INF/LangExtensions.xml index 828c13162af1..58d9e87e271a 100644 --- a/platform/platform-resources/src/META-INF/LangExtensions.xml +++ b/platform/platform-resources/src/META-INF/LangExtensions.xml @@ -1749,5 +1749,7 @@ topic="com.intellij.codeInsight.lookup.LookupManagerListener"/> + diff --git a/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/library/ProjectLibraryTableBridgeImpl.kt b/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/library/ProjectLibraryTableBridgeImpl.kt index 6157b09df094..77881764d764 100644 --- a/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/library/ProjectLibraryTableBridgeImpl.kt +++ b/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/library/ProjectLibraryTableBridgeImpl.kt @@ -68,7 +68,15 @@ class ProjectLibraryTableBridgeImpl( init { project.messageBus.connect(this).subscribe(WorkspaceModelTopics.CHANGED, object : WorkspaceModelChangeListener { + /** + * This is a flag indicating that the [beforeChanged] method was called. Due to the fact that we subscribe using the code, this + * may lead to IDEA-324532. + * With this flag we skip the "after" event if the before event wasn't called. + */ + private var beforeCalled = false + override fun beforeChanged(event: VersionedStorageChange) { + beforeCalled = true val libraryChanges = event.getChanges(LibraryEntity::class.java) val removeChanges = libraryChanges.filterProjectLibraryChanges().filterIsInstance>() if (removeChanges.isEmpty()) return @@ -83,6 +91,8 @@ class ProjectLibraryTableBridgeImpl( } override fun changed(event: VersionedStorageChange) { + if (!beforeCalled) return + beforeCalled = false val changes = event.getChanges(LibraryEntity::class.java).filterProjectLibraryChanges() .orderToRemoveReplaceAdd() // Since the listener is not deprecated, it will be better to keep the order of events as remove -> replace -> add if (changes.isEmpty()) return diff --git a/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/module/LegacyProjectModelListenersBridge.kt b/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/module/LegacyProjectModelListenersBridge.kt index 6635144ebc40..5677a9c93ed5 100644 --- a/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/module/LegacyProjectModelListenersBridge.kt +++ b/platform/projectModel-impl/src/com/intellij/workspaceModel/ide/impl/legacyBridge/module/LegacyProjectModelListenersBridge.kt @@ -33,9 +33,17 @@ internal class LegacyProjectModelListenersBridge( private val moduleModificationTracker: SimpleModificationTracker, private val moduleRootListenerBridge: ModuleRootListenerBridge ) : WorkspaceModelChangeListener { - + + /** + * This is a flag indicating that the [beforeChanged] method was called. Due to the fact that we subscribe using the code, this + * may lead to IDEA-324532. + * With this flag we skip the "after" event if the before event wasn't called. + */ + private var beforeCalled = false + override fun beforeChanged(event: VersionedStorageChange) { LOG.trace { "Get before changed event" } + beforeCalled = true moduleRootListenerBridge.fireBeforeRootsChanged(project, event) val moduleMap = event.storageBefore.moduleMap for (change in event.getChanges(ModuleEntity::class.java)) { @@ -50,6 +58,8 @@ internal class LegacyProjectModelListenersBridge( } override fun changed(event: VersionedStorageChange) { + if (!beforeCalled) return + beforeCalled = false LOG.trace { "Get changed event" } val moduleLibraryChanges = event.getChanges(LibraryEntity::class.java).filterModuleLibraryChanges() val changes = event.getChanges(ModuleEntity::class.java)