Repository navigation
fix(mobile): resolve document providers once per selection - #13147
Conversation
Signed-off-by: jabrailkhalil <[email protected]>
|
I checked the browser smoke log: this job was cancelled at its 30-minute limit, rather than completing with an Android picker assertion failure. Chromium/WebKit installation consumed 15m34s, and the browser smoke started about 22m35s into the job before cancellation during execution. Run/job log. The native picker checks pass on API 26 and API 36, and the latest code review reports no findings. I cannot rerun upstream Actions with the repository permissions available to my account. Please rerun the timed-out browser job when reviewing; I am keeping CI timeout/download changes outside this picker fix. |
Local verification of #13147 — verdict: merge-ready264/264 scripted assertions passed across 6 harness arms (base / head / 4 mutants). Verified head: 中文摘要结论：可以合并。 本地双臂 A/B 验证了核心声明：同一 authority 的 100 个文档，改动前解析 provider 100 次，改动后 1 次（混合双 authority 为 2 次），且每个 URI 的权限检查一次不少（100 次保留）。S3 证明缓存仅存在于单次选择内（连续两次选择 → 2 次解析，而非 1 次）。变异矩阵：无 authority 键的单槽缓存（m1）会放过同 UID 危险 provider 的文档（S11 被错误接受）， hoist PackageManager 但不缓存（m2）在混合 authority 下计数不符——两者恰被新增的 mixed-authority 测试钉住，与作者声明一致；m3（去掉 UID 检查）/m4（每 authority 只查一次授权）分别被既有安全性与逐文档授权场景击杀，构成阳性对照。另核实：两份设计文档链接目标均存在且中英对称；head 的 CI 在 API 26/36 模拟器上 29 项设备测试全部通过。未覆盖：本地 Gradle 全量构建与真机/模拟器执行（本机无 Android SDK，已由 CI 覆盖）、第三方 provider 行为、UI 延迟。详细数字见下方表格。 Central claim and A/BClaim: validating a picker result resolves each document-provider authority once per selection while keeping every URI permission check; the cache lives only for one validation call. Method: compiled the real
Evidence:
Mutation matrix (vacuity check on the new tests)Four single-point mutants of the head file were compiled and driven through the identical 11 scenarios; each behaves exactly as predicted, and each diverges from head on at least one cell the checked-in test class pins (evidence:
m3 kills prove the harness can fail (it does, on the exact cells), so the green cells are evidence, not a dead harness. This matches the PR's own R1-1 mutation claims and sharpens them: m1 is not merely a count mismatch, it accepts a document from a provider owned by the app's own UID.
Corrections to earlier review comments
Other checks
FindingsNone blocking. One informational observation: a hypothetical m2-style regression (hoisting Not covered
MethodologymacOS (arm64), JBR 17.0.10, Kotlin compiler 1.9.20 (embeddable jar, matching the project's pinned Kotlin version). Sources extracted with |
|
@wenshao Thank you for the detailed independent verification and mutation matrix. The mixed-authority case is useful evidence that the cache retains the provider UID boundary as well as reducing lookups. I will keep the verified head |
|
@qewn-code /triage |
|
@wenshao The independently verified head is still 29a21f1; I have not changed it. I noticed the latest triage mention says @qewn-code, so if a rerun was intended, the trigger is @qwen-code /triage. The Android checks passed, while the Web Shell smoke run remains cancelled after its time limit. Could you retry the intended triage trigger and the smoke check on this head when convenient? I will address any confirmed current-head finding. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed head: 29a21f16ce2713abc89e55d2895b89eb5c71421e (base main).
Approve. No historical blocking finding stands on this PR, and a Critical-only scan of the production change found nothing blocking. The change is six lines in a security-relevant validator, so I read all of it and the tests that pin it.
Historical blocking findings
None to re-verify. Both review threads are resolved, there are no [Critical] inline comments and no review ledger, and reviewDecision is REVIEW_REQUIRED — no CHANGES_REQUESTED was ever filed. wenshao approved this exact head.
The one finding referenced on the PR, R1-1, was a request for a mixed-authority regression test rather than a defect in shipped behaviour. It was addressed in da88d57c and the current head goes well past it: four separate tests now cover mixed authorities rather than the one that was asked for.
Critical-only scan
The cache narrows only the provider resolution, and every per-URI control still runs per URI. In NativeFilePicker.validateResult the loop keeps all four checks in place — content scheme, non-blank authority, provider ownership (rejecting a provider whose applicationInfo.uid equals the app's own), and checkUriPermission for FLAG_GRANT_READ_URI_PERMISSION — and only the resolveContentProvider call moves behind providers.getOrPut(uri.authority!!) { … }. So a selection of 100 documents from one authority performs one Binder resolution and still 100 permission checks, which is the claim the PR makes and the code bears it out.
Nothing unsafe can be cached, and an unresolvable provider still rejects the whole selection. The map is MutableMap<String, ProviderInfo> with a non-null value type, and the lambda's ?: return null is a non-local return out of validateResult (getOrPut is inline, and the enclosing function returns Array<Uri>? and already uses bare return null at seven other sites, including inside a lambda at line 90). So when a provider cannot be resolved the function returns immediately, the put never happens, and no null or sentinel enters the map — a later URI of the same authority cannot read a cached miss and skip the rejection.
The cache is scoped to one selection, which is what keeps it safe across requests. providers is a local val created inside validateResult before the loop, not a field on the picker or a hoisted manager reference. A provider ownership or availability change between two picker results is therefore picked up on the next call; only redundant work within one synchronous result is eliminated. Within a selection the loop is synchronous, so all URIs sharing an authority are judged against one consistent ProviderInfo — no wider TOCTOU window than the per-URI re-resolution it replaces.
Authority keying is the load-bearing detail and the tests pin it from both sides. sameProviderIsResolvedOncePerSelectionAndEveryGrantIsChecked asserts providerManagerReads == 1 and permissionChecks == (index + 1) * uris.size across repeated selections, so it fails if the cache is removed and fails if a permission check is cached away. cachedProviderDoesNotAllowAnotherDocumentWithoutItsOwnGrant is the security-critical case: two documents from one provider with the second ungranted must reject the entire result while still showing one resolution and two permission checks — proving a document cannot ride on its sibling's grant. secondAuthorityInOneSelectionIsResolvedSeparately and mixedAuthoritiesStillRequireSeparateGrantsForTheSameDocumentIndex require two resolutions, so a one-slot cache without an authority key would fail them, and grants are shown to be per-authority rather than per-document-index. multipleGrantedAuthoritiesPreserveOrderAndResolveEachProviderOnce adds exact-order assertion and reads each document's bytes back against the fixture's expected contents, so the cache cannot cross-wire two authorities. The fixture provider now serves two authorities and the counting context overrides getPackageManager() and checkUriPermission, counting at the controller's real boundary rather than at a mock seam.
No Critical found. The two design-doc edits add synchronised English/Chinese pointers to the implemented file-selection design, and the manifest change only declares the fixture provider's second authority.
CI
One check reports failure and it is not attributable to this PR. web-shell E2E Smoke (ubuntu-latest, Node 22.x) has conclusion = cancelled with no failed step; its log shows the vite proxy repeatedly refused by a backend that never came up (connect ECONNREFUSED 127.0.0.1:4170, proxy errors on /workspace/models) until the job hit its 30-minute timeout and was cancelled. The run was against this head, so it is not stale — but this diff touches only packages/mobile-shell/** and two docs/design/mobile-android-shell*.md files, with no web-shell, TypeScript, daemon or port-configuration change that could affect that lane. The remaining 17 checks pass and 26 are skipped.
One coverage caveat worth stating plainly: there is no mobile or Android lane in this PR's check list at all, so no CI job compiles or runs the changed Kotlin. Green CI here is not evidence about this change. My approval rests on reading the six production lines and the tests, plus the author's reported local Gradle and JVM-harness runs; I did not build or execute anything myself.
Scope note
Approval is bound to commit 29a21f16. Given the absent CI coverage for packages/mobile-shell, running the documented ./gradlew :app:assembleDebug :app:testDebugUnitTest :app:lintDebug and the FilePickerDeviceTest connected suite once on a device or emulator before merge would be the natural last step — not because I found a defect, but because nothing in CI would catch one.
3 participants
What this PR does
Resolve each document-provider authority once while validating a native file-picker result. Keep the cache local to one result and still check the content scheme, authority, provider ownership and read permission for every selected URI.
Why it's needed
The follow-up review of #12126 identified repeated provider-resolution Binder calls for documents from the same authority. Before this change, a selection of 100 documents from one provider performs 100 resolutions. The new code performs one resolution while retaining all 100 permission checks. This addresses the nonblocking provider-cache suggestion separately from the already merged file-selection feature. The Phase 1 design also gains synchronized English/Chinese notices linking to the implemented file-selection design, addressing the stale-documentation finding in the same parent review.
Reviewer Test Plan
How to verify
packages/mobile-shell, run./gradlew :app:assembleDebug :app:assembleDebugAndroidTest :app:testDebugUnitTest :app:lintDebug../gradlew :app:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=com.qwen.mobileshell.FilePickerDeviceTest.Evidence (Before & After)
The actual production controller was also compiled and executed in a local JVM harness with controlled Android API stubs. Both cache-count assertions fail on upstream
0a5f518b4f73and pass with this change; all nine behavioral/security controls pass before and after.The harness also checks provider ownership/availability changes between requests, denied URI grants, unsafe/unknown/same-UID providers, deduplication and the selection limit. These are controller call-count observations, not Android UI timing measurements. The checked-in Android tests count accesses to the package manager at the controller's provider-resolution boundary and delegate real permission checks to the target context. To verify R1-1, the three actual checked-in test methods were also executed through JUnit on the JVM with controlled Android API stubs: production passes all three; both a one-slot cache without an authority key and a manager-hoisting/no-cache mutation pass the old two tests but fail the new mixed-authority test (expected 2 manager accesses, observed 1). This mutation execution is not Android instrumentation.
Tested on
70256adaa9: API 26: 22 passed / 4 expected skips; API 36: 25 passed / 1 expected skip; all 19 file-picker tests passed on both; CI for the updated test head is pendingEnvironment
Windows, JDK 17, Gradle 8.2.1, Android SDK 34. The instrumentation tests were compiled and packaged, but have not been executed on an Android emulator or physical device locally.
Risk & Scope
Linked Issues
Follow-up to #12126, related to #11704 and #13111. Reviewer evidence: #12126 (comment).
中文说明
本 PR 的改动
验证原生文件选择器结果时，每个文档提供程序 authority 只解析一次。缓存仅存在于本次结果处理中；仍对每个选中的 URI 检查 content scheme、authority、提供程序所有者以及读取权限。
为什么需要
#12126 的后续评审指出，同一 authority 下的多个文档会重复触发提供程序解析的 Binder 调用。改动前，同一个提供程序的 100 个文档会触发 100 次解析；改动后只解析一次，同时保留全部 100 次权限检查。本 PR 单独处理这一非阻塞缓存建议，不扩大已合入文件选择功能的范围。Phase 1 设计文档同时添加同步的中英文说明，链接到已实现的文件选择设计，处理同一次原 PR 评审中的文档过时问题。
评审测试计划
如何验证
packages/mobile-shell运行./gradlew :app:assembleDebug :app:assembleDebugAndroidTest :app:testDebugUnitTest :app:lintDebug。./gradlew :app:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=com.qwen.mobileshell.FilePickerDeviceTest。证据（改动前后）
另用受控 Android API 桩构建本地 JVM harness，编译并运行实际生产控制器。在 upstream
0a5f518b4f73上，两项缓存调用次数断言均失败；改动后均通过。其余九项行为与安全控制在改动前后均通过。Harness 还检查请求之间提供程序所有者或可用性的变化、URI 授权拒绝、危险或未知或同 UID 的提供程序、去重以及数量上限。这些数据是控制器调用次数，不是 Android UI 耗时。提交的 Android 测试统计控制器解析提供程序时对 package manager 的访问，并把实际权限判断委托给目标 context。R1-1 的三个实际测试方法还通过 JUnit 在受控 Android API 桩的 JVM 上执行：生产实现全部通过；无 authority 键的单槽缓存和移出循环但不缓存解析的两种变异均通过旧的两项测试，却在新增混合 authority 测试上失败（应为 2 次访问，实际 1 次）。这一变异验证不是 Android instrumentation。
本地测试环境
70256adaa9的 upstream Android CI：API 26 为 22 通过 / 4 预期跳过；API 36 为 25 通过 / 1 预期跳过；两者均通过全部 19 项 file-picker 测试；新增测试提交的 CI 待运行环境
Windows、JDK 17、Gradle 8.2.1、Android SDK 34。Instrumentation 测试已编译打包，但本地尚未在 Android 模拟器或真机上执行。
风险与范围
关联
#12126 的后续工作，关联 #11704、#13111。评审证据：https://github.com/QwenLM/qwen-code/pull/12126#issuecomment-5848429760。