JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

OutputPublisher's model of SAF is asserted only against fakes built to match it #226

New Issue
Closed
opened 2026-09-06 02:53:14 +00:00 by JMR-dev · 2 comments
JMR-dev commented 2026-09-06 02:53:14 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Filed from the 2026-09-05 e2e read of the instrumented suite on main @ 4b02294.

OutputPublisher.publish is where the user's file is written. Its safety net turns on a claim about SAF that is asserted only by a fake built to match it.

SafPickerRoundTripTest's KDoc says the position plainly:

the only SAF coverage is the publish side, in OutputPublisherPublishTest, against hand-written ContentProvider fakes

Two parts, with very different costs. They are one ticket because they share a fixture and (a) is most of the work for (b); split them if (b) is deferred.

(a) publish has never met a real DocumentsProvider — cheap, headless, CI-stable

OutputPublisherPublishTest is Robolectric with FakeSafProvider + FakePlainProvider, registered through registerProvider(..., asDocumentsProvider = true). That flag is what makes DocumentsContract.isDocumentUri answer true — so the branch guarding the delete is decided by a test-only registration, not by a provider.

FixtureDocumentsProvider already exists in the instrumented suite and is a real DocumentsProvider behind a real ContentResolver. It is read-only today: root flags are FLAG_LOCAL_ONLY, the root document's flags are 0, and there is no createDocument.

Extending it is small and self-contained:

  • Root.FLAG_SUPPORTS_CREATE on the root
  • Document.FLAG_DIR_SUPPORTS_CREATE on the root document
  • createDocument(parentDocumentId, mimeType, displayName) creating a real zero-byte file
  • openDocument honouring "w"
  • deleteDocument + Document.FLAG_SUPPORTS_DELETE

Then drive publish at a URI from that provider and assert the three behaviours the fakes currently assert: a copy that fails partway deletes the document, a destination that already held bytes is not deleted, and a non-document URI is left alone.

Mind the language constraint. FixtureDocumentsProvider is the module's only Java file on purpose — it runs in a bare org.libremediaconverter.test process with no Kotlin stdlib, and a kotlin.* reference gives NoClassDefFoundError: kotlin/jvm/internal/Intrinsics. Its KDoc says so. Keep the additions Java and androidx-free.

Mutation: drop the DocumentsContract.isDocumentUri guard in deletePartialOutput. Against a real provider the non-document case now deletes; the fake-based test would catch this too, so pair it with one only the real provider can bite — e.g. remove Document.FLAG_SUPPORTS_DELETE and confirm the cleanup path reports rather than silently succeeding.

(b) The premise itself, which nothing has ever observed

app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt:94-96

// SAF's CreateDocument contract hands back a document that already exists and is
// empty, so that is the state every destination starts in here.
FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0))

The test manufactures the precondition. publish's KDoc rests on the same claim and calls it settled:

SAF's CreateDocument contract creates the document before this is called, which is why every fixture in OutputPublisherPublishTest starts as an existing empty file.

destinationIsKnownEmpty reads OpenableColumns.SIZE and authorises the delete only on a positive zero. Its own KDoc is careful that "I could not tell" must never authorise one — which is correct, and is exactly what makes the premise load-bearing: if stock DocumentsUI hands back a document that reports no size, or a non-zero one, destinationIsKnownEmpty returns false, deletePartialOutput never runs, and the fix for the truncated-file defect (D4) is inert in production while every test stays green.

Nothing on any source set has watched real DocumentsUI do this.

This is O3-shaped: record the answer either way

The measurement is small — create a document through the real ACTION_CREATE_DOCUMENT contract against stock DocumentsUI (not the fixture provider), and before writing anything assert:

  1. DocumentsContract.isDocumentUri(context, uri) is true, and
  2. OpenableColumns.SIZE is present and 0.

A "no" is a defect, not a failed test. A written "stock DocumentsUI reports X, so the guard cannot fire, here is the measurement" is the successful outcome if it goes that way, and it changes publish rather than the test.

