iOS crash due to recursive call while starting app in the ongoing state with the native consent missing #18

shankari · 2018-09-25T20:43:17Z

This is a super corner case, but I want to document it in case we can fix it as part of the upcoming refactoring. This happens only IFF:

the app is in ONGOING_STATE while being launched
the app has no native consent set

If there is no native consent set, but there is local consent, we call markConsented
which calls initWithConsent, which creates the TripDiaryStateMachine using self.tripDiaryStateMachine = [TripDiaryStateMachine instance];

While creating the TripDiaryStateMachine, we look to see whether the currState is the ongoingTripState

    if (self.currState == kOngoingTripState) {
        // If we restarted, we recreate the location manager, but then it won't have
        // the fine location turned on, since that is not carried through over restarts.
        // So let's restart the tracking
        [TripDiaryActions startTracking:CFCTransitionTripRestarted withLocationMgr:self.locMgr];
        // Note that if the phone was shut down when the app was in the ongoing trip state, and it was
        // turned back on at home, we will start tracking here but will most probably not get a visit transition
        // so the data collection will be turned on until the NEXT trip ends. This is why we need remote pushes, I think.
        // would be good to test, though.
    }

startTrackingActions generates a notification

    [[NSNotificationCenter defaultCenter] postNotificationName:CFCTransitionNotificationName
                                                        object:CFCTransitionTripStarted];

