Fix crash when JSONObject is backed by immutable Kotlin map

[laurisvr]

When building with R8 (minification enabled), toJSON() methods in Locator.Locations, Locator.Metadata, Metadata, and Properties crash with:

java.lang.UnsupportedOperationException: Operation is not supported for read-only collection
    at kotlin.collections.EmptyMap.remove(Maps.kt)
    at org.json.JSONObject.remove(JSONObject.java)
    at org.readium.r2.shared.extensions.JSONKt.putIfNotEmpty(JSON.kt)
    at org.readium.r2.shared.publication.Locator$Locations.toJSON(Locator.kt)

The issue is that these classes have otherLocations/otherMetadata/otherProperties fields that default to emptyMap(), which is Kotlin's immutable empty map singleton. When this gets passed to the JSONObject(Map) constructor, R8 can optimize away the internal copy into a LinkedHashMap (since the map is empty, the copy loop is a no-op and R8 eliminates it). This leaves the JSONObject backed by the immutable EmptyMap, and then any call to putIfNotEmpty that triggers remove() crashes.

The fix just wraps the map in HashMap() before passing it to the JSONObject constructor so it's always mutable.

Hit this in my app targeting API 36 with AGP 8.13 / R8 full mode, on the notifyCurrentLocation flow in EpubNavigatorFragment.

[qnga]

Isn't this a R8 bug? It should not use an immutable collection where a mutable one is expected and is actually mutated.

That being said, I have nothing against a workaround.

[laurisvr]

You're right that R8 is being overly aggressive here it sees the JSONObject(Map) constructor iterating over an empty map and eliminates the copy as a no-op, not realizing the important part is the assignment to a new mutable LinkedHashMap, not the iteration itself. So yes, arguably an R8 bug.

That said, there's a compounding factor: the bundled org.json:json:20090211 (which Readium pulls in transitively) has a JSONObject(Map) constructor that doesn't copy the map at all it just stores the reference directly. So with that version on the classpath, the crash can happen even without R8. In my case I worked around that separately by excluding the bundled org.json, but the combination makes the pattern fragile from two directions.

Either way, wrapping in HashMap() is a one-line defensive fix that makes it correct regardless of which org.json implementation is resolved and regardless of R8 optimization level.

[qnga]

org.json:json:20090211 (which Readium pulls in transitively) has a JSONObject(Map)

So I guess it casts the Map to a MutableMap to be able to put new keys into it?

In my case I worked around that separately by excluding the bundled org.json

What do you use instead? As I guess you're being using just another version, have you figured out which dependency pulls transitively the 20090211 version?

[qnga]

By the way, given your explanation, I agree on the need for a fix. Would you mind using toMutableMap instead of wrapping the map into a HashMap? I think it'll be a bit clearer. And a comment on one of the uses explaining that some versions of JSONObject require the argument to be mutable despite the signature.

[laurisvr]

The 20090211 version stores the map reference directly in its internal nameValuePairs field without copying. So when emptyMap() (Kotlin's immutable singleton) is passed in, put() calls on the JSONObject throw UnsupportedOperationException.

I exclude it globally with configurations.all { exclude(group = "org.json", module = "json") } and rely on Android's framework org.json instead, which does copy into a new LinkedHashMap. It comes in transitively through Readium. I didn't track down the exact intermediate dependency.

[laurisvr]

Sure, toMutableMap() reads better. I'll update the PR.

[Copilot]

Pull request overview

This PR prevents toJSON() crashes under R8/minification when JSONObject(Map) ends up backed by Kotlin’s immutable emptyMap() (leading to UnsupportedOperationException when remove() is invoked by putIfNotEmpty).

Changes:

• Wrap otherProperties/otherMetadata/otherLocations with toMutableMap() before passing to JSONObject(Map) to guarantee a mutable backing map.
• Add an explanatory comment in Locator.Locations.toJSON() about the mutability requirement.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
readium/shared/src/main/java/org/readium/r2/shared/publication/Properties.kt	Ensure Properties.toJSON() always builds JSONObject from a mutable map.
readium/shared/src/main/java/org/readium/r2/shared/publication/Metadata.kt	Ensure Metadata.toJSON() starts from a mutable map before putIfNotEmpty calls.
readium/shared/src/main/java/org/readium/r2/shared/publication/Locator.kt	Ensure locator-related toJSON() methods start from mutable maps; add context comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[qnga]

Thanks