RuedigerMoeller / fast-serialization

FST: fast java serialization drop in-replacement
Apache License 2.0
1.59k stars 246 forks source link

Android N: NoSuchMethodException Crash, caused by OpenJDK? #121

Closed Wavesonics closed 8 years ago

Wavesonics commented 8 years ago

I'm not sure this is due to OpenJDK, but here's what I've found.

The Android N preview now uses OpenJDK, and when running my app using FST on the emulator using the latest N preview system image, I get the following crash:

Caused by: java.lang.NoSuchMethodException: newInstance [class java.lang.Class, long]
  at java.lang.Class.getMethod(Class.java:2016)
  at java.lang.Class.getDeclaredMethod(Class.java:1995)
  at org.objenesis.instantiator.android.Android18Instantiator.getNewInstanceMethod(Android18Instantiator.java:53)
  at org.objenesis.instantiator.android.Android18Instantiator.<init>(Android18Instantiator.java:38) 
  at org.objenesis.strategy.StdInstantiatorStrategy.newInstantiatorOf(StdInstantiatorStrategy.java:86) 
  at org.objenesis.ObjenesisBase.getInstantiatorOf(ObjenesisBase.java:91) 
  at org.nustaq.serialization.FSTObjenesisInstantiator.<init>(FSTObjenesisInstantiator.java:38) 
  at org.nustaq.serialization.FSTConfiguration$4.getInstantiator(FSTConfiguration.java:372) 
  at org.nustaq.serialization.FSTClazzInfo.<init>(FSTClazzInfo.java:129) 
  at org.nustaq.serialization.FSTClazzInfoRegistry.getCLInfo(FSTClazzInfoRegistry.java:130) 
  at org.nustaq.serialization.FSTClazzNameRegistry.addClassMapping(FSTClazzNameRegistry.java:98) 
  at org.nustaq.serialization.FSTClazzNameRegistry.registerClassNoLookup(FSTClazzNameRegistry.java:85) 
  at org.nustaq.serialization.FSTClazzNameRegistry.registerClass(FSTClazzNameRegistry.java:81) 
  at org.nustaq.serialization.FSTConfiguration.addDefaultClazzes(FSTConfiguration.java:773) 
  at org.nustaq.serialization.FSTConfiguration.initDefaultFstConfigurationInternal(FSTConfiguration.java:445) 
  at org.nustaq.serialization.FSTConfiguration.createAndroidDefaultConfiguration(FSTConfiguration.java:375) 
  at org.nustaq.serialization.FSTConfiguration.createDefaultConfiguration(FSTConfiguration.java:438) 
  at org.nustaq.serialization.FSTConfiguration.createDefaultConfiguration(FSTConfiguration.java:433) 
  at org.nustaq.serialization.FSTConfiguration.getDefaultConfiguration(FSTConfiguration.java:185) 
  at org.nustaq.serialization.FSTObjectOutput.<init>(FSTObjectOutput.java:74) 
  at ...
RuedigerMoeller commented 8 years ago

fst detects android and uses objenesis library to instantiate classes then. Once android uses openjdk, this is not required anymore and one could use the default instantiator.

to fix this, openjdk on android must be detected and the default jdk instantiator should get used. I'd say this should be fixed in objenesis, however it could be workarounded in fst as well.

Wavesonics commented 8 years ago

Looks like they may have fixed this in the latest version if objenesis: https://github.com/easymock/objenesis/releases/tag/2.2

New SearchWorkingInstantiator allowing to identify which instantiator can work on a platform

So we should just need to update to 2.2?

RuedigerMoeller commented 8 years ago

try it, I don't use fst myself on android :). If it works I'll change dependencies and release 2.46

Wavesonics commented 8 years ago

Hhmmm so I tried this:

    compile('de.ruedigermoeller:fst:2.45') {
        exclude group: 'org.objenesis', module: 'objenesis'
    }
    compile 'org.objenesis:objenesis:2.2'

