1349853 - Sync sign-in unresponsive

Status: VERIFIED FIXED

(Firefox for iOS :: Sync, defect, P1)

Description

[Maurya Talisetti]

Attachment: Simulator Screen Shot 23-Mar-2017, 11.18.30 AM.png

Sync sign-in has been unresponsive (on version 8.0(1)) since yesterday (Mar 22, 2017).

1. If you are already logged into Sync, first "Sign Out" from Settings.
2. Navigate to Settings -> Sign In to Firefox
3. Enter your email/password and tap "Sign in".

**If using this account for the first time on the device:**
You'll be sent a confirmation email for validation. So, go ahead and confirm that. You'll notice that the Settings screen on your device recognizes that the confirmation has been done. However, it will not log you into the Sync account. It'll still display the "Sign In to Firefox" option in Settings.

**If you have previously used this account on the device:**
You'll notice that the spinner keeps spinning forever. You'll also notice that a red bar appears at the top of the page with the text "Working...". (See screenshot)

The behavior is the same on the iOS simulator and on an iPhone. However, it works fine for the App Store version (6.1). It's strange given that both use the same URL: https://accounts.firefox.com/signin?service=sync&context=fx_ios_v1

If you try to manually navigate to that URL from your browser's URL bar (desktop or mobile), you'll see the same unresponsive behavior.

I didn't classify the bug under "Firefox/Sync" because I'm able to sync from version 6.1 which is using the same URL.

Comment 1

[Catalin Suciu [:csuciu]]

Able to reproduce on latest master (0da53b909b). Works fine in Beta 2150.

Debugger:
[Info] [KeychainCache.swift:54] checkpoint() > Storing account.state in Keychain with label account.state.EVSXHJYa05ki.
[Warning] [KeychainCache.swift:42] fromBranch(_:withLabel:withDefault:factory:) > Did not find account.syncAuthState in Keychain with label account.syncAuthState.EVSXHJYa05ki.
[Warning] [KeychainCache.swift:48] fromBranch(_:withLabel:withDefault:factory:) > Failed to read account.syncAuthState from Keychain.

Comment 2

[Stephan Leroux [:sleroux]]

Possibly related to https://github.com/mozilla-mobile/firefox-ios/commit/8d08ece9603b74083e9f71d5c55cc504872e5c5f?

Needs some investigation. Moving into this sprint since this is our last 8.0 sprint.

Comment 3

[Jacob White]

This might be because the simulator doesn't support remote push notifications. I can't test on device right now.

Comment 4

[Maurya Talisetti]

Jacob, the result was the same on both the simulator and on a device. And, I've used Sync without any problems (including with remote push notifications) on a simulator multiple times in the recent past.

Comment 5

[Stephan Leroux [:sleroux]]

After reverting https://github.com/mozilla-mobile/firefox-ios/commit/8d08ece9603b74083e9f71d5c55cc504872e5c5f it looks like this is still occurring so that doesn't seem to be the issue...

Comment 6

[Maurya Talisetti]

Stephan,

(from comment #0)
> https://accounts.firefox.com/signin?service=sync&context=fx_ios_v1
> 
> If you try to manually navigate to that URL from your browser's URL bar
> (desktop or mobile), you'll see the same unresponsive behavior.
> version 6.1 which is using the same URL.

So, it's probably not necessarily to do with the Firefox for iOS app.

Comment 7

[Jacob White]

(In reply to Maurya Talisetti from comment #6)
> Stephan,
> 
> (from comment #0)
> > https://accounts.firefox.com/signin?service=sync&context=fx_ios_v1
> > 
> > If you try to manually navigate to that URL from your browser's URL bar
> > (desktop or mobile), you'll see the same unresponsive behavior.
> > version 6.1 which is using the same URL.
> 
> So, it's probably not necessarily to do with the Firefox for iOS app.

Maybe this bug is being intermittent?

Comment 8

[Stephan Leroux [:sleroux]]

Looks like the issue is that after signing into your FxA account, we go ahead and show the prompt asking the user's permission for notifications [1]. When the user accepts the prompt, the app delegate's didRegister notificationSettings method is called with the result of the user's choice. We don't actually implement this method anywhere in the app delegate so essentially the user's choice is swallowed up and the FxALoginHelper doesn't continue onwards causing the success callback which updates the settings view to show the user is logged in to never be called!

I'm surprised this ever worked. I think what might happen is that on a real device we actually register with the push service which kicks the FxALoginHelper to continue onwards to call the success callback. The simulator doesn't support push which might be a reason why this work on device but not on the simulator.

Note: To replicate this, you'll need to 'Reset Content And Settings' in the simulator to reset user choice when prompted for notification settings.

:jhugman, you're probably the best person to update the FxALoginHelper flow to work with the new delegate method. Could you take a look at this?

[1] https://github.com/mozilla-mobile/firefox-ios/blob/master/Client/Helpers/FxALoginHelper.swift#L141

Comment 9

[James Hugman [:jhugman] [@jhugman]]

:sleroux On it.

The func application(application: UIApplication, didRegisterUserNotificationSettings notificationSettings: UIUserNotificationSettings) method is implemented at https://github.com/mozilla-mobile/firefox-ios/blob/master/Client/Application/AppDelegate.swift#L648

I'm looking at this flow wrt Bug 1351310

Comment 10

[Maurya Talisetti]

(In reply to Stephan Leroux [:sleroux] from comment #8)
> I'm surprised this ever worked. I think what might happen is that on a real
> device we actually register with the push service which kicks the
> FxALoginHelper to continue onwards to call the success callback. The
> simulator doesn't support push which might be a reason why this work on
> device but not on the simulator.
Stephan, it doesn't work on a real device as well. Try on the latest commit acdd7a2. However, it works on the 7.0 beta (from TestFlight) and also on 6.1 (from the AppStore).

Comment 11

[James Hugman [:jhugman] [@jhugman]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2573

Comment 12

[James Hugman [:jhugman] [@jhugman]]

Ok, got it. The swift 3.0 version of the application:didRegister*: methods were being called, and we were implementing the 2.x version.

-    func application(application: UIApplication, didRegisterUserNotificationSettings notificationSettings: UIUserNotificationSettings)
+    func application(_ application: UIApplication, didRegister notificationSettings: UIUserNotificationSettings)

-    func application(application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data)
+    func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data)

Unfortunately we can't mark these as `override`, which would help us for the _next_ update. 

PR attached.

Comment 13

[James Hugman [:jhugman] [@jhugman]]

Merged into master.

Comment 14

[Simon Basca [:SimonB]]

Verifying as fix on master b3b12590bebb48. Tested all the scenarios from the description and was unable to reproduce this issue.