Refactor LCP Module

[stevenzeck]

1. Summary

The readium/lcp module suffers from severe threading anti-patterns, relies on outdated and unmaintained dependencies, duplicates networking logic, and enforces View-bound UI components that aren't easily adaptable by modern Jetpack Compose applications.

This modernization effort will resolve critical concurrency hazards, significantly reduce the binary footprint, improve decryption performance, and improve developer experience for modern Android apps.

---

2. Critical Architectural Fixes (Coroutines & Concurrency)

The most urgent issues reside in how the LCP module manages Coroutines and state, leading to potential deadlocks and memory leaks.

2.1. The runBlocking Deadlock Hazard in LicenseValidation.kt

Currently, the custom StateMachine used for license validation processes state transitions and networking calls inside a runBlocking block:

private fun handle(state: State) {
    try {
        runBlocking {
            when (state) { /* suspending network calls */ }
        }
    }
}

This blocks the underlying thread while waiting for suspending network operations (fetchStatus, fetchLicense). A comment in LicensesService.kt explicitly notes that if this runs on the Main Dispatcher, it causes a severe deadlock with the UI when prompting for a passphrase.

Actions:

• Rip out the custom StateMachine and the runBlocking wrapper.
• Replace it with a native Coroutine solution (e.g., a Channel-based actor or a MutableStateFlow loop) where state transitions and network calls are natively suspend functions. This eliminates the need for dangerous thread-shifting workarounds.

2.2. Coroutine Scope Leak in LicensesService.kt

LicensesService implements CoroutineScope via delegation to MainScope():

internal class LicensesService(...) : LcpService, CoroutineScope by MainScope()

While it launches coroutines within this scope, it never actually cancels it (it lacks a close() or onDestroy() method). If a reading app drops its reference to the LcpService or instantiates a new one, the MainScope will leak indefinitely along with any active background tasks.

Actions:

• Remove the CoroutineScope by MainScope() delegation.
• Use structured concurrency (e.g., coroutineScope { } or supervisorScope { } blocks) within the suspending functions.
• If a persistent scope is strictly necessary, LicensesService must expose a close() function to explicitly call cancel() on the scope when the host app shuts down the service.

---

3. Dependency Cleanups

The module relies on several outdated libraries that bloat the application size and complicate maintenance.

3.1. Remove com.mcxiaoke.koi:core

Context: This library is ancient and unmaintained. It is used exclusively for SHA-256 hashing and hex string conversions (e.g., HASH.sha256(clearPassphrase)).

Actions:

• Remove the dependency.
• Extend readium/lcp/src/main/java/org/readium/r2/lcp/util/Digest.kt to support String and ByteArray using standard java.security.MessageDigest.
• Replace koi hex conversions with Kotlin's standard library ByteArray.toHexString().

3.2. Remove joda-time

Context: joda-time is obsolete and adds unnecessary binary size. It is primarily used in CRLService.kt for calculating certificate expiration.

Actions:

• Remove the dependency.
• Replace org.joda.time.DateTime with org.readium.r2.shared.util.Instant (which Readium uses to wrap kotlinx-datetime), or use standard java.time APIs with desugaring.

---

4. Network Stack Unification

Context: The LCP module implements its own networking via NetworkService.kt, which manually wraps HttpURLConnection and manages BufferedInputStream to download files and make REST calls.

Actions:

• Delete NetworkService.kt.
• Migrate all network calls to the central org.readium.r2.shared.util.http.HttpClient provided by the readium-shared module.
• Implement a file-download extension for HttpClient to replace the custom onProgress stream downloading logic.

---

5. Native JNI & Performance Optimizations

Context: To make the native liblcp binary optional, LcpClient.kt relies on reflection for every JNI call, including the highly repetitive decrypt function. This incurs a constant performance penalty during CBC stream decryption.

Action:

• Define an internal interface LcpNativeProxy.
• Instantiate its reflection-backed implementation once when LcpClient.isAvailable() is called.
• Subsequent method calls should execute via the interface, eliminating the reflection lookup overhead in the critical decryption path.

---

6. UI Modernization & Compose Compatibility

The current UI implementations aren't easily adaptable by apps built entirely in Jetpack Compose, as they hardcode legacy Android View dependencies.

6.1. Abstract LcpDialogAuthentication.kt

Context: This class is tightly coupled to Android XML layouts and PopupWindow. Compose apps must provide a root anchor View just to show a dialog.

Actions:

• Decouple the authentication business logic from the view logic.
• Provide a Compose-friendly LcpAuthenticating implementation that exposes the authentication state as a state stream.

6.2. Abstract MaterialRenewListener.kt

Context: Currently requires a FragmentManager to present a Material Date Picker for license renewal.

Action: Provide a Compose-native alternative that uses Jetpack Compose date pickers, completely eliminating the need for FragmentManager.

---

7. API Modernization

7.1. Flow for License Acquisition

Context: LcpService.acquirePublication() tracks progress via a closure onProgress: (Double) -> Unit.

Actions:

• Introduce an overloaded method that returns a Flow<AcquisitionStatus>.
• AcquisitionStatus should be a sealed class representing Loading, Success, and Failure.

---

8. JSON Parsing

Context: Every single data class in readium/lcp/src/main/java/org/readium/r2/lcp/license/model/ (e.g., LicenseDocument, StatusDocument, Link, Rights, etc.) is manually parsing JSON using org.json.JSONObject. This is prone to runtime crashes, lacks type safety, and results in a lot of boilerplate code.