And still get the same crash, so looks like maybe it is not fixed up stream.

Wavesonics commented 8 years ago

Actually looking deeper at the FST gradle file, there are a lot of explicit references to 2.1, so my hack may not have worked.

RuedigerMoeller commented 8 years ago

I am using maven for the standard build. Can you give me a snipped of code which reliably detects OpenJDK running on Android ? Currently there is an Android check only, but for the future I need to 1st detect Android in order to use an Android-Instantiation Strategy + Configuration, then I need to detect wether its the OpenJDK Android internally moving to native FST Object Instantiation strategy.

Wavesonics commented 8 years ago

Android API 24 and above will be based on OpenJDK. However if you need a way to detect it without compiling against Android symbols, I'm not sure.

It did look like Objenesis atleast tried to solve this: https://github.com/easymock/objenesis/issues/34

RuedigerMoeller commented 8 years ago

should be possible using system properties like "os.name" etc. could you dump and post system properties with openjdk/android n ?

Wavesonics commented 8 years ago

Ok so os.name isn't helpful:

os.name: 'Linux'

Here are some values that you could use to detect Android in general:

java.vm.name: 'Dalvik' java.vm.specification.name: 'Dalvik Virtual Machine Specification' java.vendor: 'The Android Project' java.runtime.name: 'Android Runtime'

Even ART runtimes report as Dalvik. So no help there.

The VM Version reports as the same value between 6.0 (JDK) and 6.1 (OpenJDK)

java.vm.version: '2.1.0'

Even the library reports the same

java.specification.name: 'Dalvik Core Library'

So tl;dr: I haven't found anything you can switch on yet.

However, could you use reflection to see if the OpenJDK instantiate method exists?

Wavesonics commented 8 years ago

Ok idk how I feel about this, but I dumped all the system properties and think I may have found one to switch on: java.boot.class.path contains a new jar file which is the custom OpenJDK that they compiled: core-oj.jar or more completely /system/framework/core-oj.jar

I confirmed this does not exist on previous versions of Android, and even found the check-in where this was added which confirms it is what contains the OpenJDK classes.

The full value of java.boot.class.path is as such: /system/framework/core-oj.jar:/system/framework/core-libart.jar:/system/framework/conscrypt.jar:/system/framework/okhttp.jar:/system/framework/core-junit.jar:/system/framework/bouncycastle.jar:/system/framework/ext.jar:/system/framework/framework.jar:/system/framework/telephony-common.jar:/system/framework/voip-common.jar:/system/framework/ims-common.jar:/system/framework/apache-xml.jar:/system/framework/org.apache.http.legacy.boot.jar

If you do switch based on this I recommend just using the jar file name and not it's full path as OEMs have been known to mess with things like that.

Edit: Since this is an Android specific way of detecting the OpenJDK, I would probably pair a check for that jar along with a check of java.runtime.name == 'Android Runtime' or something...

yuriymyronovych commented 8 years ago

Hi guys, can you help me out on this as from this thread I am still not clear how would you fix this one. I assume I have to do hacks myself?

Thanks, Y

yuriymyronovych commented 8 years ago

Assuming I have to do fix myself, Wavesonics, any chance you might share code you used to fix this one?

yuriymyronovych commented 8 years ago

RuedigerMoeller, in your replies you kind of implied you are planning to fix it in 2.46, is this still the plan?

RuedigerMoeller commented 8 years ago

With Android N, fst actually does not need to make use of Objenesis.

in FSTConfiguration.java

