Skip to content

Commit 15bb0e9

Browse files
Daniel-ADFADaniel-ADFA
authored
Merge branch 'stage' into ADFA-3597-layout-editor
2 parents b26d853 + bf89f5b commit 15bb0e9

8 files changed

Lines changed: 263 additions & 34 deletions

File tree

‎app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/*
2+
* This file is part of AndroidIDE.
3+
*
4+
* AndroidIDE is free software: you can redistribute it and/or modify
5+
* it under the terms of the GNU General Public License as published by
6+
* the Free Software Foundation, either version 3 of the License, or
7+
* (at your option) any later version.
8+
*
9+
* AndroidIDE is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
* GNU General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU General Public License
15+
* along with AndroidIDE. If not, see <https://www.gnu.org/licenses/>.
16+
*/
17+
18+
package com.itsaky.androidide.ui
19+
20+
import android.content.Context
21+
import android.graphics.Canvas
22+
import android.util.AttributeSet
23+
import com.github.mikephil.charting.charts.LineChart
24+
import org.slf4j.LoggerFactory
25+
26+
/**
27+
* A [LineChart] that guards its [onDraw] against the MPAndroidChart axis-rendering race.
28+
*
29+
* MPAndroidChart is not thread-safe: [com.github.mikephil.charting.renderer.AxisRenderer.computeAxisValues]
30+
* writes `mEntryCount` and reallocates the `mEntries` array in two separate statements. When the view is
31+
* drawn from more than one thread at once, a reader can observe the new `mEntryCount` while `mEntries` is
32+
* still the old (shorter) array, throwing an [IndexOutOfBoundsException] from the label renderer.
33+
*
34+
* This happens in AndroidIDE because Sentry Session Replay records the screen by drawing the view
35+
* hierarchy on a background thread, which races the main-thread updates of the memory-usage chart. The
36+
* chart is a non-critical diagnostic view, so dropping the occasional frame is preferable to crashing the
37+
* whole IDE. The next `invalidate()` recovers cleanly.
38+
*
39+
*/
40+
class SafeLineChart : LineChart {
41+
42+
constructor(context: Context) : super(context)
43+
constructor(context: Context, attrs: AttributeSet?) : super(context, attrs)
44+
constructor(
45+
context: Context,
46+
attrs: AttributeSet?,
47+
defStyleAttr: Int,
48+
) : super(context, attrs, defStyleAttr)
49+
50+
companion object {
51+
private val log = LoggerFactory.getLogger(SafeLineChart::class.java)
52+
}
53+
54+
private var skippedFrames = 0L
55+
56+
override fun onDraw(canvas: Canvas) {
57+
try {
58+
super.onDraw(canvas)
59+
} catch (e: IndexOutOfBoundsException) {
60+
// Transient race in MPAndroidChart's axis renderer (see class doc). Skip this frame.
61+
// Only log occasionally to avoid flooding logcat, since onDraw runs every frame.
62+
if (skippedFrames++ % 60L == 0L) {
63+
log.warn("Skipped {} chart frame(s) due to a transient axis-rendering race", skippedFrames, e)
64+
}
65+
}
66+
}
67+
}

‎app/src/main/res/layout/layout_mem_usage.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
android:layout_height="@dimen/editor_mem_usage_view_height">
2222

2323
<!-- Need to wrap this in a layout so that we can apply margins at runtime-->
24-
<com.github.mikephil.charting.charts.LineChart
24+
<com.itsaky.androidide.ui.SafeLineChart
2525
android:id="@+id/chart"
2626
android:layout_width="0dp"
2727
android:layout_height="0dp"

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import com.itsaky.androidide.lsp.models.DocumentChange
1818
import com.itsaky.androidide.lsp.models.TextEdit
1919
import com.itsaky.androidide.resources.R
2020
import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol
21-
import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic
2221
import org.slf4j.LoggerFactory
2322

