Fix the manager crashing on launch

manager/build.gradle.kts

@@ -97,6 +97,9 @@ android {
  packaging {
  resources {
  excludes += "META-INF/**"
+ // Java resources only, so it is inert against the Android artifact, which ships its
+ // public suffix list under assets/. Pinning the JVM variant would move that list back
+ // to okhttp3/internal/publicsuffix/ and this line would then delete it.
  excludes += "okhttp3/**"
  excludes += "kotlin/**"
  excludes += "**.properties"

manager/src/main/kotlin/org/matrix/vector/manager/data/github/GitHubRepository.kt

@@ -699,6 +699,23 @@ class GitHubRepository(
  } ?: CommitPerson(login = name, avatarUrl = null, profileUrl = null)
  }
 
+ /**
+ * The releases page, or null when GitHub could not be reached.
+ *
+ * Both readers of this endpoint are launched from a `viewModelScope` or a `LaunchedEffect`,
+ * neither of which has a `CoroutineExceptionHandler` anywhere in this app — so a throw here is
+ * a fatal on the main thread rather than a card that stays empty. Every other request in this
+ * file already guards itself; these two did not, which is how a resolver failure took the whole
+ * manager down instead of leaving the update card blank.
+ *
+ * One helper for both because it is the same request: fetching it twice under two URLs built
+ * from the same three constants is how the two lists drift apart.
+ */
+ private fun releaseListJson(freshness: Freshness): String? =
+ runCatching { get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness) }
+ .onFailure { e -> Log.w(Constants.TAG, "update: github release list unavailable", e) }
+ .getOrNull()
+
  /**
  * The canary builds, newest first.
  *
@@ -714,9 +731,7 @@ class GitHubRepository(
  */
  suspend fun canaryBuilds(freshness: Freshness = Freshness.Revalidate): List<CanaryBuild> =
  withContext(Dispatchers.IO) {
- val body =
- get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness)
- ?: return@withContext emptyList()
+ val body = releaseListJson(freshness) ?: return@withContext emptyList()
 
  runCatching { json.decodeFromString<List<GhRelease>>(body) }
  .onFailure { e -> Log.e(Constants.TAG, "update: canary release list unreadable", e) }
@@ -757,9 +772,7 @@ class GitHubRepository(
  suspend fun frameworkReleases(freshness: Freshness = Freshness.Revalidate):
  List<FrameworkRelease> =
  withContext(Dispatchers.IO) {
- val body =
- get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness)
- ?: return@withContext emptyList()
+ val body = releaseListJson(freshness) ?: return@withContext emptyList()
 
  runCatching { json.decodeFromString<List<GhRelease>>(body) }
  .onFailure { e -> Log.e(Constants.TAG, "update: release list unreadable", e) }

manager/src/main/kotlin/org/matrix/vector/manager/data/log/CrashRecorder.kt

@@ -53,7 +53,7 @@ object CrashRecorder {
  @Volatile private var installed = false
 
  /**
- * Takes over the default handler, once per process.
+ * Takes over the default handler, once per process, and discards what another build left.
  *
  * Called from [org.matrix.vector.manager.di.ServiceLocator.attach], early in the activity's
  * `onCreate` and before anything that could fail, so the handler is in place before any screen
@@ -64,6 +64,7 @@ object CrashRecorder {
  if (installed) return
  installed = true
  val application = context.applicationContext ?: context
+ runCatching { discardOtherBuilds(application) }
  val previous = Thread.getDefaultUncaughtExceptionHandler()
  Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
  runCatching { record(application, thread, throwable) }
@@ -90,6 +91,29 @@ object CrashRecorder {
  runCatching { files(context).forEach { it.delete() } }
  }
 
+ /**
+ * Drops the records that some other build of the manager wrote.
+ *
+ * A record outlives the build that made it — the directory is this process's cache, which an
+ * update does not clear — so the status card goes on showing a crash that the running build
+ * has already fixed, until somebody thinks to press clear. That is not a stale line on a
+ * screen. It is what a reporter copies into the issue to show the fix did not work: #799 was
+ * answered with five traces from the build before the one that fixed them.
+ *
+ * Only at [install], never at [record]: this is about what a *new* build inherits, and a crash
+ * loop within one build must keep every record it makes.
+ *
+ * A file whose second line cannot be read is kept. Losing a trace is the worse of the two
+ * failures, and a record this class cannot parse is exactly the one worth still having.
+ */
+ private fun discardOtherBuilds(context: Context) {
+ val current = BUILD
+ files(context).forEach { file ->
+ val theirs = runCatching { file.useLines { it.drop(1).firstOrNull() } }.getOrNull()
+ if (theirs != null && !theirs.startsWith(current)) runCatching { file.delete() }
+ }
+ }
+
  /** Newest first, which is the order both the card and the clipboard want. */
  private fun files(context: Context): List<File> =
  directory(context)
@@ -124,11 +148,22 @@ object CrashRecorder {
  val parasitic = context.packageName == BuildConfig.INJECTED_PACKAGE_NAME
  val host = if (parasitic) "parasitic in ${context.packageName}" else "standalone"
  return "${TIMESTAMP.format(Date(at))}\n" +
- "manager ${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE}) " +
- "${BuildConfig.VERSION_HASH} · $host · thread ${thread.name} · " +
+ "$BUILD · $host · thread ${thread.name} · " +
  "android ${android.os.Build.VERSION.RELEASE} (sdk ${android.os.Build.VERSION.SDK_INT})"
  }
 
+ /**
+ * How a record says which build wrote it, and so what [discardOtherBuilds] matches on.
+ *
+ * The whole line rather than the hash alone: a build from a dirty tree carries the hash of the
+ * commit it was built from, so two of them can share it while differing by everything that was
+ * uncommitted at the time.
+ */
+ private val BUILD: String
+ get() =
+ "manager ${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE}) " +
+ BuildConfig.VERSION_HASH
+
  private const val SUFFIX = ".log"
 
  private val TIMESTAMP

manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt

@@ -12,6 +12,10 @@ import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.SharingStarted
 import android.annotation.SuppressLint
 import android.content.Context
+import coil3.ImageLoader
+import coil3.PlatformContext
+import coil3.SingletonImageLoader
+import coil3.network.okhttp.OkHttpNetworkFetcherFactory
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.SupervisorJob
@@ -36,6 +40,7 @@ import org.matrix.vector.manager.data.repository.SettingsRepository
 import org.matrix.vector.manager.ipc.DaemonClient
 import org.matrix.vector.manager.ipc.packageEventsFlow
 import org.matrix.vector.manager.net.HttpClientFactory
+import org.matrix.vector.manager.net.VectorDns
 
 /**
  * Hand-rolled service location, deliberately not a DI framework.
@@ -77,7 +82,16 @@ object ServiceLocator {
 
  val daemon: DaemonClient by lazy { DaemonClient(service) }
 
- val http: OkHttpClient by lazy { HttpClientFactory.create(context, settings) }
+ private val net: HttpClientFactory.NetStack by lazy {
+ HttpClientFactory.create(context, settings)
+ }
+
+ val http: OkHttpClient
+ get() = net.client
+
+ /** The resolver inside [http], so the settings sheet can report what DoH is actually doing. */
+ val dns: VectorDns
+ get() = net.dns
 
  val settings: SettingsRepository by lazy { SettingsRepository(context) }
 
@@ -195,6 +209,21 @@ object ServiceLocator {
  // Before anything else that could fail. Nothing below is load-bearing for it, and a crash
  // during startup is exactly the one that is hardest to catch on a cable.
  CrashRecorder.install(appContext!!)
+
+ // Coil is configured by hand rather than through its manifest hooks, for the same reason
+ // OkHttp is: parasitically this app's manifest is never installed, so nothing that
+ // self-registers there ever runs. Here rather than in the activity because every entry
+ // point comes through `attach` — including the debug demo host, which never opens
+ // MainActivity and so had no image loader at all while this lived there.
+ //
+ // The factory is not called until the first image, so this costs nothing at startup and
+ // does not build the OkHttp client before something asks for it.
+ SingletonImageLoader.setSafe { platformContext: PlatformContext ->
+ ImageLoader.Builder(platformContext)
+ .components { add(OkHttpNetworkFetcherFactory(callFactory = { http })) }
+ .build()
+ }
+
  observePackageChanges()
  }
 

manager/src/main/kotlin/org/matrix/vector/manager/net/HttpClientFactory.kt

@@ -4,6 +4,7 @@ import android.content.Context
 import java.io.File
 import java.util.concurrent.TimeUnit
 import okhttp3.Cache
+import okhttp3.OkHttp
 import okhttp3.OkHttpClient
 import org.matrix.vector.manager.data.repository.SettingsRepository
 
@@ -25,7 +26,30 @@ object HttpClientFactory {
  private const val CACHE_DIR = "http_cache"
  private const val CACHE_SIZE_BYTES = 16L * 1024 * 1024
 
- fun create(context: Context, settings: SettingsRepository): OkHttpClient {
+ /**
+ * The client and the resolver inside it.
+ *
+ * The resolver comes back alongside rather than being fished out of `client.dns` later: it is
+ * the only thing that knows whether DoH is actually working, the settings sheet reports that,
+ * and a cast back from the `Dns` interface would be a promise that nothing checks.
+ */
+ class NetStack(val client: OkHttpClient, val dns: VectorDns)
+
+ fun create(context: Context, settings: SettingsRepository): NetStack {
+ // OkHttp's Android artifact ships the public suffix list as an *asset* and reaches it
+ // through a process-static Context that `PlatformInitializer` sets from `androidx.startup`.
+ // Parasitically this app's manifest is never installed, so that provider never runs and the
+ // first DoH lookup — which asks the list whether a host is private before opening any
+ // socket — dies with "Unable to load PublicSuffixDatabase.list". OkHttp latches that
+ // failure for the life of the process, so it has to be prevented rather than recovered
+ // from.
+ //
+ // Here rather than in the activity because this is the only place a client is built, which
+ // puts it on the path to every request in both parasitic and standalone runs and in the
+ // debug demo host, which never opens MainActivity. Idempotent, so it is a no-op in the
+ // standalone install, where the Startup initializer did run.
+ OkHttp.initialize(context)
+
  val cache = Cache(File(context.cacheDir, CACHE_DIR), CACHE_SIZE_BYTES)
 
  val base =
@@ -40,6 +64,7 @@ object HttpClientFactory {
  // and the shared client — with its connection pool and its disk cache — is never rebuilt.
  // `base` is passed in as the bootstrap client because a DoH client must not itself resolve
  // through DoH.
- return base.newBuilder().dns(VectorDns(settings, base)).build()
+ val dns = VectorDns(settings, base)
+ return NetStack(base.newBuilder().dns(dns).build(), dns)
  }
 }

manager/src/main/kotlin/org/matrix/vector/manager/net/VectorDns.kt

@@ -5,14 +5,48 @@ import android.util.Log
 import java.net.InetAddress
 import java.net.Proxy
 import java.net.ProxySelector
-import java.net.UnknownHostException
 import java.util.concurrent.TimeUnit
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.asStateFlow
 import okhttp3.Dns
 import okhttp3.HttpUrl.Companion.toHttpUrl
 import okhttp3.OkHttpClient
 import okhttp3.dnsoverhttps.DnsOverHttps
 import org.matrix.vector.manager.data.repository.SettingsRepository
 
+/**
+ * What the last name lookup of this session actually did.
+ *
+ * Deliberately a record of the real resolver rather than something a probe could produce. A "test
+ * DoH" button would be a second code path — another host, another moment — and it can pass while
+ * the client that fetches the module list is failing. Only the shared resolver knows the truth, so
+ * only the shared resolver reports it.
+ */
+sealed interface DohStatus {
+ /** Nothing has been resolved yet, so there is nothing to report. */
+ data object Untested : DohStatus
+
+ /** The setting is off; names went to the system resolver. */
+ data object Disabled : DohStatus
+
+ /** A proxy is configured, so resolving is its job and DoH was skipped. */
+ data object Bypassed : DohStatus
+
+ /** The last lookup went through the DoH endpoint. */
+ data class Working(val host: String) : DohStatus
+
+ /**
+ * DoH failed and the session has fallen back to the system resolver.
+ *
+ * No hostname here on purpose. What failed is reaching the DoH endpoint; whichever name was
+ * being looked up at that moment is incidental — it is simply whatever the app asked for first
+ * — and naming it reads as though that host were the subject of a test. The log line keeps it
+ * for anyone diagnosing; the sheet does not need it.
+ */
+ data class FellBack(val reason: String) : DohStatus
+}
+
 /**
  * Name resolution: DNS over HTTPS when it helps, the system resolver when it does not.
  *
@@ -27,15 +61,24 @@ import org.matrix.vector.manager.data.repository.SettingsRepository
  * - a configured HTTP proxy disables DoH entirely, because the proxy is doing the resolving and
  * bootstrap IPs are meaningless to it.
  *
- * The setting is read **per lookup** rather than baked into the client at construction. OkHttp
- * cannot have its DNS swapped on a live client, and rebuilding the shared client would drop the
- * connection pool and orphan the disk cache, so reading it here is what lets the switch take effect
- * before the next process start.
+ * Every one of those branches used to be invisible: off, bypassed and latched all looked like the
+ * same working switch, and the fallback existed only as a log line. [status] is what each lookup
+ * did, so the sheet that owns the switch can say which of them is happening.
+ *
+ * The setting and the proxy are both read **per lookup** rather than baked into the client at
+ * construction. OkHttp cannot have its DNS swapped on a live client, and rebuilding the shared
+ * client would drop the connection pool and orphan the disk cache, so reading them here is what
+ * lets a switch — or joining a VPN — take effect before the next process start.
  */
 class VectorDns(private val settings: SettingsRepository, bootstrapClient: OkHttpClient) : Dns {
 
  private val endpoint = "https://cloudflare-dns.com/dns-query".toHttpUrl()
 
+ private val _status = MutableStateFlow<DohStatus>(DohStatus.Untested)
+
+ /** What the last lookup did. See [DohStatus] for why this is observed and never probed. */
+ val status: StateFlow<DohStatus> = _status.asStateFlow()
+
  /**
  * Latched once the DoH endpoint proves unreachable.
  *
@@ -66,28 +109,79 @@ class VectorDns(private val settings: SettingsRepository, bootstrapClient: OkHtt
  .build()
  }
 
- /** True when nothing is proxying our traffic, which is the only case where DoH is ours to do. */
- private val direct: Boolean by lazy {
+ /**
+ * True when nothing is proxying our traffic, which is the only case where DoH is ours to do.
+ *
+ * Asked on every lookup rather than cached. A proxy can appear mid-session — joining a VPN or a
+ * work profile does exactly that — and a value read once at startup would keep sending queries
+ * to Cloudflare long after the answer changed, while [status] claimed a state that was no
+ * longer true. The call is local and a lookup is about to do network I/O anyway.
+ */
+ private fun direct(): Boolean =
  runCatching {
  ProxySelector.getDefault().select(endpoint.toUri()).firstOrNull() == Proxy.NO_PROXY
  }
  .getOrDefault(true)
+
+ /**
+ * Clears the session latch so the next lookup tries DoH again.
+ *
+ * The latch is what keeps a blocked endpoint from costing five seconds per name, but "the
+ * session" here is `com.android.shell`, a process nobody can restart on purpose — so without
+ * this a single bad lookup on a captive portal disables DoH until something else happens to
+ * kill the host. This is the way back, and it is offered only once the fallback has happened.
+ */
+ fun retry() {
+ dohUnavailable = false
+ _status.value = DohStatus.Untested
  }
 
  override fun lookup(hostname: String): List<InetAddress> {
- if (settings.dohEnabled.value && direct && !dohUnavailable) {
+ if (!settings.dohEnabled.value) {
+ _status.value = DohStatus.Disabled
+ } else if (!direct()) {
+ _status.value = DohStatus.Bypassed
+ } else if (!dohUnavailable) {
  try {
- return doh.lookup(hostname)
- } catch (e: UnknownHostException) {
+ val resolved = doh.lookup(hostname)
+ _status.value = DohStatus.Working(hostname)
+ return resolved
+ } catch (e: Exception) {
+ // Every way DoH can fail, not only "no such host". A blocked endpoint raises
+ // UnknownHostException, a slow one raises InterruptedIOException from the timeouts
+ // above, and a resolver that cannot read its own public suffix list raises
+ // IllegalStateException before a socket is ever opened — which used to escape this
+ // method entirely, so the latch never closed and the fallback this class is built
+ // around never engaged. Anything arriving here means DoH is not usable, which is
+ // the condition documented above.
+ //
+ // Exception and not Throwable: an OutOfMemoryError is not a DNS outcome. There is
+ // no CancellationException to preserve either — this is a plain blocking call on an
+ // OkHttp dispatcher thread, with no coroutine in the stack.
  dohUnavailable = true
+ _status.value = DohStatus.FellBack(e.describe(hostname))
  Log.w(
  Constants.TAG,
  "dns: DoH lookup of $hostname failed, using the system resolver for this session",
  e,
  )
  }
  }
+ // Latched: the status already says why, and repeating it on every name would only replace
+ // the host that actually failed with whichever one asked next.
  return Dns.SYSTEM.lookup(hostname)
  }
 
+ /**
+ * A line short enough to sit under a switch, and worth the room it takes.
+ *
+ * The class name whenever the message would not add anything. `UnknownHostException` carries
+ * the hostname as its entire message, so using it verbatim printed the name twice — "could not
+ * resolve example.org (example.org)" — while the one thing a reader wants, *which way* it
+ * failed, went missing. The class name is jargon, but it is jargon that distinguishes a blocked
+ * endpoint from a timeout, and it is what a bug report needs to carry anyway.
+ */
+ private fun Throwable.describe(host: String): String =
+ message?.takeIf { it.isNotBlank() && !it.equals(host, ignoreCase = true) }
+ ?: javaClass.simpleName
 }