Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParcelFileDescriptor image loading is broken pre 4.1.1_r1 #157

Closed
TWiStErRob opened this issue Sep 25, 2014 · 5 comments
Closed

ParcelFileDescriptor image loading is broken pre 4.1.1_r1 #157

TWiStErRob opened this issue Sep 25, 2014 · 5 comments
Assignees
Labels
Milestone

Comments

@TWiStErRob
Copy link
Collaborator

My friend complained that image chosen from Gallery didn't show up in my app on GT-I9000 (2.3.6):

Intent galleryIntent = new Intent(Intent.ACTION_GET_CONTENT);
galleryIntent.setType("image/*");

// onActivityResult:
String uri = intent.getData(); // "content://media/external/images/media/2"
Glide.with(this).load(uri).into(image);

I debugged on emulator API 10 / 2.3.7.

There's nothing in the logs, it just keeps skipping a line when I'm stepping and it's like I pressed Resume on debugger. It luckily breaks consistently and when I did "Run > Force Step into" I got into the IncompatibleClassChangeError constructor. Then new Exception().printStackTrace() in Evaluate helped (added corresponding lines for top 3):

IncompatibleClassChangeError: interface not implemented
    at com.bumptech.glide.load.data.LocalUriFetcher.cleanup(LocalUriFetcher.java:57): data.close();
    at com.bumptech.glide.load.model.ImageVideoModelLoader$ImageVideoFetcher.cleanup(ImageVideoModelLoader.java:102): fileDescriptorFetcher.cleanup();
    at com.bumptech.glide.load.engine.SourceResourceRunner.decodeFromSource(SourceResourceRunner.java:187): finally { fetcher.cleanup(); }
    at com.bumptech.glide.load.engine.SourceResourceRunner.runWrapped(SourceResourceRunner.java:144)
    at com.bumptech.glide.load.engine.SourceResourceRunner.run(SourceResourceRunner.java:123)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:444)
    at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:306)
    at java.util.concurrent.FutureTask.run(FutureTask.java:138)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
    at java.lang.Thread.run(Thread.java:1019)
    at com.bumptech.glide.load.engine.executor.FifoPriorityThreadPoolExecutor$DefaultThreadFactory$1.run(FifoPriorityThreadPoolExecutor.java:52)

At data.close() data is declared as T data (<T extends Closeable>) and in memory we have:

data instanceof android.content.ContentResolver$ParcelFileDescriptorInner == true
data instanceof java.io.Closeable == false

So it's trying to make a virtual call to close() through the Closeable interface:

data.close();
this     //    3    7:aload_0         
.data    //    4    8:getfield        #18  <Field Closeable data>
.close() //    5   11:invokeinterface #19  <Method void Closeable.close()>

The Closeable interface to ParcelFileDescriptor was added in this commit and it has the tag 4.1.1_r1.

So I suspect the following versions are broken (not tested):

Platform Version API Level VERSION_CODE
Android 4.1, 4.1.1 16 JELLY_BEAN
Android 4.0.3, 4.0.4 15 ICE_CREAM_SANDWICH_MR1
Android 4.0, 4.0.1, 4.0.2 14 ICE_CREAM_SANDWICH
Android 3.2 13 HONEYCOMB_MR2
Android 3.1.x 12 HONEYCOMB_MR1
Android 3.0.x 11 HONEYCOMB
Android 2.3.3, 2.3.4 10 GINGERBREAD_MR1
and below 1..9 but you just said it's not supported
@sjudd
Copy link
Collaborator

sjudd commented Sep 26, 2014

It's worth testing this, the framework claims that #close() was added in API level 1. I wonder if there is a weird device specific bug? It's possible something about not having the interface but having the method is messing with things? I should add a Gallery sample app, I've written parts of it before but never committed.

@TWiStErRob
Copy link
Collaborator Author

Java doesn't support duck-typing. Having the method is not enough to call it through the interface. It should jave failed much earlier with a ClassCastException, but it didn't because of the erasure. I agree though some repro is needed. Do you have old devices or just emu?

@sjudd
Copy link
Collaborator

sjudd commented Sep 26, 2014

Right it doesn't support duck typing, what I meant was it seems conceptually like nothing should be broken, the method in question has existed forever. If adding a new interface that covered an existing method broke code on old platforms that would be a problem?

I have both real devices and emulators. I've tested on a couple versions of Android using emulators and so far I can't reproduce, I'll keep trying and try real devices tomorrow.

@TWiStErRob
Copy link
Collaborator Author

TWiStErRob commented Sep 26, 2014

It's not trivial to get the exception stack trace. The only visible repro I could do is not showing an image. Here's an app which displays the stack trace for me, even on 2.3.7: https://github.com/TWiStErRob/random/tree/master/GlideGallery https://github.com/TWiStErRob/repros/tree/main/glide/IncompatibleClassChangeError-on-Closeable

it seems conceptually like nothing should be broken

ParcelFileDescriptor.close() and Closeable.close() are two different methods:

Method pfdClose = ParcelFileDescriptor.class.getDeclaredMethod("close")
Method cClose = Closeable.class.getDeclaredMethod("close");
assert !cClose.equals(pfdClose); // even on 4.4

invokeinterface above does this I guess: cClose.invoke(aDescriptor); and since in the old version they are not related it just blows up. The following snippet works on 4.4, but fails on 2.3.7 (just break anywhere in an activity and do a Run > Evaluate Expression):

// 7973 has to be a valid image id
java.io.Closeable.class.getDeclaredMethod("close").invoke(getContentResolver().openFileDescriptor(Uri.parse("content://media/external/images/media/7973"), "r"))

// 2.3.7: java.lang.IllegalArgumentException: object is not an instance of the class

Also this would fail compiling with target < 4.1.1:

Closeable data = getContentResolver().openFileDescriptor(null, null);

@sjudd
Copy link
Collaborator

sjudd commented Sep 26, 2014

Argh I'm sorry I should have looked at the code before responding. I was assuming it was calling close() on the ParcelFileDescriptor, not on the Closeable interface, but I just looked and I am in fact calling close on the interface. Sorry for wasting your time! I'll have a fix up shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants