BasicFieldImageView crashes when switching categories too quickly

[xtknight]

When using blocks containing an image, if we switch Blockly categories too quickly, there is the possibility of a crash. This is in production code and we have copied the Blockly module source code into our project and are compiling directly due to the need for customizations.

(Placing around 10 BasicFieldImageViews referencing an image from a network resource in a few categories and switching between them fast should be enough to reproduce the issue. Reproducible in emulator.) I suppose even if it's not a network resource, the possibility for a crash could still exist depending on how long it takes to load the resource from the desired media. I suppose it's fundamentally unsafe to run an asynchronous task and at an undefined time destroy what it relies on causing a null dereference, despite the fact that the case may be rare.

10-12 13:11:18.299 23203-23203/com.xxxx.yyyy E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.xxxx.yyyy, PID: 23203
    java.lang.NullPointerException: Attempt to invoke virtual method 'int com.google.blockly.model.FieldImage.getWidth()' on a null object reference
        at com.google.blockly.android.ui.fieldview.BasicFieldImageView.updateViewSize(BasicFieldImageView.java:189)
        at com.google.blockly.android.ui.fieldview.BasicFieldImageView$2.onPostExecute(BasicFieldImageView.java:143)
        at com.google.blockly.android.ui.fieldview.BasicFieldImageView$2.onPostExecute(BasicFieldImageView.java:118)
        at android.os.AsyncTask.finish(AsyncTask.java:667)
        at android.os.AsyncTask.-wrap1(AsyncTask.java)
        at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:684)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:154)
        at android.app.ActivityThread.main(ActivityThread.java:6119)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

It seems to be because when the category changes, the layout of the ImageView gets destroyed but yet updateViewSize() gets called. It seems sufficient to busy-wait for the AsyncTask of downloading the image to be complete. I don't know if this is the "proper" solution but it seems to work for us so I wanted to share. Nor have I had the opportunity to try this on the latest git revision but wanted to provide this solution as reference for others.

BasicFieldImageView.java (startLoadingImage function):

diff --git a/blocklylib-core/src/main/java/com/google/blockly/android/ui/fieldview/BasicFieldImageView.java b/blocklylib-core/src/main/java/com/google/blockly/android/ui/fieldview/BasicFieldImageView.java
index 89e9123..91abe62 100755
--- a/blocklylib-core/src/main/java/com/google/blockly/android/ui/fieldview/BasicFieldImageView.java
+++ b/blocklylib-core/src/main/java/com/google/blockly/android/ui/fieldview/BasicFieldImageView.java
@@ -31,6 +31,7 @@ import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.util.concurrent.ExecutionException;
 import java.util.regex.Pattern;

 /**
@@ -114,7 +115,7 @@ public class BasicFieldImageView extends android.support.v7.widget.AppCompatImag
         final String source = mImageField.getSource();

         // Do I/O in the background
-        new AsyncTask<String, Void, Bitmap>() {
+        AsyncTask task = new AsyncTask<String, Void, Bitmap>() {
             @Override
             protected Bitmap doInBackground(String... strings) {
                 try {
@@ -135,6 +136,7 @@ public class BasicFieldImageView extends android.support.v7.widget.AppCompatImag

             @Override
             protected void onPostExecute(Bitmap bitmap) {
+                //Log.w(TAG, "Might crash soon");
                 if (bitmap != null) {
                     setImageBitmap(bitmap);
                     mImageSrc = source;
@@ -146,6 +148,19 @@ public class BasicFieldImageView extends android.support.v7.widget.AppCompatImag
                 requestLayout();
             }
         }.execute(source);
+
+        // Crash when layout gets destroyed and we call updateViewSize()
+        // Can't execute network on main thread
+        // So execute it on an AsyncTask and wait instead.
+        try {
+            task.get();
+        } catch (InterruptedException e) {
+            Log.w(TAG, "BasicFieldImageView AsyncTask interrupted");
+            e.printStackTrace();
+        } catch (ExecutionException e) {
+            Log.w(TAG, "BasicFieldImageView AsyncTask execution error");
+            e.printStackTrace();
+        }
     }

TL;DR: workaround, not patch

[AnmAtAnm]

Thanks @xtknight for the excellent bug report and possible solution.

That said, I have not been able to recreate the error. I've added blocks to dev tests that I believe reflect the scenario you've described. I've run this on the emulator with several Android API levels. None crash.

Can you provide an example block and toolbox where you see this problem?

Looking through the code, I see that line is already wrapped in a null check for mImageField. That generally should protect from the NullPointerException in you stack trace.

It looks like there is a possible race condition between setField(..) and updateViewSize() (possible from the AsyncTask post execute). I've added code to protect against that, but I don't have a fail case to test whether it solves the problem.

[xtknight]

Thank you for investigating. This does seem to be fixed on the latest version! If I use the new code in updateViewSize() to check null for mImageField, it seems to fix the problem. I thought I had checked the diff between the BasicFieldImageView I had and the latest git version but I guess I just missed it. But I guess it doesn't hurt to add the test for regression testing purposes anyway. Apparently the fix was already issued when this person had the problem: https://groups.google.com/d/msg/blockly/lC91XADUiI4/Y0cLRAYQBQAJ

Although this is a different issue, the image loading does seem awful slow (each get updated one-by-one) when using multiple blocks.

[AnmAtAnm]

Although this is a different issue, the image loading does seem awful slow (each get updated one-by-one) when using multiple blocks.

Yes. I hadn't realized how bad that code was until investigating this issue. It needs a local cache, a way to update multiple fields looking at the same image, and ideally a pool of threads specific for network requests (as opposed to local disk I/O, which should be very fast). I'll put up some help-wanted issues for all of these.

[xtknight]

I suppose loading images from the network in this case may not be the most common use case but very attentive to detail. Thanks!

[xtknight]

Going to close this for now since it's not a crash in HEAD and regression tests have been added.