2423
class AddImportAction : BaseKotlinCodeAction() {
@@ -46,14 +45,13 @@ class AddImportAction : BaseKotlinCodeAction() {
4645
return
4746
}
4847

49-
val diagnostic = extra.diagnostic as? KaFirDiagnostic.UnresolvedReference?
50-
if (diagnostic == null) {
48+
val reference = extra.unresolvedReference
49+
if (reference == null) {
5150
markInvisible()
5251
return
5352
}
5453

5554
val env = extra.compilationEnv
56-
val reference = diagnostic.reference
5755
val hasImportableSymbols = env.ktSymbolIndex
5856
.findSymbolBySimpleName(reference, limit = 0)
5957
.any { it.kind.isClassifier }
@@ -65,10 +63,10 @@ class AddImportAction : BaseKotlinCodeAction() {
6563
}
6664

6765
override suspend fun execAction(data: ActionData): Map<JvmSymbol, List<TextEdit>> {
68-
val (diagnostic, env) = data.require<DiagnosticItem>().extra as? KotlinDiagnosticExtra
66+
val (reference, env) = data.require<DiagnosticItem>().extra as? KotlinDiagnosticExtra
6967
?: return emptyMap()
7068

71-
diagnostic as KaFirDiagnostic.UnresolvedReference
69+
if (reference == null) return emptyMap()
7270

7371
val file = data.requireFile()
7472
val nioPath = file.toPath()
@@ -77,7 +75,7 @@ class AddImportAction : BaseKotlinCodeAction() {
7775
?: return emptyMap()
7876

7977
return env.ktSymbolIndex
80-
.findSymbolBySimpleName(diagnostic.reference, limit = 0)
78+
.findSymbolBySimpleName(reference, limit = 0)
8179
.filter { it.kind.isClassifier }
8280
.associateWith { symbol -> insertImport(ktFile, symbol.fqName) }
8381
}

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -221,22 +221,31 @@ internal class CompilationEnvironment(
221221
@OptIn(KaImplementationDetail::class)
222222
private inline fun notifyElementModifiedForPath(
223223
path: Path,
224-
typeProvider: (KtFile) -> KaElementModificationType,
224+
crossinline typeProvider: (KtFile) -> KaElementModificationType,
225225
) {
226-
val structureProvider = ProjectStructureProvider.getInstance(project)
227-
val ktFile = path.toVirtualFileOrNull()?.let {
228-
psiManager.findFile(it) as? KtFile
229-
}
226+
// Resolve PSI/module structure under the read lock, mirroring loadKtFile(); driving
227+
// psiManager.findFile / structureProvider concurrently with an `analyze` read section
228+
// otherwise races.
229+
val (ktFile, module) = project.read {
230+
val structureProvider = ProjectStructureProvider.getInstance(project)
231+
val ktFile = path.toVirtualFileOrNull()?.let {
232+
psiManager.findFile(it) as? KtFile
233+
}
230234

231-
if (ktFile != null) {
232-
KaSourceModificationService.getInstance(project)
233-
.handleElementModification(ktFile, typeProvider(ktFile))
234-
}
235+
val module = (ktFile?.let { structureProvider.getModule(it, null) }
236+
?: structureProvider.findModuleForSourceId(path.pathString)) as? AbstractKtModule
235237

236-
val module = (ktFile?.let { structureProvider.getModule(it, null) }
237-
?: structureProvider.findModuleForSourceId(path.pathString)) as? AbstractKtModule
238+
ktFile to module
239+
}
238240

239241
project.write {
242+
// Must run under the write lock so the session mutation can't race a concurrent
243+
// `analyze` (which only holds the read lock); see onFileContentChanged.
244+
if (ktFile != null) {
245+
KaSourceModificationService.getInstance(project)
246+
.handleElementModification(ktFile, typeProvider(ktFile))
247+
}
248+
240249
if (module != null) {
241250
module.invalidateSearchScope()
242251
project.publishModificationEvent(

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,46 @@ import org.jetbrains.kotlin.psi.KtElement
1111
import org.jetbrains.kotlin.psi.KtFile
1212
import org.jetbrains.kotlin.psi.UserDataProperty
1313
import java.nio.file.Path
14+
import java.util.concurrent.locks.ReentrantLock
15+
import kotlin.concurrent.withLock
1416

1517
private val KT_LSP_COMPLETION_BACKING_FILE = Key<Path>("KT_LSP_COMPLETION_BACKING_FILE")
1618
var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE)
1719

18-
internal inline fun <R> analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R {
19-
if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) {
20-
return analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action)
21-
}
20+
/**
21+
* Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`).
22+
*
23+
* The Analysis API tracks its `analyze` lifetime context in a per-thread stack and is not safe to
24+
* drive concurrently from multiple background threads without the platform read-action coordination
25+
* that this LSP replaces with a custom [com.itsaky.androidide.lsp.kotlin.compiler.read] lock.
26+
* Indexing, diagnostics and completion all run analysis on `Dispatchers.Default` and frequently
27+
* target the same edited file, so overlapping `analyze` calls corrupted the lifetime/session
28+
* lifecycle and surfaced as
29+
* `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.`
30+
*
31+
* Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a
32+
* [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock.
33+
*
34+
* **Footgun:** analysis runs under the *read* (shared) side of the global
35+
* [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is
36+
* non-upgradeable. Code running inside [withAnalysisLock] / an `analyze` block must therefore never
37+
* call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread
38+
* deadlocks.
39+
*/
40+
private val analysisLock = ReentrantLock()
41+
42+
/**
43+
* Runs [action] while holding the shared [analysisLock]. **All** Analysis API access must go through
44+
* this helper (or [analyzeMaybeDangling], which already does); never call `analyze` / `analyzeCopy`
45+
* directly, or the serialization guarantee is lost.
46+
*/
47+
internal inline fun <R> withAnalysisLock(action: () -> R): R = analysisLock.withLock(action)
2248

23-
return analyze(useSiteElement, action)
24-
}
49+
internal inline fun <R> analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R =
50+
withAnalysisLock {
51+
if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) {
52+
analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action)
53+
} else {
54+
analyze(useSiteElement, action)
55+
}
56+
}

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.completion
33
import com.itsaky.androidide.lookup.Lookup
44
import com.itsaky.androidide.lsp.api.describeSnippet
55
import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment
6+
import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling
67
import com.itsaky.androidide.lsp.kotlin.compiler.read
78
import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext
89
import com.itsaky.androidide.lsp.kotlin.utils.ContextKeywords
@@ -30,8 +31,6 @@ import org.jetbrains.kotlin.analysis.api.KaContextParameterApi
3031
import org.jetbrains.kotlin.analysis.api.KaExperimentalApi
3132
import org.jetbrains.kotlin.analysis.api.KaIdeApi
3233
import org.jetbrains.kotlin.analysis.api.KaSession
33-
import org.jetbrains.kotlin.analysis.api.analyzeCopy
34-
import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode
3534
import org.jetbrains.kotlin.analysis.api.renderer.types.KaTypeRenderer
3635
import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource
3736
import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol
@@ -149,10 +148,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult {
149148
env.project.read {
150149
abortIfCancelled()
151150

152-
analyzeCopy(
153-
useSiteElement = completionKtFile,
154-
resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF,
155-
) {
151+
analyzeMaybeDangling(completionKtFile) {
156152
val ctx =
157153
resolveAnalysisContext(
158154
env = env,
@@ -168,7 +164,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult {
168164
completionOffset,
169165
params.file
170166
)
171-
return@analyzeCopy CompletionResult.EMPTY
167+
return@analyzeMaybeDangling CompletionResult.EMPTY
172168
}
173169

174170
abortIfCancelled()

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import org.jetbrains.kotlin.analysis.api.KaExperimentalApi
1313
import org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter
1414
import org.jetbrains.kotlin.analysis.api.diagnostics.KaDiagnosticWithPsi
1515
import org.jetbrains.kotlin.analysis.api.diagnostics.KaSeverity
16+
import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic
1617
import org.jetbrains.kotlin.com.intellij.openapi.util.TextRange
1718
import org.jetbrains.kotlin.com.intellij.psi.PsiErrorElement
1819
import org.jetbrains.kotlin.com.intellij.psi.PsiFile
@@ -23,7 +24,14 @@ import java.nio.file.Path
2324
private val logger = LoggerFactory.getLogger("KotlinDiagnosticProvider")
2425

2526
internal data class KotlinDiagnosticExtra(
26-
val diagnostic: KaDiagnosticWithPsi<*>,
27+
/**
28+
* The unresolved-reference name extracted from an [KaFirDiagnostic.UnresolvedReference]
29+
* diagnostic, or `null` for any other diagnostic. This is plain data extracted *inside* the
30+
* `analyze` block on purpose: storing the [KaDiagnosticWithPsi] (a `KaLifetimeOwner`) here and
31+
* reading its members later from a code action would access it outside an `analyze` context and
32+
* crash with `KaInaccessibleLifetimeOwnerAccessException`.
33+
*/
34+
val unresolvedReference: String?,
2735
val compilationEnv: CompilationEnvironment,
2836
)
2937

@@ -79,8 +87,12 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu
7987
ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS)
8088
.forEach { diagnostic ->
8189
cancelChecker.abortIfCancelled()
90+
// Extract plain data while still inside the analyze context; never let
91+
// the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra).
92+
val unresolvedReference =
93+
(diagnostic as? KaFirDiagnostic.UnresolvedReference)?.reference
8294
add(diagnostic.toDiagnosticItem().apply {
83-
extra = KotlinDiagnosticExtra(diagnostic, env)
95+
extra = KotlinDiagnosticExtra(unresolvedReference, env)
8496
})
8597
}
8698
}

0 commit comments

Comments
 (0)