CB-8917: Added pending plugin callbacks to resume event payload

[riknoll]

This is a redo of #236 after receiving some feedback. This relates to CB-8917 and CB-9189.

Background

The issue at hand is that plugins can make calls to an external Activity and, if the device is low on memory, there is a chance that the CordovaActivity will get killed in the background causing the plugin to lose its state as well as its context for the callback. Activities in Android typically handle this situation by using onSaveInstanceState() and onCreate() methods to save and restore state respectively as the Activity is created and destroyed. This solution exposes that lifecycle to plugins, allowing them to save state and have it restored if necessary.

Saving/Restoring plugin state

Two new methods are exposed to plugins that they can override to save/restore state.

/**
 * Called when the Activity is being destroyed (e.g. if a plugin calls out to an external
 * Activity and the OS kills the CordovaActivity in the background). The plugin should save its
 * state in this method only if it is awaiting the result of an external Activity and needs
 * to preserve some information so as to handle that result; onRestoreStateForActivityResult()
 * will only be called if the plugin is the recipient of an Activity result
 *
 * @return  Bundle containing the state of the plugin or null if state does not need to be saved
 */
public Bundle onSaveInstanceState() {}

/**
 * Called when a plugin is the recipient of an Activity result after the CordovaActivity has
 * been destroyed. The Bundle will be the same as the one the plugin returned in
 * onSaveInstanceState()
 *
 * @param state             Bundle containing the state of the plugin
 * @param callbackContext   Replacement Context to return the plugin result to
 */
public void onRestoreStateForActivityResult(Bundle state, CallbackContext callbackContext) {}

The plugin is given a replacement CallbackContext as part of onRestoreStateForActivityResult that can accept the result the plugin would normally return and add it to the resume event payload for use in the js (see JSON below). Thus, it requires minimal modifications to existing plugins

NOTE: When I mention that plugins are given the opportunity to restore state, I want to clarify that this only happens for plugins that are waiting for an external Activity result. This makes the API a little less intuitive, but otherwise we would be conflicting with the accepted behavior that plugins currently get destroyed (i.e. lose all of their state) and are selectively rebuilt whenever a new URL is loaded into the webview. If we restore state on resume, then we can end up with some awkward cases where part of the resuming involves loading a new page so the state gets lost again and so on and so forth. My thinking is that restoring the other state is better left to app developers

Saving/Restoring js state

We already send out pause and resume events. This solution enhances these events in the case of Activity destruction by adding to them the result of any pending Plugin calls. The resume event is of the form

{
    action: "resume",
    pendingResult: {
        pluginServiceName: <plugin service name e.g. "Camera">,
        pluginStatus: <description of result' status (see PluginResult.java)>,
        result: <argument(s) that would have been given to the callback>
    }
}

It is the responsibility of the application developer to properly use these events and save their state as well as keep information about what plugin results they have pending. We should provide guidance for this in the Android documentation and plugin documentation should clearly communicate when it is necessary.

Discussion

Benefits:

• It requires minimal updates to existing plugins (and no updates at all if the plugin doesn't use an external Activity)
• It is a general solution/pattern that plugins can follow rather than forcing them to include platform specific methods in their APIs

Downsides:

• The resume callback will only ever get received after the initial page loads (potential page flickering)
• The pending result part of the event object doesn't provide much context (so it puts more responsibility on the app developer to keep state so they know what they're getting)

In the core plugins, this is mostly relevant to the Camera plugin which previously would crash upon receiving the Activity result if the CordovaActivity had been killed by the OS while a picture was being taken/chosen (CB-9189). The updated Camera Plugin can be found in this branch and a (trivial) example application that uses this API + instructions for testing can be found here.

[riknoll]

@infil00p @purplecabbage @jasongin please review. This incorporates feedback received on the first PR. Example plugin and application code are linked at bottom of the description

[jasongin]

It looks like there were no real changes to this file. Was this just leftover from a previous iteration?

[riknoll]

Yep! I'll update the PR.

[Titoine]

Hi,
First of all, thanks from the great work!
But I'm getting a lot of crash from this problem.

Do you have any idea when this will be merge ?

[riknoll]

Hey all, I've added a PR to the docs repo that documents the result callback part of this change as well as general Android lifecycle considerations: apache/cordova-docs#428

[infil00p]

@Titoine Are you having problems after the patch or before? I'm not sure.

[Titoine]

@infil00p Before the patch.

[Bnaya]

@riknoll, after going over the docs (which looks great btw) its seems it supports only single plugin result
What if you wait for two or more plugins responses? like file system operation and camera plugin result?

[riknoll]

@Bnaya That is correct and it was intentional. Is there a need for extending this to all plugin callbacks currently pending? The way I see it, the burden is on the app developer for being careful about what plugins are launching external Activities and maintaining their state accordingly (for example, not launching the camera activity while waiting on a file transfer). Plugins have access to the Activity lifecycle on the native side, so they should be able to handle whatever is happening. I guess plugins lose the ability to notify the javascript side about what's going on, (like a file transfer failing) but hopefully the app developer is being careful so that those situations don't occur.