which calls fireGenericTransitionFor, which in turn calls

    if ([TripDiaryStateMachine instance].currState == kWaitingForTripStartState &&

This is a recursive call, so this fails.

The text was updated successfully, but these errors were encountered:

shankari · 2018-09-25T20:59:54Z

In the general case, the BEMTransitionNotifier will register for CFCTransitionNotificationName only after the TripDiaryStateMachine is initialized, so this won't happen.

    [[NSNotificationCenter defaultCenter] addObserverForName:CFCTransitionNotificationName object:nil queue:nil
                                                  usingBlock:^(NSNotification* note) {
                                                      __typeof(self) __strong strongSelf = weakSelf;
                                                      [strongSelf fireGenericTransitionFor:(NSString*)note.object withUserInfo:note.userInfo];
                                                  }];

But in this case, we delayed initializing the TripDiaryStateMachine because there was no consent. Normally, if there is no consent, the user has not installed the app, so the state machine has not been initialized, so this will not happen. So this is a really corner case.

But the fix is pretty simple. We can just check for the consent before registering for the notifications too.

Back in the old days, we tried to move all storage to local storage so that we could read it without waiting for ionicPlatform.ready and startup quickly. Well, it turned out that sometimes local storage was deleted on iOS, so we ended up with the case where local intro_done was deleted, so we would reset the UI. Which looked stupid and like a bug (https://github.com/e-mission/e-mission-phone/issues/443). Re-writing to pull out the KV storage into a service, write primarily to native with local as backup, and read from both and unify. Everything was already promisified because we were reading consent from native code, so I am not sure that this actually affects performance. And it is a heck of a lot easier to understand. Testing done: - started app -> went to intro - restarted app after restarting emulator -> went to current screen. For fixes to the current screen, see 0ecfdae - cleared out intro_done and consent from local storage and restarted app -> went to current screen - cleared all native storage, generated popups for intro_done and consent, then crashed due to e-mission/e-mission-transition-notify#18 On relaunch, launched properly without any popups. I declare that this is done. ``` [phonegap] [console.log] DEBUG:uc_stored_val = null ls_stored_val = {"intro_done":"2018-09-25T13:14:51-07:00"} ```

shankari · 2018-10-21T20:14:47Z

ok so I ran into this in a slightly different context (e-mission/cordova-jwt-auth#33). In this case, the FSM state does not matter. Instead, the client calls BEMDataCollection.setConfig to configure the data collection settings on startup. That calls BEMDataCollection.restartCollection, which in turn posts a generic STOP notification.

    [[NSNotificationCenter defaultCenter] postNotificationName:CFCTransitionNotificationName
                                                        object:CFCTransitionForceStopTracking];

which calls fireGenericTransitionFor and the rest of the code path continues as above (#18 (comment)).

The final fix needs to address this use case as well.

The entire call stack is as below.

#2	0x00000001030121b8 in _dispatch_once [inlined] at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator11.4.sdk/usr/include/dispatch/once.h:84
#3	0x000000010301219d in +[TripDiaryStateMachine instance] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/Location/TripDiaryStateMachine.m:54
#4	0x000000010301e066 in -[BEMTransitionNotifier fireGenericTransitionFor:withUserInfo:] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.transitionnotify/BEMTransitionNotifier.m:55
#5	0x000000010301defb in __41-[BEMTransitionNotifier pluginInitialize]_block_invoke at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.transitionnotify/BEMTransitionNotifier.m:48
...
#13	0x000000010300e464 in +[TripDiaryActions startTracking:withLocationMgr:] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/Location/TripDiaryActions.m:40
#14	0x0000000103012666 in -[TripDiaryStateMachine init] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/Location/TripDiaryStateMachine.m:94
#15	0x0000000103012212 in __33+[TripDiaryStateMachine instance]_block_invoke at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/Location/TripDiaryStateMachine.m:55
...
#18	0x00000001030121b8 in _dispatch_once [inlined] at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator11.4.sdk/usr/include/dispatch/once.h:84
#19	0x000000010301219d in +[TripDiaryStateMachine instance] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/Location/TripDiaryStateMachine.m:54
#20	0x000000010301e066 in -[BEMTransitionNotifier fireGenericTransitionFor:withUserInfo:] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.transitionnotify/BEMTransitionNotifier.m:55
#21	0x000000010301defb in __41-[BEMTransitionNotifier pluginInitialize]_block_invoke at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.transitionnotify/BEMTransitionNotifier.m:48
.....
#29	0x0000000103008c9c in -[BEMDataCollection restartCollection] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/BEMDataCollection.m:309
#30	0x00000001030071a4 in -[BEMDataCollection setConfig:] at /Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/edu.berkeley.eecs.emission.cordova.datacollection/BEMDataCollection.m:163

uwtcat · 2018-10-21T20:37:07Z

I'm not convinced this is the same issue as #33, although that issue was contributed via video, so I cannot say whether it's the second time the user tried to open the OpenToAll login.

If we are, we skip the call to allow the original call to complete. This is part of the fix - the other part will be checked in to the transition notifier shortly. This fixes e-mission/e-mission-transition-notify#18

If we are, this will return NULL and we skip the translation since it is unlikely to be a real transition anyway. This is part of the fix - the other part will be checked in to the transition notifier shortly. This fixes e-mission#18

shankari · 2020-04-04T00:13:27Z

This fix didn't work.

shankari · 2020-04-04T00:13:50Z

Essentially it makes it so that every call except the first one returns null.

shankari · 2020-04-04T00:35:41Z

ok so let's try to fix this the principled way.

The fundamental problem is that as part of TripDiaryStateMachine init, we start tracking.
And starting the tracking generates a tracking action.

There can be any number of listeners to the action and they can do anything, including calling the TripDiaryStateMachine again, like the transition code currently does.

The main problem is that the action listeners all execute in the same thread as the init. It seems like we have two possible fixes here.

run the start tracking code in a separate thread
restrict access to the [TripDiaryStateMachine instance] call through an interface which spawns separate threads

there is only one call to [TripDiaryStateMachine instance] that is not the initial init so the second option would not involve too much rewriting.

e-mission-phone/plugins/edu.berkeley.eecs.emission.cordova.datacollection/src/ios/BEMDataCollection.m:    self.tripDiaryStateMachine = [TripDiaryStateMachine instance];
e-mission-phone/plugins/edu.berkeley.eecs.emission.cordova.transitionnotify/src/ios/BEMTransitionNotifier.m:    if ([TripDiaryStateMachine instance].currState == kWaitingForTripStartState &&

shankari · 2020-04-04T00:49:36Z

the first option is really not consistent with the current code. We do a lot of tracking code actions in that init. We could move all of them to separate threads for consistency, but then we would need to re-test super carefully.

The other option is to restrict access to the [TripDiaryStateMachine instance] call through an interface. This may be the better long-term option. However, since:

it is only used in one other place, and
none of the other reposts in that other place have a check on the current state, and
there is no documentation on why this was needed, an easier option is to just remove the outside check for now

See discussion at e-mission#18 (comment) This fixes e-mission#18

shankari mentioned this issue Oct 21, 2018

emTripLog crashes on opening on iOS e-mission/cordova-jwt-auth#33

Closed

shankari mentioned this issue Apr 3, 2020

Add a check to see if we are initializing the state machine recursively e-mission/e-mission-data-collection#181

Merged

shankari mentioned this issue Apr 3, 2020

Add a check to see if we are initializing the state machine recursively #22

Merged

shankari closed this as completed in e-mission/e-mission-data-collection#181 Apr 3, 2020

shankari reopened this Apr 4, 2020

shankari added a commit to shankari/e-mission-transition-notify that referenced this issue Apr 4, 2020

More principled fix for the transition notify crash

88cc83f

See discussion at e-mission#18 (comment) This fixes e-mission#18

shankari mentioned this issue Apr 4, 2020

Add a more principled fix for the transition notify crash #23

Merged

shankari closed this as completed in #23 Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS crash due to recursive call while starting app in the ongoing state with the native consent missing #18

iOS crash due to recursive call while starting app in the ongoing state with the native consent missing #18

shankari commented Sep 25, 2018

shankari commented Sep 25, 2018

shankari commented Oct 21, 2018 •

edited

Loading

uwtcat commented Oct 21, 2018

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

iOS crash due to recursive call while starting app in the ongoing state with the native consent missing #18

iOS crash due to recursive call while starting app in the ongoing state with the native consent missing #18

Comments

shankari commented Sep 25, 2018

shankari commented Sep 25, 2018

shankari commented Oct 21, 2018 • edited Loading

uwtcat commented Oct 21, 2018

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

shankari commented Apr 4, 2020

shankari commented Oct 21, 2018 •

edited

Loading