Cost, honestly: this half needs real DocumentsUI, so it pays #190's flake tax and lands next to the picker test that has already cost #80, #93 and #96. That is the whole reason it is separable from (a) — do (a) regardless; do (b) when someone is willing to own a second system-UI-driving test.

Related

  • docs/defect-audit.md D4 — the truncated-file defect this guard fixed
  • #190, #93 — what driving stock DocumentsUI costs on the gating legs
_Filed from the 2026-09-05 e2e read of the instrumented suite on `main` @ `4b02294`._ `OutputPublisher.publish` is where the user's file is written. Its safety net turns on a claim about SAF that is asserted only by a fake built to match it. `SafPickerRoundTripTest`'s KDoc says the position plainly: > the only SAF coverage is the publish side, in `OutputPublisherPublishTest`, against hand-written `ContentProvider` fakes **Two parts, with very different costs.** They are one ticket because they share a fixture and (a) is most of the work for (b); split them if (b) is deferred. --- ## (a) `publish` has never met a real `DocumentsProvider` — cheap, headless, CI-stable `OutputPublisherPublishTest` is Robolectric with `FakeSafProvider` + `FakePlainProvider`, registered through `registerProvider(..., asDocumentsProvider = true)`. That flag is what makes `DocumentsContract.isDocumentUri` answer true — so the branch guarding the delete is decided by a test-only registration, not by a provider. `FixtureDocumentsProvider` already exists in the instrumented suite and is a **real** `DocumentsProvider` behind a real `ContentResolver`. It is read-only today: root flags are `FLAG_LOCAL_ONLY`, the root document's flags are `0`, and there is no `createDocument`. Extending it is small and self-contained: - `Root.FLAG_SUPPORTS_CREATE` on the root - `Document.FLAG_DIR_SUPPORTS_CREATE` on the root document - `createDocument(parentDocumentId, mimeType, displayName)` creating a real zero-byte file - `openDocument` honouring `"w"` - `deleteDocument` + `Document.FLAG_SUPPORTS_DELETE` Then drive `publish` at a URI from that provider and assert the three behaviours the fakes currently assert: a copy that fails partway deletes the document, a destination that already held bytes is not deleted, and a non-document URI is left alone. **Mind the language constraint.** `FixtureDocumentsProvider` is the module's only Java file *on purpose* — it runs in a bare `org.libremediaconverter.test` process with no Kotlin stdlib, and a `kotlin.*` reference gives `NoClassDefFoundError: kotlin/jvm/internal/Intrinsics`. Its KDoc says so. Keep the additions Java and `androidx`-free. *Mutation:* drop the `DocumentsContract.isDocumentUri` guard in `deletePartialOutput`. Against a real provider the non-document case now deletes; the fake-based test would catch this too, so pair it with one only the real provider can bite — e.g. remove `Document.FLAG_SUPPORTS_DELETE` and confirm the cleanup path reports rather than silently succeeding. --- ## (b) The premise itself, which nothing has ever observed ``` app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt:94-96 ``` ```kotlin // SAF's CreateDocument contract hands back a document that already exists and is // empty, so that is the state every destination starts in here. FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0)) ``` The test **manufactures** the precondition. `publish`'s KDoc rests on the same claim and calls it settled: > SAF's `CreateDocument` contract creates the document *before* this is called, which is why every fixture in `OutputPublisherPublishTest` starts as an existing empty file. `destinationIsKnownEmpty` reads `OpenableColumns.SIZE` and authorises the delete **only** on a positive zero. Its own KDoc is careful that "I could not tell" must never authorise one — which is correct, and is exactly what makes the premise load-bearing: if stock DocumentsUI hands back a document that reports no size, or a non-zero one, `destinationIsKnownEmpty` returns false, `deletePartialOutput` never runs, and **the fix for the truncated-file defect (`D4`) is inert in production** while every test stays green. Nothing on any source set has watched real DocumentsUI do this. ### This is O3-shaped: record the answer either way The measurement is small — create a document through the real `ACTION_CREATE_DOCUMENT` contract against **stock** DocumentsUI (not the fixture provider), and before writing anything assert: 1. `DocumentsContract.isDocumentUri(context, uri)` is true, and 2. `OpenableColumns.SIZE` is present and `0`. **A "no" is a defect, not a failed test.** A written *"stock DocumentsUI reports X, so the guard cannot fire, here is the measurement"* is the successful outcome if it goes that way, and it changes `publish` rather than the test. **Cost, honestly:** this half needs real DocumentsUI, so it pays #190's flake tax and lands next to the picker test that has already cost #80, #93 and #96. That is the whole reason it is separable from (a) — do (a) regardless; do (b) when someone is willing to own a second system-UI-driving test. ## Related - `docs/defect-audit.md` **D4** — the truncated-file defect this guard fixed - #190, #93 — what driving stock DocumentsUI costs on the gating legs
JMR-dev commented 2026-09-06 05:24:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Part (a) cannot be done, and the reason changes this ticket. Measured on a local API 34 emulator, three approaches, all blocked by the same platform rule.