[riknoll]

Also, I don't think you can get into a situation where you have two Activity results pending (correct me if I'm wrong on that one).

[Bnaya]

I will extend the scope of that issue.

This change can/should also help the apps developers to recover from situations that the app sent to the background not only by activity from the app, but also if the user hit the home button/notifications etc
And lets say the app had several IO ongoing operations by plugins. even 2 operations by the same plugin.
The operating system can kill the activity also in such case, and then if the user navigates back to the app and the webview will be reloaded with resume event.

[infil00p]

@Bnaya @riknoll Unless you're getting a result back from an Activity, the plugin processes belong to the activity that was put in the background, and the plugins would die with the activity that was killed. Therefore, there could only be one result returning to the application. This issue is related to communication between two activities, not trying to resurrect plugin code, which is really just class that's not even guaranteed to have its own thread (although it should).

I think the problems are different enough that the scope shouldn't be changed here, and that the second problem that your mentioning needs to be fleshed out more.

[riknoll]

Alright, I plan on merging this in soon. I want to do some more testing and write some plugin-side docs before I do, but since this PR's been open a while and I don't think this will really be much of a breaking change, I am going to push forward. I plan to merge it in early next week. Let me know if there are any objections and I will try to address them!

[riknoll]

I will submit a PR that updates the camera plugin to use the save-restore API as well, which should fix CB-9189. I'll also take a look at the contacts plugin, because I believe it uses external activities.

[riknoll]

Apologies for not getting to this last week like I said I would (I didn't really have much of an Internet connection). I did some more testing on this PR today and found a few bugs/edge cases that I've now fixed. This PR has been rebased to master and I pushed a new commit that handles the following:

• PluginResults with multipart messages are now handled properly in the resume payload
• The resume event is now fired when the Activity is destroyed even if there isn't a pending result
• A potential concurrency issue in CoreAndroid has been handled

Please review this new commit. Since I pushed some changes I will wait to merge this in until end of day tomorrow. Let met know if there are any objections. For the record, I tested as follows:

• Ran mobilespec auto tests on a 6.0.0 device and a 5.1.1 emulator (minus the file transfer tests) and confirmed output matched master (one orientation failure on 5.1.1, but I believe it is the emulator's fault)
• Ran the platform unit tests on a 6.0.0 device and a 5.1.1 emulator and confirmed output matched master (one failure)
• Did some manual testing with a 6.0.0 device and a 5.1.1 emulator

[jasongin]

LGTM

[Titoine]

When I try my app with "Don't keep activities" on, my app crash and I have the "unfortunately MyApp has stopped" message. The app doesn't restart. Will it restart with your fix or on some devices the problem still occur ?
On Wiko Cink Five with Android 4.1.2

[Bnaya]

@Titoine This is not a complete fix, and its not released yet
There's also corresponding changes to the camera plugin this change
When it will be released you will need to update your android platform and your camera plugin.

For now, you can try my fix for that issue, which includes more fixes for other edge cases with the plugin
https://github.com/photomania/cordova-plugin-camera/commits/prevent-crash-on-external-image-selection

Note that its not rebased with the lasted code of the camera plugin but its works ok and we use if in production.

In order to fetch the recovered image result after the restart you should use plugin that reads app preferences (we use cordova-plugin-app-preferences 0.6.1 "AppPreferences")
And when the app starts we do:

        checkForActivityRestart: function () {
            window.plugins.appPreferences.fetch (function (cameraPluginResult) {
                if (cameraPluginResult) {
                    window.plugins.appPreferences.remove(function () {

                        // if we have preserved cameraPluginResult that means
                        // the activity was killed when the camera/gallery activity opened
                        this.sandbox.bi.track('debug', {action: 'activity recovery', label: ''});
                        this.duringGetPicture = true;
                        this.photoSelected('restart recovery', cameraPluginResult);

                    }.bind(this), function () {
                        // ignore errors
                    }, 'camera-plugin-result');
                } else {
                    // old or its not android, do nothing
                    return;
                }
            }.bind(this), function () {
                // ignore errors
                return;
            }, 'camera-plugin-result');

        },

[Titoine]

Ok! With your fix or in futur update, when the app crash, is the user have to launch it again or does It launch itself ? Or both of the case could happen ?

[Bnaya]

The app will not crash but just restart itself without the user doing anything.

The thing is that, if you want to use the picture he choose before the restart you will need another code path to handle it. the callback you gave to getPicture will not be called.

You can try an just let him to select photo again, but then the app might restart soon

[Titoine]

Thanks, I understand now.
Hope to see this implemented soon. It's a critical issue for me.
Anyway, thanks you all for the great work!