Actions: Replace all org.json.JSONObject usage with Kotlin's @Serializable. This will improve parsing performance, ensure null-safety at compile-time, and delete boilerplate.

---

9. ZIP Modification

Context: In ContentZipLicenseContainer.kt, there is a performance problem. When the app needs to inject a downloaded LCP license into a file accessed via Android's ContentResolver, it currently copies the entire file to the device's cache directory, modifies the ZIP, and then copies the whole archive back over the ContentResolver stream. If a user downloads a large LCP-protected book, injecting a 2KB license file will cause a massive I/O spike, doubling the storage required temporarily and freezing the app.

Action: Modernize using a streaming ZIP manipulation approach, which allows updating ZIP entries without copying the entire payload to the local cache.

---

10. IDE Warnings

Context: There are a lot of warnings which somehow aren't stopped by allWarningsAsErrors.

Action: Clean up all warnings and weak warnings.

---

11. Tests

Context: There are only 3 files of tests for LCP.

Action: Add more before starting each piece of refactoring.

[qnga]

• I'd be very careful with 2 as this is a very complex logic.
• 3 and 4 sound fine to me.
• I'm not sure about the speed improvements you can achieve with 5, but why not.
• I don't think 6 makes much sense. All the stuff hardcoding uses of Material or Android views is optional.
• 7 is an arguable public API change. To be discussed.
• 8 would be very welcome, as it would be a contribution to the KMP effort.
• 9 makes sense, but I'm not sure there is a trivial solution.

[mickael-menu]

Thank you for opening this discussion, modernizing the LCP module is worth it.

First of all, this code is critical, and we cannot afford any regressions. Before refactoring any significant part of the LCP service (especially LCP License Validation) we must introduce a comprehensive test suite for all LCP interactions and statuses. Unfortunately we cannot commit the LCP test setup to the repository, but we can use a build script and hidden GitHub repository secrets to initialize LCP on the CI. I already implemented this approach in the Swift toolkit. Test LCPL in various statuses can be generated from https://front-test.edrlab.org. Let me know if I can help you out designing this test suite.

For reference, I think that this statechart is up to date:

Now to each of your points. I think we agree with @qnga:

• 2.1 The problem is real, but I think we can simplify the code and avoid Channel or MutableStateFlow, as long as we have good traces and a comprehensive test suite. Something like that (but returning a Try<ValidatedDocument, LcpException>):

    suspend fun validate(data: ByteArray, preloadedStatus: StatusDocument? = null): ValidatedDocuments {
        val license = parseLicense(data)
  
        val status = preloadedStatus ?: tryFetchStatus(license)
  
        val (resolvedLicense, resolvedStatus) = if (status != null && license.updated < status.licenseUpdated) {
            val updatedData = fetchLicense(status)
            parseLicense(updatedData) to status
        } else {
            license to status
        }
  
        val statusError = checkLicenseStatus(resolvedLicense, resolvedStatus, statusDocumentTakesPrecedence = preloadedStatus == null)
        if (statusError != null) {
            return ValidatedDocuments(resolvedLicense, Either.Right(statusError), resolvedStatus)
        }
  
        val passphrase = passphrases.request(resolvedLicense, authentication, allowUserInteraction)
            ?: throw LcpException(LcpError.RequestCancelled)
  
        val lcpContext = validateIntegrity(resolvedLicense, passphrase)
        val documents = ValidatedDocuments(resolvedLicense, Either.Left(lcpContext), resolvedStatus)
  
        val registerLink = resolvedStatus?.link(StatusDocument.Rel.Register) ?: return documents
        val newStatusData = device.registerLicense(resolvedLicense, registerLink)
            ?: return documents
  
        return documents.copy(status = StatusDocument(newStatusData))
    }

• 2.2 Sounds good, I think.

• 3 Yes

• 4 Yes, something I wanted to do for a while.

• 5 I don't think we saw a bottleneck here. But if it's cleaner, why not.

• 6 The existing Android View adapters are fine and should not be removed. But Compose-ready adapters would be welcome. I did something similar in the Swift toolkit by introducing a LCPDialog SwiftUI view. That might expose issues with the current API, regarding passing the host context. See for reference: Add a SwiftUI LCP authentication dialog swift-toolkit#607

• 7 I don't think this is necessarily better. We can talk about it again once the rest is tackled.

• 8 Yes

• 9 Sounds theoretically good, but I have no idea if this is possible and how to do it.

• 10 Warnings are not caught because the app is not compiled with LCP enabled in the CI. My first paragraph would help with that.

[stevenzeck]

Yes, I'll start with adding tests and committing those before refactoring.

2. Tackle this last, as its the most complex.
3. In agreement
4. In agreement
5. TBD. Can do benchmarking to see performance changes.
6. Is this truly optional for the implementer to use? If so, maybe less of a priority. Another TBD.
7. Holding off for now
8. In agreement
9. TBD
10. The app is compiled with LCP, the compiler must not classify the warnings in the IDE the same. For example, LicenseValidation has several warnings about Property with 'Array' type in a 'data' class: it is recommended to override 'equals()' and 'hashCode()', but never reported when compiling. But introduce something like val bundle = bundleOf("TEST" to 1) in LCP, it does fail. Weird. If it's a safe change to get rid of a warning, I'll do it.