Skip to content

Commit 3d8090f

Browse files
authored
Stop the parasitic manager crashing on its first network request (#802)
OkHttp 5's Android artifact keeps the public suffix list in assets/, reached through a process-static Context set from androidx.startup. Parasitically the manager's manifest is never installed, so that provider never runs and DnsOverHttps -- which asks the list whether a host is private before opening any socket -- died with "Unable to load PublicSuffixDatabase.list" on the first lookup. Coil was already hand-initialised for this reason; OkHttp was not. Two things made that fatal rather than degrading: VectorDns caught only UnknownHostException, so an IllegalStateException escaped it and the fallback never engaged; and canaryBuilds and frameworkReleases called get() unguarded, with no CoroutineExceptionHandler anywhere in the manager, so the throw landed on the main thread. Coil's own initialisation then moves out of MainActivity to ServiceLocator.attach, for the same reason OkHttp's did: the debug demo host never opens MainActivity, so it had no image loader at all. Separately, for diagnosis: off, bypassed by a proxy, and latched onto the system resolver all looked identical from the sheet, so VectorDns now records what each lookup did and the sheet renders it, with a way to clear the latch. Two fixes fell out: - `direct` was a lazy val, so the proxy check ran once per process and joining a VPN mid-session was invisible. Read per lookup now. - The failure line named the host being resolved, which UnknownHostException already carries as its whole message -- so it printed twice while which way it failed went missing. Finally, a crash record is discarded when another build wrote it. A record outlives the build that made it, so the status card kept showing a crash the running build had already fixed -- which is what #799 was answered with: five traces from the build before the one that fixed them. Fixes #799
1 parent c147582 commit 3d8090f

28 files changed

Lines changed: 399 additions & 37 deletions

File tree

‎manager/build.gradle.kts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,9 @@ android {
9797
packaging {
9898
resources {
9999
excludes += "META-INF/**"
100+
// Java resources only, so it is inert against the Android artifact, which ships its
101+
// public suffix list under assets/. Pinning the JVM variant would move that list back
102+
// to okhttp3/internal/publicsuffix/ and this line would then delete it.
100103
excludes += "okhttp3/**"
101104
excludes += "kotlin/**"
102105
excludes += "**.properties"

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

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,23 @@ class GitHubRepository(
699699
} ?: CommitPerson(login = name, avatarUrl = null, profileUrl = null)
700700
}
701701

702+
/**
703+
* The releases page, or null when GitHub could not be reached.
704+
*
705+
* Both readers of this endpoint are launched from a `viewModelScope` or a `LaunchedEffect`,
706+
* neither of which has a `CoroutineExceptionHandler` anywhere in this app — so a throw here is
707+
* a fatal on the main thread rather than a card that stays empty. Every other request in this
708+
* file already guards itself; these two did not, which is how a resolver failure took the whole
709+
* manager down instead of leaving the update card blank.
710+
*
711+
* One helper for both because it is the same request: fetching it twice under two URLs built
712+
* from the same three constants is how the two lists drift apart.
713+
*/
714+
private fun releaseListJson(freshness: Freshness): String? =
715+
runCatching { get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness) }
716+
.onFailure { e -> Log.w(Constants.TAG, "update: github release list unavailable", e) }
717+
.getOrNull()
718+
702719
/**
703720
* The canary builds, newest first.
704721
*
@@ -714,9 +731,7 @@ class GitHubRepository(
714731
*/
715732
suspend fun canaryBuilds(freshness: Freshness = Freshness.Revalidate): List<CanaryBuild> =
716733
withContext(Dispatchers.IO) {
717-
val body =
718-
get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness)
719-
?: return@withContext emptyList()
734+
val body = releaseListJson(freshness) ?: return@withContext emptyList()
720735

721736
runCatching { json.decodeFromString<List<GhRelease>>(body) }
722737
.onFailure { e -> Log.e(Constants.TAG, "update: canary release list unreadable", e) }
@@ -757,9 +772,7 @@ class GitHubRepository(
757772
suspend fun frameworkReleases(freshness: Freshness = Freshness.Revalidate):
758773
List<FrameworkRelease> =
759774
withContext(Dispatchers.IO) {
760-
val body =
761-
get("$API/$REPO/releases?per_page=$CANARY_FETCH", freshness)
762-
?: return@withContext emptyList()
775+
val body = releaseListJson(freshness) ?: return@withContext emptyList()
763776

764777
runCatching { json.decodeFromString<List<GhRelease>>(body) }
765778
.onFailure { e -> Log.e(Constants.TAG, "update: release list unreadable", e) }

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

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ object CrashRecorder {
5353
@Volatile private var installed = false
5454

5555
/**
56-
* Takes over the default handler, once per process.
56+
* Takes over the default handler, once per process, and discards what another build left.
5757
*
5858
* Called from [org.matrix.vector.manager.di.ServiceLocator.attach], early in the activity's
5959
* `onCreate` and before anything that could fail, so the handler is in place before any screen
@@ -64,6 +64,7 @@ object CrashRecorder {
6464
if (installed) return
6565
installed = true
6666
val application = context.applicationContext ?: context
67+
runCatching { discardOtherBuilds(application) }
6768
val previous = Thread.getDefaultUncaughtExceptionHandler()
6869
Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
6970
runCatching { record(application, thread, throwable) }
@@ -90,6 +91,29 @@ object CrashRecorder {
9091
runCatching { files(context).forEach { it.delete() } }
9192
}
9293

94+
/**
95+
* Drops the records that some other build of the manager wrote.
96+
*
97+
* A record outlives the build that made it — the directory is this process's cache, which an
98+
* update does not clear — so the status card goes on showing a crash that the running build
99+
* has already fixed, until somebody thinks to press clear. That is not a stale line on a
100+
* screen. It is what a reporter copies into the issue to show the fix did not work: #799 was
101+
* answered with five traces from the build before the one that fixed them.
102+
*
103+
* Only at [install], never at [record]: this is about what a *new* build inherits, and a crash
104+
* loop within one build must keep every record it makes.
105+
*
106+
* A file whose second line cannot be read is kept. Losing a trace is the worse of the two
107+
* failures, and a record this class cannot parse is exactly the one worth still having.
108+
*/
109+
private fun discardOtherBuilds(context: Context) {
110+
val current = BUILD
111+
files(context).forEach { file ->
112+
val theirs = runCatching { file.useLines { it.drop(1).firstOrNull() } }.getOrNull()
113+
if (theirs != null && !theirs.startsWith(current)) runCatching { file.delete() }
114+
}
115+
}
116+
93117
/** Newest first, which is the order both the card and the clipboard want. */
94118
private fun files(context: Context): List<File> =
95119
directory(context)
@@ -124,11 +148,22 @@ object CrashRecorder {
124148
val parasitic = context.packageName == BuildConfig.INJECTED_PACKAGE_NAME
125149
val host = if (parasitic) "parasitic in ${context.packageName}" else "standalone"
126150
return "${TIMESTAMP.format(Date(at))}\n" +
127-
"manager ${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE}) " +
128-
"${BuildConfig.VERSION_HASH} · $host · thread ${thread.name} · " +
151+
"$BUILD · $host · thread ${thread.name} · " +
129152
"android ${android.os.Build.VERSION.RELEASE} (sdk ${android.os.Build.VERSION.SDK_INT})"
130153
}
131154

155+
/**
156+
* How a record says which build wrote it, and so what [discardOtherBuilds] matches on.
157+
*
158+
* The whole line rather than the hash alone: a build from a dirty tree carries the hash of the
159+
* commit it was built from, so two of them can share it while differing by everything that was
160+
* uncommitted at the time.
161+
*/
162+
private val BUILD: String
163+
get() =
164+
"manager ${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE}) " +
165+
BuildConfig.VERSION_HASH
166+
132167
private const val SUFFIX = ".log"
133168

134169
private val TIMESTAMP

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

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ import kotlinx.coroutines.flow.map
1212
import kotlinx.coroutines.flow.SharingStarted
1313
import android.annotation.SuppressLint
1414
import android.content.Context
15+
import coil3.ImageLoader
16+
import coil3.PlatformContext
17+
import coil3.SingletonImageLoader
18+
import coil3.network.okhttp.OkHttpNetworkFetcherFactory
1519
import kotlinx.coroutines.CoroutineScope
1620
import kotlinx.coroutines.Dispatchers
1721
import kotlinx.coroutines.SupervisorJob
@@ -36,6 +40,7 @@ import org.matrix.vector.manager.data.repository.SettingsRepository
3640
import org.matrix.vector.manager.ipc.DaemonClient
3741
import org.matrix.vector.manager.ipc.packageEventsFlow
3842
import org.matrix.vector.manager.net.HttpClientFactory
43+
import org.matrix.vector.manager.net.VectorDns
3944

4045
/**
4146
* Hand-rolled service location, deliberately not a DI framework.
@@ -77,7 +82,16 @@ object ServiceLocator {
7782

7883
val daemon: DaemonClient by lazy { DaemonClient(service) }
7984

80-
val http: OkHttpClient by lazy { HttpClientFactory.create(context, settings) }
85+
private val net: HttpClientFactory.NetStack by lazy {
86+
HttpClientFactory.create(context, settings)
87+
}
88+
89+
val http: OkHttpClient
90+
get() = net.client
91+
92+
/** The resolver inside [http], so the settings sheet can report what DoH is actually doing. */
93+
val dns: VectorDns
94+
get() = net.dns
8195

8296
val settings: SettingsRepository by lazy { SettingsRepository(context) }
8397

@@ -195,6 +209,21 @@ object ServiceLocator {
195209
// Before anything else that could fail. Nothing below is load-bearing for it, and a crash
196210
// during startup is exactly the one that is hardest to catch on a cable.
197211
CrashRecorder.install(appContext!!)
212+
213+
// Coil is configured by hand rather than through its manifest hooks, for the same reason
214+
// OkHttp is: parasitically this app's manifest is never installed, so nothing that
215+
// self-registers there ever runs. Here rather than in the activity because every entry
216+
// point comes through `attach` — including the debug demo host, which never opens
217+
// MainActivity and so had no image loader at all while this lived there.
218+
//
219+
// The factory is not called until the first image, so this costs nothing at startup and
220+
// does not build the OkHttp client before something asks for it.
221+
SingletonImageLoader.setSafe { platformContext: PlatformContext ->
222+
ImageLoader.Builder(platformContext)
223+
.components { add(OkHttpNetworkFetcherFactory(callFactory = { http })) }
224+
.build()
225+
}
226+
198227
observePackageChanges()
199228
}
200229

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

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import android.content.Context
44
import java.io.File
55
import java.util.concurrent.TimeUnit
66
import okhttp3.Cache
7+
import okhttp3.OkHttp
78
import okhttp3.OkHttpClient
89
import org.matrix.vector.manager.data.repository.SettingsRepository
910

@@ -25,7 +26,30 @@ object HttpClientFactory {
2526
private const val CACHE_DIR = "http_cache"
2627
private const val CACHE_SIZE_BYTES = 16L * 1024 * 1024
2728

28-
fun create(context: Context, settings: SettingsRepository): OkHttpClient {
29+
/**
30+
* The client and the resolver inside it.
31+
*
32+
* The resolver comes back alongside rather than being fished out of `client.dns` later: it is
33+
* the only thing that knows whether DoH is actually working, the settings sheet reports that,
34+
* and a cast back from the `Dns` interface would be a promise that nothing checks.
35+
*/
36+
class NetStack(val client: OkHttpClient, val dns: VectorDns)
37+
38+
fun create(context: Context, settings: SettingsRepository): NetStack {
39+
// OkHttp's Android artifact ships the public suffix list as an *asset* and reaches it
40+
// through a process-static Context that `PlatformInitializer` sets from `androidx.startup`.
41+
// Parasitically this app's manifest is never installed, so that provider never runs and the
42+
// first DoH lookup — which asks the list whether a host is private before opening any
43+
// socket — dies with "Unable to load PublicSuffixDatabase.list". OkHttp latches that
44+
// failure for the life of the process, so it has to be prevented rather than recovered
45+
// from.
46+
//
47+
// Here rather than in the activity because this is the only place a client is built, which
48+
// puts it on the path to every request in both parasitic and standalone runs and in the
49+
// debug demo host, which never opens MainActivity. Idempotent, so it is a no-op in the
50+
// standalone install, where the Startup initializer did run.
51+
OkHttp.initialize(context)
52+
2953
val cache = Cache(File(context.cacheDir, CACHE_DIR), CACHE_SIZE_BYTES)
3054

3155
val base =
@@ -40,6 +64,7 @@ object HttpClientFactory {
4064
// and the shared client — with its connection pool and its disk cache — is never rebuilt.
4165
// `base` is passed in as the bootstrap client because a DoH client must not itself resolve
4266
// through DoH.
43-
return base.newBuilder().dns(VectorDns(settings, base)).build()
67+
val dns = VectorDns(settings, base)
68+
return NetStack(base.newBuilder().dns(dns).build(), dns)
4469
}
4570
}

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

Lines changed: 104 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,48 @@ import android.util.Log
55
import java.net.InetAddress
66
import java.net.Proxy
77
import java.net.ProxySelector
8-
import java.net.UnknownHostException
98
import java.util.concurrent.TimeUnit
9+
import kotlinx.coroutines.flow.MutableStateFlow
10+
import kotlinx.coroutines.flow.StateFlow
11+
import kotlinx.coroutines.flow.asStateFlow
1012
import okhttp3.Dns
1113
import okhttp3.HttpUrl.Companion.toHttpUrl
1214
import okhttp3.OkHttpClient
1315
import okhttp3.dnsoverhttps.DnsOverHttps
1416
import org.matrix.vector.manager.data.repository.SettingsRepository
1517

18+
/**
19+
* What the last name lookup of this session actually did.
20+
*
21+
* Deliberately a record of the real resolver rather than something a probe could produce. A "test
22+
* DoH" button would be a second code path — another host, another moment — and it can pass while
23+
* the client that fetches the module list is failing. Only the shared resolver knows the truth, so
24+
* only the shared resolver reports it.
25+
*/
26+
sealed interface DohStatus {
27+
/** Nothing has been resolved yet, so there is nothing to report. */
28+
data object Untested : DohStatus
29+
30+
/** The setting is off; names went to the system resolver. */
31+
data object Disabled : DohStatus
32+
33+
/** A proxy is configured, so resolving is its job and DoH was skipped. */
34+
data object Bypassed : DohStatus
35+
36+
/** The last lookup went through the DoH endpoint. */
37+
data class Working(val host: String) : DohStatus
38+
39+
/**
40+
* DoH failed and the session has fallen back to the system resolver.
41+
*
42+
* No hostname here on purpose. What failed is reaching the DoH endpoint; whichever name was
43+
* being looked up at that moment is incidental — it is simply whatever the app asked for first
44+
* — and naming it reads as though that host were the subject of a test. The log line keeps it
45+
* for anyone diagnosing; the sheet does not need it.
46+
*/
47+
data class FellBack(val reason: String) : DohStatus
48+
}
49+
1650
/**
1751
* Name resolution: DNS over HTTPS when it helps, the system resolver when it does not.
1852
*
@@ -27,15 +61,24 @@ import org.matrix.vector.manager.data.repository.SettingsRepository
2761
* - a configured HTTP proxy disables DoH entirely, because the proxy is doing the resolving and
2862
* bootstrap IPs are meaningless to it.
2963
*
30-
* The setting is read **per lookup** rather than baked into the client at construction. OkHttp
31-
* cannot have its DNS swapped on a live client, and rebuilding the shared client would drop the
32-
* connection pool and orphan the disk cache, so reading it here is what lets the switch take effect
33-
* before the next process start.
64+
* Every one of those branches used to be invisible: off, bypassed and latched all looked like the
65+
* same working switch, and the fallback existed only as a log line. [status] is what each lookup
66+
* did, so the sheet that owns the switch can say which of them is happening.
67+
*
68+
* The setting and the proxy are both read **per lookup** rather than baked into the client at
69+
* construction. OkHttp cannot have its DNS swapped on a live client, and rebuilding the shared
70+
* client would drop the connection pool and orphan the disk cache, so reading them here is what
71+
* lets a switch — or joining a VPN — take effect before the next process start.
3472
*/
3573
class VectorDns(private val settings: SettingsRepository, bootstrapClient: OkHttpClient) : Dns {
3674

3775
private val endpoint = "https://cloudflare-dns.com/dns-query".toHttpUrl()
3876

77+
private val _status = MutableStateFlow<DohStatus>(DohStatus.Untested)
78+
79+
/** What the last lookup did. See [DohStatus] for why this is observed and never probed. */
80+
val status: StateFlow<DohStatus> = _status.asStateFlow()
81+
3982
/**
4083
* Latched once the DoH endpoint proves unreachable.
4184
*
@@ -66,28 +109,79 @@ class VectorDns(private val settings: SettingsRepository, bootstrapClient: OkHtt
66109
.build()
67110
}
68111

69-
/** True when nothing is proxying our traffic, which is the only case where DoH is ours to do. */
70-
private val direct: Boolean by lazy {
112+
/**
113+
* True when nothing is proxying our traffic, which is the only case where DoH is ours to do.
114+
*
115+
* Asked on every lookup rather than cached. A proxy can appear mid-session — joining a VPN or a
116+
* work profile does exactly that — and a value read once at startup would keep sending queries
117+
* to Cloudflare long after the answer changed, while [status] claimed a state that was no
118+
* longer true. The call is local and a lookup is about to do network I/O anyway.
119+
*/
120+
private fun direct(): Boolean =
71121
runCatching {
72122
ProxySelector.getDefault().select(endpoint.toUri()).firstOrNull() == Proxy.NO_PROXY
73123
}
74124
.getOrDefault(true)
125+
126+
/**
127+
* Clears the session latch so the next lookup tries DoH again.
128+
*
129+
* The latch is what keeps a blocked endpoint from costing five seconds per name, but "the
130+
* session" here is `com.android.shell`, a process nobody can restart on purpose — so without
131+
* this a single bad lookup on a captive portal disables DoH until something else happens to
132+
* kill the host. This is the way back, and it is offered only once the fallback has happened.
133+
*/
134+
fun retry() {
135+
dohUnavailable = false
136+
_status.value = DohStatus.Untested
75137
}
76138

77139
override fun lookup(hostname: String): List<InetAddress> {
78-
if (settings.dohEnabled.value && direct && !dohUnavailable) {
140+
if (!settings.dohEnabled.value) {
141+
_status.value = DohStatus.Disabled
142+
} else if (!direct()) {
143+
_status.value = DohStatus.Bypassed
144+
} else if (!dohUnavailable) {
79145
try {
80-
return doh.lookup(hostname)
81-
} catch (e: UnknownHostException) {
146+
val resolved = doh.lookup(hostname)
147+
_status.value = DohStatus.Working(hostname)
148+
return resolved
149+
} catch (e: Exception) {
150+
// Every way DoH can fail, not only "no such host". A blocked endpoint raises
151+
// UnknownHostException, a slow one raises InterruptedIOException from the timeouts
152+
// above, and a resolver that cannot read its own public suffix list raises
153+
// IllegalStateException before a socket is ever opened — which used to escape this
154+
// method entirely, so the latch never closed and the fallback this class is built
155+
// around never engaged. Anything arriving here means DoH is not usable, which is
156+
// the condition documented above.
157+
//
158+
// Exception and not Throwable: an OutOfMemoryError is not a DNS outcome. There is
159+
// no CancellationException to preserve either — this is a plain blocking call on an
160+
// OkHttp dispatcher thread, with no coroutine in the stack.
82161
dohUnavailable = true
162+
_status.value = DohStatus.FellBack(e.describe(hostname))
83163
Log.w(
84164
Constants.TAG,
85165
"dns: DoH lookup of $hostname failed, using the system resolver for this session",
86166
e,
87167
)
88168
}
89169
}
170+
// Latched: the status already says why, and repeating it on every name would only replace
171+
// the host that actually failed with whichever one asked next.
90172
return Dns.SYSTEM.lookup(hostname)
91173
}
92174

175+
/**
176+
* A line short enough to sit under a switch, and worth the room it takes.
177+
*
178+
* The class name whenever the message would not add anything. `UnknownHostException` carries
179+
* the hostname as its entire message, so using it verbatim printed the name twice — "could not
180+
* resolve example.org (example.org)" — while the one thing a reader wants, *which way* it
181+
* failed, went missing. The class name is jargon, but it is jargon that distinguishes a blocked
182+
* endpoint from a timeout, and it is what a bug report needs to carry anyway.
183+
*/
184+
private fun Throwable.describe(host: String): String =
185+
message?.takeIf { it.isNotBlank() && !it.equals(host, ignoreCase = true) }
186+
?: javaClass.simpleName
93187
}

0 commit comments

Comments
 (0)