This ticket splits into a cheap headless half (a) and an expensive picker-driven half (b), on the premise that a real DocumentsProvider can be reached without DocumentsUI. It cannot. (a) is not cheaper than (b) — it is (b).

What was tried

1. A second, unprotected DocumentsProvider in the instrumentation APK. FixtureDocumentsProvider is behind MANAGE_DOCUMENTS; the idea was a sibling declaration without it, since nothing needs to pick from it. The platform refuses to install it at all:

java.lang.RuntimeException: Unable to get provider …PublishTargetProvider:
  java.lang.SecurityException: Provider must be protected by MANAGE_DOCUMENTS
    at android.app.ActivityThread.installProvider(ActivityThread.java:7770)

Any provider carrying the DOCUMENTS_PROVIDER intent filter must hold that permission. And the filter is not optional: DocumentsContract.isDocumentUri returns false without it, which is precisely the branch guarding deletePartialOutput — so a provider without the filter tests nothing this ticket is about.

2. Borrowing the test APK's identity. The instrumentation APK owns the provider, so Instrumentation.getContext() looked like a way in. It is not — instrumentation runs in the target app's process, so test code carries the app's uid however the Context was obtained:

java.lang.SecurityException: Permission Denial: opening provider …FixtureDocumentsProvider
  from ProcessRecord{… org.libremediaconverter/u0a192} (pid=4145, uid=10192)
  requires that you obtain access using ACTION_OPEN_DOCUMENT or related APIs

3. uiAutomation.adoptShellPermissionIdentity("android.permission.MANAGE_DOCUMENTS"). Same denial, verbatim. The check is not "do you hold MANAGE_DOCUMENTS" but "do you hold a URI grant from the picker", and shell identity does not satisfy it.

What that means

The message in 2 and 3 is the answer: a documents provider is reachable only through a grant issued by ACTION_OPEN_DOCUMENT / ACTION_CREATE_DOCUMENT. So any test of publish against a real DocumentsProvider must drive DocumentsUI, and pays #190's flake tax.

The work is therefore one item, not two, and its cost is (b)'s. The natural home is SafPickerRoundTripTest, which already drives DocumentsUI, already owns the retry and readable-screen machinery that took #80, #93 and #96 to get right, and currently stops at Ready — the same class could carry a CREATE_DOCUMENT round trip.

(b)'s question is unchanged and still worth answering, and it is now the whole ticket: does stock DocumentsUI hand back a pre-created, positively-zero-byte document? OutputPublisherPublishTest.kt:94-96 manufactures that precondition under a comment asserting it is how SAF behaves, and destinationIsKnownEmpty → deletePartialOutput — D4's fix — is inert in production if it is false.

Not done, deliberately

I wrote the provider extension (createDocument, "w" mode, deleteDocument, a refusing document, recorded deletes) and reverted it. It is dead code until something can reach it, and this repo does not keep speculative test scaffolding. The shape is in this comment if the picker-driven version is picked up.