protected static FSTConfiguration createAndroidDefaultConfiguration(ConcurrentHashMap<FieldKey,FSTClazzInfo.FSTFieldInfo> shared) {
        final Objenesis genesis = new ObjenesisStd();
        FSTConfiguration conf = new FSTConfiguration(shared) {
            @Override
            public FSTClassInstantiator getInstantiator(Class clazz) {
                return new FSTObjenesisInstantiator(genesis,clazz); // <= this is not required for ANDROID N
            }
        };
[...]

so in case Android N is detected (System props or so, see above) you can do plain conf = new FSTConfiguration(shared);.

Wavesonics commented 8 years ago

OK I spent about 30 seconds trying to do this in the library it's self, but apparently the version I forked doesn't contain the createAndroidDefaultConfiguration method for some reason? Not sure, but here is the Android N/OpenJDK detection code, I tested this on a few emulators and it works properly:

    public static boolean hasOpenJdk()
    {
        final String classPath = System.getProperty( "java.boot.class.path" );
        return classPath.toLowerCase().contains( "core-oj.jar" );
    }

In order to work around the fact that I didn't do this in the library it's self, I rolled that into a larger class as such:

package org.nustaq.serialization;

import org.nustaq.serialization.serializers.FSTMapSerializer;

import java.util.concurrent.ConcurrentHashMap;

public class AndroidFSTConfiguration extends FSTConfiguration
{
    protected AndroidFSTConfiguration( final ConcurrentHashMap<FieldKey, FSTClazzInfo.FSTFieldInfo> sharedFieldInfos )
    {
        super( sharedFieldInfos );
    }

    public static FSTConfiguration create( final ConcurrentHashMap<FieldKey, FSTClazzInfo.FSTFieldInfo> sharedFieldInfos )
    {
        final FSTConfiguration conf;
        if( hasOpenJdk() )
        {
            conf = new FSTConfiguration( sharedFieldInfos );
            initDefaultFstConfigurationInternal(conf);
            if ( isAndroid ) {
                try {
                    conf.registerSerializer( Class.forName("com.google.gson.internal.LinkedTreeMap"), new FSTMapSerializer(), true);
                } catch (ClassNotFoundException e) {
                    //silent
                }
                try {
                    conf.registerSerializer(Class.forName("com.google.gson.internal.LinkedHashTreeMap"), new FSTMapSerializer(), true);
                } catch (ClassNotFoundException e) {
                    //silent
                }
            }
        }
        else
        {
            conf = FSTConfiguration.createAndroidDefaultConfiguration( sharedFieldInfos );
        }

        return conf;
    }

    public static boolean hasOpenJdk()
    {
        final String classPath = System.getProperty( "java.boot.class.path" );
        return classPath.toLowerCase().contains( "core-oj.jar" );
    }
}

And I use that to create a config:

private static final FSTConfiguration s_fst = AndroidFSTConfiguration.create( null );

Which I in turn use create my FST Streams:

FSTObjectOutput oos = s_fst.getObjectOutput( outputStream );

And that all works, no more crashing on Android N.

RuedigerMoeller commented 8 years ago

Good!

However this won't work when using a fst server and you have mixed OpenJDK / Dalvik clients. but i will incorporate the "idea" of your fix into fst 2.46

yuriymyronovych commented 8 years ago

Thank you guys, that is very helpful.

narayank commented 8 years ago

Android Maintainer here : I was about to contact you folks about this.

(1) Please use sun.misc.Unsafe.allocateInstance on OpenJDK based Androids. That's what the Sun instantiator for objenesis will do (eventually) but not before trying many other options that aren't available on Android.

(2) I'd strongly recommend against looking at the boot classpath system property. The safest way to get information about Android devices is by using the android.os.Build class. usesOpenJdk = (android.os.Build.VERSION.SDK_INT > 23)

RuedigerMoeller commented 8 years ago

"sun.misc.Unsafe.allocateInstance" does not run default construction for transient fields. e.g.

class SerializableClass impls Serzbe{
   transient String foo = "BAR";
}

will have "foo" set to null after deserialization .. when allocated with Unsafe

RuedigerMoeller commented 8 years ago

@narayank regarding os detection: Pls prefer a System property for version detection as referencing Android specific classes is a hassle when not running on Android ..

Wavesonics commented 8 years ago

@narayank it would be great if Android set some more detail in the standard Java System properties.

Specifically, Why do ART run times report as Dalvik?

java.vm.name: 'Dalvik'

java.vm.specification.name and even java.vm.version all remain constant over quite a few versions of Android.

narayank commented 8 years ago

We don't plan on exposing any additional properties about system versions etc, especially since the equivalent constants exist in android.*. System properties are very brittle - they're mutable and can easily be ovewritten by code that's trying to be too "smart" (and yes - i've seen several examples of this). Using reflection to probe the existence of a certain class or method is a few more lines of code but is a bit more foolproof.

