Handle process death during filling a form

[grzesiek2010]

Closes #4250
Closes #4327
Closes #3873

What has been done to verify that this works as intended?

I tested the fix manually.

Why is this the best possible solution? Were any other approaches considered?

It consists a couple of fixes and I can't come up with any better solution. generally our code was prepared for such cases but we have never tested process death so the code needed some improvements and fixes.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I hope it will fix the problem with null form controller that we have handled in many cases adding null checks.
Testing the issue in the way I described using the forms I mentioned would be enough.
Please additionally check the functionality with and without don't keep activities option. You can also test killing the app manually what you probably do more often)

There is one thing: during testing I noticed that sometimes we can get stuck in a state where savepoints are not saved/updated so if we add changes in a form and then kill the app those changes are lost. unfortunately I'm not able to reproduce it now so please keep an eye on it.

Do we need any specific form for testing your changes? If so, please attach one.

Any form with media files to reproduce the steps described in the issue, it might be the demo form.
Also it would be good to tests a form with multiple questions on one page, I used this one:
groupTestForm.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[grzesiek2010]

There is one question that remains open: Do we need all those null checks we added to avoid using null formController object?
My answer is yes for now we need it because we start loading form in onCreate(), then it usually takes some time but other lifecycle methods like onStart(), onResume() are called and don't wait for form loading to be finished. In those methods we may try to use formController which at that point is null.

So this might be something for a separate pr. We could get rid of all those null checks if we added checks in lifecycle methods to make sure we skip executing them if a form is not ready.

Such cleaning would be great of course but I'm also a bit anxious, maybe there are other cases and removing those null checks would result in strange crashes? Hmm if we want take that risk and make the code a bit clearer it would be definitely something for an early beta.

@lognaturel @seadowg please share your thoughts.

[seadowg]

I'll hopefully be able to take a look at this tomorrow!

[grzesiek2010]

no rush it won't go in v1.29 and you probably have something more important.

[grzesiek2010]

In onResume() we have code that my heart would love to remove but I feel anxious:

if (formLoaderTask != null) {
            formLoaderTask.setFormLoaderListener(this);
            if (formController == null
                    && formLoaderTask.getStatus() == AsyncTask.Status.FINISHED) {
                FormController fec = formLoaderTask.getFormController();
                if (fec != null) {
                    if (!readPhoneStatePermissionRequestNeeded) {
                        loadingComplete(formLoaderTask, formLoaderTask.getFormDef(), null);
                    }
                } else {
                    DialogUtils.dismissDialog(FormLoadingDialogFragment.class, getSupportFragmentManager());
                    FormLoaderTask t = formLoaderTask;
                    formLoaderTask = null;
                    t.cancel(true);
                    t.destroy();
                    // there is no formController -- fire MainMenu activity?
                    startActivity(new Intent(this, MainMenuActivity.class));
                }
            }
        } else {
            if (formLoaderTask == null) {
                // there is no formController -- fire MainMenu activity?
                startActivity(new Intent(this, MainMenuActivity.class));
                finish();
                return;
            }
        }

Situation where we are in onResume() and formLoaderTask is not null and is finished but formController is null seems unrealistic.
Another case where both formLoaderTask and formController are nulls also seems strange.

What about logging events to check if it happens?

[grzesiek2010]

I'm still thinking about those null checks we have to avoid NPEs...
Doing it in a perfect way with our current code seems impossible, even if we introduce improvements in order to wait until form loading is finished and then remove all null checks it will be ok just for now but in the future it will be extremely difficult to remember about it to make sure we won't introduce NPEs.

A perfect solution could be separating form loading and displaying a form into two separate fragments. Thanks to that during form loading we wouldn't try to do all those things that are related to forms themselves and we wouldn't need all those null checks then.

Can you maybe thing of other solutions?

[seadowg]

@grzesiek2010 looks like there is a conflict to fix here before handing this to QA.

[grzesiek2010]

looks like there is a conflict to fix here before handing this to QA.

Yes I expected conflicts after merging last fixes for v1.29.2 today I'm going to review and fix.

[grzesiek2010]

After merging last fixes for v1.29.2 we need fewer changes here.

• 4f66854 is added because if pendingActivityResult is true in 4f66854#diff-150be8209a860d77a445a11267821bfba08a71b0b8c581bbb115641f42dc7919R2314 return is called and formControllerAvailable() won't be called. The app wouldn't crash in this case because we have this fix 2e762a7 but generally we shouldn't relay on it.
• 35884cb is added to fix returning to field-list group what we can test using the form I attached
  and that's seems to be everything we need for now.

There is one open question about those null checks what I described #4286 (comment) and in comments below.

[seadowg]

It looks like this change set breaks IdentifyUserTest (I think @lognaturel might have run into this before). We can probably do some rework on identify user or to get this to work in a simpler manner, but maybe we go with your original change (dealing with the pending activity result case specifically) for now?

[grzesiek2010]

It looks like this change set breaks

that small change breaks tests? I didn't expect it at all... I need to take a look to understand what's going on.

[seadowg]

that small change breaks tests? I didn't expect it at all... I need to take a look to understand what's going on.

@grzesiek2010 Yeah annoyingly. It feels like it's probably due to some over complication in the Identify User implementation. Your original solution didn't break those tests though.

[lognaturel]

Yes, this is now the same as #4326. To elaborate on the issue with the user identification, it's that the formLoaded method for that ViewModel requires the instance folder to exist which may not yet be the case at that point. I added the comment Precondition: the instance directory must be ready so that the audit file can be created on formControllerAvailable to hint at that.

I think this PR would ideally remove the hack introduced at #4328. We could do it later but it would be a nice sanity check that the issue is addressed since we can easily reproduce the navigation case.

[grzesiek2010]

Ok I fixed the problem and removed the fix implemented in #4328

[seadowg]

@getodk-bot label "needs testing"

[grzesiek2010]

There is one more issue that we may want to fix as a part of this pr:
https://console.firebase.google.com/u/0/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/f9991c3f867d61355cc17df3abee708c?time=last-thirty-days&versions=v1.29.2%20(4041)&sessionEventKey=5FF850BD00CA0001602398EAF3CBFBD3_1493812044950110121

I think it might share the same scenario with process death but additional condition is that null instance file.
What's interesting it's not a hard crash despite the fact it's NPE.

We could handle this case and close the form like we do in other similar cases but if it's not a hard crash I wonder why it doesn't cause other similar NPEs related to the instance file in other places...

[seadowg]

@grzesiek2010 I'm thinking that might be good to fix as a follow-up PR (probably worth creating an issue)?

[grzesiek2010]

Yes I agree especially that it's confusing. I reported it here: #4340

[mmarciniak90]

Tested with success

Verified on Android devices: 7.0, 8.1, 10.0

Verified cases:

• repro steps with image widget
• repro steps with video widget
• repro steps with audio widget
• form with multiple widgets on one page
• don't keep activities turned on and off
• verified image, selfie, annotate, new image widgets
• killing app manually

[kkrawczyk123]

Verified also on Androids: 5.1, 7.1 and 9.0.

@getodk-bot unlabel "needs testing"
@getodk-bot label "behavior verified"