**Part (a) cannot be done, and the reason changes this ticket.** Measured on a local API 34 emulator, three approaches, all blocked by the same platform rule. This ticket splits into a cheap headless half (a) and an expensive picker-driven half (b), on the premise that a real `DocumentsProvider` can be reached without DocumentsUI. **It cannot.** (a) is not cheaper than (b) — it *is* (b). ## What was tried **1. A second, unprotected `DocumentsProvider` in the instrumentation APK.** `FixtureDocumentsProvider` is behind `MANAGE_DOCUMENTS`; the idea was a sibling declaration without it, since nothing needs to *pick* from it. The platform refuses to install it at all: ``` java.lang.RuntimeException: Unable to get provider …PublishTargetProvider: java.lang.SecurityException: Provider must be protected by MANAGE_DOCUMENTS at android.app.ActivityThread.installProvider(ActivityThread.java:7770) ``` Any provider carrying the `DOCUMENTS_PROVIDER` intent filter **must** hold that permission. And the filter is not optional: `DocumentsContract.isDocumentUri` returns false without it, which is precisely the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing this ticket is about. **2. Borrowing the test APK's identity.** The instrumentation APK *owns* the provider, so `Instrumentation.getContext()` looked like a way in. It is not — **instrumentation runs in the target app's process**, so test code carries the app's uid however the `Context` was obtained: ``` java.lang.SecurityException: Permission Denial: opening provider …FixtureDocumentsProvider from ProcessRecord{… org.libremediaconverter/u0a192} (pid=4145, uid=10192) requires that you obtain access using ACTION_OPEN_DOCUMENT or related APIs ``` **3. `uiAutomation.adoptShellPermissionIdentity("android.permission.MANAGE_DOCUMENTS")`.** Same denial, verbatim. The check is not "do you hold `MANAGE_DOCUMENTS`" but "do you hold a URI grant from the picker", and shell identity does not satisfy it. ## What that means The message in 2 and 3 is the answer: **a documents provider is reachable only through a grant issued by `ACTION_OPEN_DOCUMENT` / `ACTION_CREATE_DOCUMENT`.** So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI, and pays #190's flake tax. The work is therefore one item, not two, and its cost is (b)'s. The natural home is `SafPickerRoundTripTest`, which already drives DocumentsUI, already owns the retry and readable-screen machinery that took #80, #93 and #96 to get right, and currently stops at `Ready` — the same class could carry a `CREATE_DOCUMENT` round trip. **(b)'s question is unchanged and still worth answering**, and it is now the whole ticket: does stock DocumentsUI hand back a pre-created, positively-zero-byte document? `OutputPublisherPublishTest.kt:94-96` manufactures that precondition under a comment asserting it is how SAF behaves, and `destinationIsKnownEmpty` → `deletePartialOutput` — D4's fix — is inert in production if it is false. ## Not done, deliberately I wrote the provider extension (`createDocument`, `"w"` mode, `deleteDocument`, a refusing document, recorded deletes) and reverted it. It is dead code until something can reach it, and this repo does not keep speculative test scaffolding. The shape is in this comment if the picker-driven version is picked up.
JMR-dev commented 2026-09-06 14:02:56 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Re-checked against main at b3d4318, with #245 merged. The finding stands; the cost argument that came with it does not.

E7 is untouched

#245 changes zero lines in app/src/androidTest/AndroidManifest.xml, FixtureDocumentsProvider.java or FixtureContentProvider.java. E7 is a platform rule — any provider carrying the DOCUMENTS_PROVIDER filter must hold MANAGE_DOCUMENTS, and instrumentation runs in the target app's process so nothing can borrow the test APK's identity. Nothing in #245 bears on it.

So this is still one picker-driven item, not two. There is no cheap headless half.

What changed is the price, which is what I actually declined on