Anyhow - re your comment above, are you saying sun.misc.Unsafe.allocateInstance isn't good enough for you ? Objenesis uses it on Sun VMs.

henri-tremblay commented 8 years ago

@narayank Do you know why sun.reflect.ReflectionFactory isn't in Android N since it is supposed to be an OpenJDK class? Also, Android N currently returns 23. Is that a bug that will be fixed?

RuedigerMoeller commented 8 years ago

@narayank

"are you saying sun.misc.Unsafe.allocateInstance isn't good enough for you "

Its just wrong and so is Objenesis :), see my reasoning, that's not compatible to JDK serialization. That's why sun.reflect.ReflectionFactory is the best solution.

henri-tremblay commented 8 years ago

@RuedigerMoeller Objenesis should not be needed indeed but sadly is. Objenesis uses allocateInstance anyway in some cases. However, on OpenJDK it's not. It is using the ReflectionFactory because benchmarks proved it faster. And ObjectStreamClass for serialization.

In the case of Android N, it seems that ReflectionFactory isn't available. So I will use Unsafe and ObjectStreamClass.

But I was surprised to see ReflectionFactory disappear. Though my question to @narayank

henri-tremblay commented 8 years ago

To give you a heads up, right now I'm quite ready to deliver. I'm only still puzzled by the fact that the API level is still 23... So I'm currently using the bootclasspath trick.

narayank commented 8 years ago

The API level will be stuck at 23 (for complicated reasons I can't quite recall) until the Android N API is finalized.

I realize this is pretty awkward for folks doing things on the developer preview - you'll probably have to use "N".equals(VERSION.CODENAME) or your existing trick for the time being.

henri-tremblay commented 8 years ago

FWI, Objenesis 2.3 was released yesterday. It now support Android N

Wavesonics commented 8 years ago

Sounds like just updating the dependency and doing a release might fix our issue without any of these work arounds.

Wavesonics commented 8 years ago

The Android N public beta was released today making this a bit more of a pressing issue, @RuedigerMoeller would it be possible to upgrade to Objenesis 2.3 and do a release?

GaelCougnaud commented 8 years ago

Hi,

Our application use a third library, which has a dependency to fast-serialization. With first Android N versions, this issue happens. But with the last release of Android N (Developper preview 3), this issue no longer exists. Do you notice the same behavior ?

RuedigerMoeller commented 8 years ago

I'll update to objenesis 2.3 this week if you can confirm it fixes the issue. Still have some work/validation to do regarding unmodifiable related pull requests ..

henri-tremblay commented 8 years ago

And, BTW, objenesis 2.4 is now out. Le 23 mai 2016 9:15 AM, "moru0011" notifications@github.com a écrit :

I'll update to objenesis 2.3 this week if you can confirm it fixes the issue. Still have some work/validation to do regarding unmodifiable related pull requests ..

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/RuedigerMoeller/fast-serialization/issues/121#issuecomment-220976706

RuedigerMoeller commented 8 years ago

thanks :)

RuedigerMoeller commented 8 years ago

upgraded to objenesis 2.4 in fst 2.46

Wavesonics commented 8 years ago

Sweet! Just confirmed this does indeed fix the crash on Android N!

RuedigerMoeller commented 8 years ago

:)