Status Update
Comments
mm...@google.com <mm...@google.com>
je...@google.com <je...@google.com> #2
Ivan, since org.json is part of the platform APIs, should we automatically exclude it from the resolved dependencies ?
ga...@google.com <ga...@google.com> #3
Thanks for filing the issue and for providing lots of info.
I was trying to follow instructions from #1 to reproduce, and this is what I came up with:
package com.example.myapplication
import androidx.appcompat.app.AppCompatActivity
import android.os.Bundle
import org.json.JSONObject
class MainActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContentView(R.layout.activity_main)
val s = JSONObject("{'username': null}")
println("USERNAME = " + s.getString("username"))
println("JSON Object class loader is " + JSONObject::class.java.classLoader)
}
}
With and without implementation("com.pubnub:pubnub-kotlin:7.4.2")
I'm seeing:
System.out com.example.myapplication I USERNAME = null
System.out com.example.myapplication I JSON Object class loader is java.lang.BootClassLoader@ae0b505
in logcat (running on API 33 emulator).
This is also what I'd expect to see as BootClassLoader
is the application's parent classloader, so it should be providing JSONObject
.
Can you maybe share a snippet that reproduces the issue? Also, do you see issues in both debug and release variant?
me...@justeattakeaway.com <me...@justeattakeaway.com> #4
Thanks for this. I am trying to reproduce this issue on a minimal build and am actually seeing the same as what you're seeing in both debug
and release
variants.
I've attached a screenshot of
Non-fatal Exception: org.json.JSONException: JSONObject["payment_takeawayPayBalance"] is not a string (class org.json.b$b : null).
at org.json.JSONObject.wrongValueFormatException(JSONObject.java:2702)
at org.json.JSONObject.getString(JSONObject.java:861)
at com.justeat.webcheckout.common.AnalyticsWebInterface.logEvent(AnalyticsWebInterface.java:38)
at android.os.MessageQueue.nativePollOnce(MessageQueue.java)
at android.os.MessageQueue.next(MessageQueue.java:362)
at android.os.Looper.loop(Looper.java:185)
at android.os.HandlerThread.run(HandlerThread.java:67)
org.json.JSONObject.wrongValueFormatException(JSONObject.java:2702)
simply doesn't exist in Android so it must have come from the external org.json
library.
Can you maybe share a snippet that reproduces the issue?
I will try and strip back as much as possible from our production app enough to reproduce the issue, sanitise the code, then post it in a repo somewhere accessible. Apologies, that may take a while.
Also, do you see issues in both debug and release variant?
Interestingly in release
builds only. A few of our engineers tried reproducing with no success on debug
builds but we were still seeing reports in Firebase from our release
.
me...@justeattakeaway.com <me...@justeattakeaway.com> #5
I was able to reproduce this on a modified version of our own app. Interestingly never in debug
mode. But the release
variant is when I'm able to reproduce.
Unfortunately this is using a complete build of our app which I don't think I'm allowed to share. Allow me to slowly remove all the sensitive and unnecessary code and see if I can still reproduce. For now hopefully this screenshot shows you can we're not loading the Android platform version anymore.
me...@justeattakeaway.com <me...@justeattakeaway.com> #6
Hello again
I've been able to reproduce this in a minimal repo that you can play around with:
Let me know what you think and if you need anything else from me.
me...@justeattakeaway.com <me...@justeattakeaway.com> #7
Hi there. I just wanted to check if you've had a chance to look at the example repo and this issue again?
Many thanks.
ga...@google.com <ga...@google.com> #8
Sorry for the delay, and thanks for creating a small project to reproduce, I am now able to see the same issue.
The problem is that R8 will rename json classes:
org.json.JSONArray -> k6.a:
java.util.ArrayList myArrayList -> o
1:2:void <init>():74:75 -> <init>
so this forces the app the use classes packaged with it. Because of classloader, it would make sense to R8 to prefer classes coming from the platform.
This also reproduces with the latest R8 in AGP 8.2.0-alpha10.
me...@justeattakeaway.com <me...@justeattakeaway.com> #9
Glad you got to the bottom of it!
I don't quite understand the R8 explanation would you mind expanding a little?
The problem is that R8 will rename json classes so this forces the app the use classes packaged with it.
Are we saying that R8 is renaming both the JSON classes from the platform and the external json library to the same symbol (let's say k6.a
for arguments sake)? So that when the renamed reference gets called by the app it picks k6.a
from the external library instead of the one from the platform? Something like that? If so, how is it possible to rename two things to the same name? Wouldn't there be an error at the point of renaming saying that this name already exists?
Apologies if I've misunderstood. R8 is magic to me.
ga...@google.com <ga...@google.com> #10
Let me expand my explanation a bit:
R8 never changes the platform classes, it just analyzes them but because these are not packaged in the APK it won't ever try to rename them.
The issue however is that if there is an Android platform API e.g. Foo
and you also have Foo
in your program code (either directly as source or as a Maven library), R8 may rename Foo
to something else, as it currently assumes that you want to use Foo
from e.g. a Maven library.
me...@justeattakeaway.com <me...@justeattakeaway.com> #11
Ahhhh OK. I see. That makes sense to me.
Given that there might be a high possibility of collisions if just the class name is taken into account I assume R8 is looking at the fully qualified name of the class: my.awesome.package.Foo
rather than Foo
?
The reason this issue exists is because we have stumbled upon a scenario where the fully qualified class name from program code (or a Maven lib) happens to be the same as from the platform. The platform version is unchanged but the program code (or Maven lib) version gets changed my.awesome.package.Foo
--> a.b.L.4
as well as references to it and bish bash bosh we're calling the library version of org.json
!
Awesome, cheers for that. Have a good weekend!
ga...@google.com <ga...@google.com> #12
Correct, R8 always uses fully qualified class names, I just used Foo
to simplify. Here the issue is that org.json.JSONArray
exists both in the Android platform and the Maven lib that the project depends on.
Ian, can you please take a look at this? Feel free to reassign back if this is expected R8 behavior.
ze...@google.com <ze...@google.com> #13
Thanks for diving into this Ivan, your conclusion is correct. The issue is the duplicate definitions of classes in the program and in the bootclasspath. There is not much R8 can do to fix the issue.
-
If the version on bootclasspath is usable, then excluding the duplicate classes from the program and its dependencies is the right way to resolve it. This is the by far the best way to solve it if possible.
-
If the version in the program should be used, then it is best to repackage all of the classes in the program so that there is no overlap with those on bootclasspath. Doing so requires doing a transformation of the program prior to running R8 so that when R8 runs
org.json.*
in the program is already moved tomyapp.org.json.*
. Others have done this withjarjar
, orshadowjar
or similar tools. This is a bit complicated but reliable. -
As a last available hack, you can try adding a keep rule for all of the classes that are duplicated. In this case something like:
-keep org.json.** { *; }
. With that rule, R8 will not optimize and rename the classes and you can hope that the version on bootclasspath is consistent with what you are shipping. But it really is a bad fix and subtle issues can arise if your program ends up using a class not in the bootclasspath as it will cause the runtime to use both versions.
me...@justeattakeaway.com <me...@justeattakeaway.com> #14
Thanks for the update.
There is not much R8 can do to fix the issue.
Can R8 print a warning or something to at least highlight this? It's really unintuitive as to what is happening to the casual user. For example we had to dig pretty deep into two implementations of org.json
to figure out why we were seeing what were seeing (divergent behaviour for what we thought was the same code). It's made worse by the fact that the org.json
maintainers don't feel that they have to
A build time warning (or even better a breaking error at build time) with a suggestion to option 1 (and
ze...@google.com <ze...@google.com> #15
R8 prints a warning in one particularly nasty case "library class extends program class" which arises only when there is a hole in the type hierarchy.
I'm not sure if we could be more aggressive with warnings. Right now the compiler does not have the full view of such duplicate definitions. Maybe the build system could identify when these duplicates are present?
Back you Ivan for input on that.
ze...@google.com <ze...@google.com> #16
As and FYI, I think the json project has the right stance here. It really is an issue with the choice by Android to ship the library in bootclasspath. That means users are never really sure what version they have. This is an issue with a few libraries that really should not have been mixed in with the normal core Java libraries IMO. Unfortunately that is the case, so the choices are pretty much as detailed in
ga...@google.com <ga...@google.com> #17
In AGP there is a task that com.foo.Bar
found. This task currently checks for duplicates only in external (Maven) libraries, but it makes sense to include android.jar
in this check.
Hung, can you please look into this?
hu...@google.com <hu...@google.com> #18
Yes, I can look at adding that check in AGP.
At the same time, I think preserving the behavior across debug vs. release builds is also important, so I filed android.jar
.
Description
I fully admit this might not be the right place for this but I didn't know where would be. Feel free to move it or tell me where is best to raise this. It's also more of a question at this stage since I really don't know what the expectation should be yet, maybe I've just misunderstood things :shrug:.
Our app relies on some
org.json
classes that from the Android platform and in particularJSONObject::getString
. After a recent (unrelated) library upgrade we noticed that allorg.json
classes in a built application were being served from an external dependency, specifically:org.json:json:20230227
. We're used to transitive dependencies that are higher in version than declared dependencies taking precedence so this doesn't surprise me. However what was troubling was that the behaviour ofJSONObject::getString
from the Android platform and fromorg.json:json:20230227
was different.This caused crashes in our application that were pretty difficult to diagnose and debug. More so because when we looked at the code in Android Studio it would always link to the Android platform version which threw us off whilst debugging to begin with. However the stack trace in Firebase confirmed that it was definitely not using the Android platform version. Specifically because of a reference to the method
JSONObject::wrongValueFormatException
that doesn't exist in the Android platform but does inorg.json:json:20230227
.It essentially boils down to the handling of
null
values in the Android platform version and theorg.json:json:20230227
version inJSONObject::getString
.In both of the first lines (get(name) and this.get(key)) are functionally equivalent and are simple get operations on a Map. These both return
JSONObject::NULL
for the key"foo"
that looks like this in serialised JSON:{"foo": null}
.In the
org.json
library implementationobject instanceof String
is obviouslyfalse
. So we have nowhere left to go by throw and Exception. Which is what we saw in Firebase after our release.In the Android platform implementation we do something funky first:
JSON.toString(object);
. This is from the Android platform and looks like:At this point it is worth describing what
JSONObject::NULL
looks like: Theorg.json
library version is ever so slightly different but for this explanation that doesn't matter. Here's the Android platform version.In the Android platform version of
JSONObject::getString
we then callJSON::toString
on thisNULL
object.if (value != null)
returnstrue
because the operators==
or!=
in Java are reference checks as opposed to.equals
equality checks. We go on to returnString::valueOf(value)
.JSONObject::NULL::toString()
returns the String"null"
. However now with the transitive dependency we don't ever get to this stage and we see a crash.The workaround seems to be to exclude the
org.json
library via Gradle to force the Android platform version to be used.Whilst this works pretty well so far I can't help but wonder if that is the correct thing to do or if Android Studio should be warning us about this ahead of time. At the very least when clicking through our code with
org.json:json:20230227
as a transitive dependency I would expect Android Studio to link through to this library version rather than the Android platform source code which is what it is doing.STEPS TO REPRODUCE:
org.json
class.null
value and callgetString
fromJSONObject
.implementation 'com.pubnub:pubnub-kotlin:7.4.2'
which will pullorg.json:json:20230227
transitively.null
value and callgetString
fromJSONObject
.The behavior changes simply by including the new transitive dependency.
As I said at the beginning, I'm not questioning transitive dependencies overriding the version of declared dependencies, I understand that behaviour. I'm questioning whether:
Feel free to move this elsewhere if appropriate. It would also be good to document this _gotcha _ somewhere.
Studio Build: Android Studio Hedgehog | 2023.1.1 Canary 4 Version of Gradle Plugin: 7.4.2 Version of Gradle: 7.5.1 Version of Java: 17 OS: Linux