I deferred this on cost, and #245 removes most of it:

  • Both SafPickerRoundTripTest methods now carry @FailsOnEmulatorApi37 (:329, :350), so the picker class is entirely off the API 37 gating leg. My specific objection — that a third picker-driven test would make the gating aborts worse — is now false. A test added here carries the marker too and cannot touch gating.
  • forceStopThePicker makes the whole-picker retry reachable, where UiObject2.click() could not see a tap that reached no window. That is reliability on API 33–36, which is where a test here would actually gate.
  • The marker's KDoc was widened to cover "passes about half the time and aborts system_server every time", which makes marking a new DocumentsUI-driving test the documented expectation rather than a special case.

And a correction to what I put on #108

I wrote there that the abort "lands on a different test each time" and that the two victims shared no cause beyond timing. Execution order says otherwise — saf runs before work:

saf.SafPickerRoundTripTest → work.ConversionWorkerTest → work.NotificationCancelActionTest

NotificationCancelActionTest fires a PendingIntent through system_server, so my two sightings of it dying are most likely the picker test's abort surfacing on the next test that needs system_server. One cause, not two — which is what #245's four-run logcat read already argued, and my comment overstated the variety.

Remaining cost, stated plainly

One more marker (baseline 5 → 6), so CI never verifies this at API 37 — the same position the picker test is now in, and the Pixel check is what covers it. Against that: the open question is unchanged and still worth answering, because if stock DocumentsUI does not hand back a pre-created, positively-zero-byte document, then destinationIsKnownEmpty never returns true, deletePartialOutput never runs, and D4's fix is inert in production while every test stays green.

Taking it now.

**Re-checked against `main` at `b3d4318`, with #245 merged. The finding stands; the cost argument that came with it does not.** ## E7 is untouched #245 changes **zero lines** in `app/src/androidTest/AndroidManifest.xml`, `FixtureDocumentsProvider.java` or `FixtureContentProvider.java`. E7 is a platform rule — any provider carrying the `DOCUMENTS_PROVIDER` filter must hold `MANAGE_DOCUMENTS`, and instrumentation runs in the target app's process so nothing can borrow the test APK's identity. Nothing in #245 bears on it. So this is still **one picker-driven item, not two**. There is no cheap headless half. ## What changed is the price, which is what I actually declined on I deferred this on cost, and #245 removes most of it: - **Both** `SafPickerRoundTripTest` methods now carry `@FailsOnEmulatorApi37` (`:329`, `:350`), so the picker class is entirely off the API 37 gating leg. My specific objection — that a third picker-driven test would make the gating aborts worse — is now false. A test added here carries the marker too and cannot touch gating. - `forceStopThePicker` makes the whole-picker retry reachable, where `UiObject2.click()` could not see a tap that reached no window. That is reliability on API 33–36, which is where a test here would actually gate. - The marker's KDoc was widened to cover "passes about half the time and aborts `system_server` every time", which makes marking a new DocumentsUI-driving test the documented expectation rather than a special case. ## And a correction to what I put on #108 I wrote there that the abort "lands on a different test each time" and that the two victims shared no cause beyond timing. Execution order says otherwise — `saf` runs before `work`: ``` saf.SafPickerRoundTripTest → work.ConversionWorkerTest → work.NotificationCancelActionTest ``` `NotificationCancelActionTest` fires a `PendingIntent` through `system_server`, so my two sightings of *it* dying are most likely the picker test's abort surfacing on the next test that needs `system_server`. **One cause, not two** — which is what #245's four-run logcat read already argued, and my comment overstated the variety. ## Remaining cost, stated plainly One more marker (baseline 5 → 6), so CI never verifies this at API 37 — the same position the picker test is now in, and the Pixel check is what covers it. Against that: the open question is unchanged and still worth answering, because if stock DocumentsUI does **not** hand back a pre-created, positively-zero-byte document, then `destinationIsKnownEmpty` never returns true, `deletePartialOutput` never runs, and D4's fix is inert in production while every test stays green. Taking it now.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels bug sev:medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#226