search

getodk / collect

ODK Collect is an Android app for filling out forms. It's been used to collect billions of data points in challenging environments around the world. Contribute and make the world a better place! ✨📋✨
https://docs.getodk.org/collect-intro
Other
719 stars 1.37k forks source link

NPE in FormEntryViewModel.java:86 #4204

Closed grzesiek2010 closed 3 years ago

grzesiek2010 commented 3 years ago

Software and hardware versions

Collect v1.28.x

Problem description

https://console.firebase.google.com/u/0/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/534ada560093cfeb6de0107286b43b3f?time=last-seven-days&versions=v1.28.3%20(3959)&sessionEventKey=5FA27246022000011325DEFD0327C76A_1469641808994180352

Fatal Exception: java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String java.lang.Throwable.getMessage()' on a null object reference
       at org.odk.collect.android.formentry.FormEntryViewModel.addRepeat(FormEntryViewModel.java:86)
       at org.odk.collect.android.activities.FormEntryActivity$4.onAddRepeatClicked(FormEntryActivity.java:1763)
       at org.odk.collect.android.formentry.repeats.AddRepeatDialog.lambda$show$0(AddRepeatDialog.java:22)
       at org.odk.collect.android.formentry.repeats.-$$Lambda$AddRepeatDialog$cqe_gnMZqwigRgODoyw0mha9oJI.onClick(-.java:2)
       at androidx.appcompat.app.AlertController$ButtonHandler.handleMessage(AlertController.java:167)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loop(Looper.java:193)
       at android.app.ActivityThread.main(ActivityThread.java:6819)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:497)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:912)

The issue is related with: https://github.com/getodk/collect/pull/4148

grzesiek2010 commented 3 years ago

@seadowg what do you think about this issue. It's related to your fix. Do you think your fix is still satisfying and we can just check for null cause or maybe we need to dig deeper?

seadowg commented 3 years ago

Hmmm. Yeah I mean that is a nullable accessor (getCause()) so yes we really should be checking for null there.

Do we have any idea of reproduction for this? It seems in most cases where we're dealing with a JavaRosaException we grab the cause Throwable rather than going to the message on the exception itself. It'd be good to understand why in this case that's different and how to account for that as (unless we have repro) we don't know if there is any point in displaying e.getMessage() to the user. Maybe for the moment we hope the message is going to be useful and display that?

grzesiek2010 commented 3 years ago

Do we have any idea of reproduction for this?

we had a form to reproduce the issue you fixed in #4148 but that throws an exception with cause so there might be another way to reproduce it.

we don't know if there is any point in displaying e.getMessage() to the user.

generally the message is helpful (if exists) because some users might figure out what they need to change on their own: Screenshot_1604492833

There is probably another way to brake something in a form. Difficult to tell what.

If we can't reproduce probably it's better to just catch that NPE and display something like Unknown exception occurred or something because most likely it's another bug in a form as I said above.

seadowg commented 3 years ago

If we can't reproduce probably it's better to just catch that NPE and display something like Unknown exception occurred or something because most likely it's another bug in a form as I said above.

Yeah I'm happy with that.