950045 - [Email] Canceling the 'Setting Up Account' screen doesn't cancel operation, will progress regardless; can stack multiple instances

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[William Hsu [:whsu]]

Attachment: WP_20131213_008.mp4

* Description:
  Trying to tap "Cancel" button while email is setting up a email account.
  You will see that the email app still continue to set up the email account
  Attach the video (WP_20131213_008.mp4)

* Reproduction steps:
  1. Please make sure there is no email account on email app
  2. Launch the email app
  3. Input the "Name", "Account", "Password" then tapping "Next" icon
  4. Tapping "Cancel" button while email app is setting up the email account

* Expected result:
  Email app stops set email account

* Actual result:
  Email app continues to set email account

* Test build:(V1.3)
 - Gaia:     588a3e02c4ace3b3341ba1f6bb7274120b53b2b3
 - Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/031270be3702
 - BuildID   20131212004003
 - Version   28.0a2

Comment 1

[Dylan Oliver [:doliver]]

triage: not blocking, not likely to be prioritized for fix in the near future. graveyarding.

Comment 3

[Oliver Nelson [:oliverthor]]

This bug has some additional behavior when attempting to setup an additional account after the 'failed' cancel which yields some additional attention. I'm presently unauthorized to view the bug 1107442 listed by :asuth in Comment 2 so I'm uncertain if any of this has been disclosed already.

Description:
When a user signs in an email address for the first time, via manual or traditional setup, they will observe that after setting their information and tapping 'Next', a notification will cover the screen to load "Setting Up Account"; if the user attempts to cancel this with the 'Cancel' button, they close the notification but the setup will progress regardless and push the user to the next setup screen after a few seconds. 
If the user were to tap 'Next' again from the setup screen after cancelling the setup, they can stack multiple instances of setup and create duplicate accounts in their 'Email' app.
This bug leads to additional issues:
1) Attempting to change emails in your drawer can fail and just close the drawer list instead of the drop-down for selecting a different email.
2) Attempting to sign up a new gmail account afterwards manually bricks the phone at the google sign-in; must remove battery.
   
Repro Steps:
1) Update a Flame device to BuildID: 20150109010206
2) Open the 'Email' App.
3) Type a new email address and name.
4) Tap 'Manual Setup'.
5) Fill out the fields on this screen; don't tap 'Next'.
6) Tap 'Next' and observe the 'Setting Up Account' notification load. Tap cancel on this screen before it dismisses.
7) Located within the 'Manual Setup' screen again, tap 'Next' again and 'Cancel' when it shows.
8) Wait after this 2nd cancel and observe UI.
  
Actual:
User is progressed through Setup of account despite canceling the screen. If stacked with another 'Next' command, user will create a duplicate account of the same email.
  
Expected: 
Canceling the screen
  
Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150109010206
Gaia: 5f0dd37917c4a6d8fa8724715d4d3797419f9013
Gecko: b3f84cf78dc2
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
  
Repro frequency: 5/5
See attached: 
video- http://youtu.be/WIUpBlqVD_k
log

Comment 4

[Peter Bylenga [:PBylenga]]

NI on Component owner, bug has gotten worse and breaks functionality of adding additional email accounts.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Oliver Nelson [:oliverthor] from comment #3)
> 2) Attempting to sign up a new gmail account afterwards manually bricks the
> phone at the google sign-in; must remove battery.

This sounds most serious but also sounds like it may be revealing an underlying platform or system/window manager bug rather than an email app bug.  (Even if we create two mozbrowser instances, it shouldn't cause problems with the rest of the system.)  Is there a logcat from this run?

Comment 6

[Peter Bylenga [:PBylenga]]

Oliver can you attach a logcat with the necessary debugging info for Email?

Comment 7

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150109_brickedSignin.txt

Repeated the steps I logged just previously, created 3 gmail accounts through one manual setup session [next, cancel, next, cancel, next]. Went to settings and saw 3 accounts of the same name, navigated to create a new account of the same name ("[e-mail]"), input new name + email address and hit next (no manual entry this time). 
The google UI loads in to progress the standard login then all input is removed from the user once it loads.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] rsajdok:bug#950045 > mozilla-b2g:master

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

I'll leave this to jrburke to review, but this seems like a nice simple fail-safe fix!  I think we still want to spin off a back-end enhancement bug to try and abort the process earlier (and avoid the network traffic and possibly the device-id registration for ActiveSync, etc.), but given the comparative complexity of addressing this in the back-end and the chance for all of that to get overhauled by the conversations refactoring, this seems like a good/fine thing to land now.

My main back-end concern would be that deleteAccount wasn't particularly engineered to deal with there being a lot of stuff going on when it deleted the account.  But quickly skimming the code, both localOpCompleted/serverOpCompleted will quickly crash-and-burn on completion when they try to dereference the deleted _opsByAccount entry's innards.  This should effectively ensure that syncFolderList will end up harmless, especially since we did take care to ensure that a dead account won't resurrect itself.  IMAP connection creation isn't guarded against on a dead account, but since IMAP connections will self-close on idle timeout, they should also be safe.  So in summary, I think the back-end will be fine with this, although we would expect it to be possible for some errors to occur as a side-effect of deleting an account that might have actively been doing stuff.  And in terms of specific risks from this patch, I think it's just syncFolderList we need to worry about.

Comment 11

[James Burke [:jrburke]]

Comment on attachment 8582361 [details] [review]
[gaia] rsajdok:bug#950045 > mozilla-b2g:master

This does look good to do, but there some changes I would like to see:

* Use of 'cancelCreation' as the flag conflicts with the function of the same name, so when .die() tries to call .cancelCreation() that shows an error in the log, since it is now a boolean in the failure case. So I would like to see the flag called something like 'createCanceled' or something like that.
* Related, let's explicitly set `this.createCanceled = false` in the onArgs as we do for the other flags, just to help indicate what all the flags that are in play in an instance.

Doing something like that locally, here is the logcat I get when running that change, if it helps :asuth feel good about the back end impact:

I/Gecko   ( 3556): WINF: [slog] imap:connected {"hostname":"imap.gmail.com","port":"993","crypto":"ssl"}
I/Gecko   ( 3556): WINF: [slog] probe:imap:success
I/Gecko   ( 3556): WINF: [slog] smtp:connected {"emailAddress":"[e-mail]","hostname":"smtp.gmail.com","port":"465","crypto":"ssl"}
I/Gecko   ( 3556): WLOG: [slog] probe:smtp:checking-address-validity
I/Gecko   ( 3556): WINF: [slog] probe:smtp:success
I/Gecko   ( 3556): WLOG: cronsync: ensureSync called
I/Gecko   ( 3556): 
I/GeckoDump( 3556): LOG: cronsync-main: ensureSync called
I/Gecko   ( 3556): WLOG: queueOp 1 syncFolderList pre-queues: local: 0 server: 0
I/Gecko   ( 3556): WLOG: runOp(do: {"type":"syncFolderList","longtermId":"1/0","lifecycle":"do","localStatus":"done","serverStatus":"doing","tryCount":0,"humanOp":"syncFolderList"})
I/GeckoDump( 3556): LOG: cronsync-main: success!
I/GeckoDump( 3556): LOG: cronsync-main: ensureSync completed
I/Gecko   ( 3556): WLOG: cronsync: received an syncEnsured via a message handler
I/Gecko   ( 3556): 
I/Gecko   ( 3556): WERR: [slog] disaster-recovery:exception {"accountId":"1","error":{},"errorName":"TypeError","errorMessage":"queues is undefined","stack":"MailUniverse.prototype.areServerJobsWaiting@app://email.gaiamobile.org/js/ext/worker-bootstrap.js:14221:13\nproperties.maybeCloseUnusedConnections@app://email.gaiamobile.org/js/ext/composite/configurator.js:9121:10\nproperties.__folderDoneWithConnection@app://email.gaiamobile.org/js/ext/composite/configurator.js:9294:9\nImapJobDriver.prototype._acquireConnWithoutFolder/</<@app://email.gaiamobile.org/js/ext/composite/configurator.js:3471:11\nexports.postJobCleanup@app://email.gaiamobile.org/js/ext/worker-bootstrap.js:20743:5\nrunOp/jobCompletedCallback<@app://email.gaiamobile.org/js/ext/worker-bootstrap.js:22578:5\ndeadConn@app://email.gaiamobile.org/js/ext/composite/configurator.js:4078:11\nproperties._bindConnectionDeathHandlers/conn.onclose<@app://email.gaiamobile.org/js/ext/composite/configurator.js:9264:13\nBrowserBox.prototype._init/this.client.onclose<@app://em
I/Gecko   ( 3556): WERR: *** Disastrous Error for email accountId 1 -- attempting to recover...
I/Gecko   ( 3556): WWAR: No job operation was currently running.


This just happened to be what I saw for my particular test, may see something different if timing the cancel differently.

If this looks OK from :asuth's back end point of view though, I think just doing the two fixes mentioned above should be able to address the concerns.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Not my favorite error, but it should be fine.  We're throwing while checking on the existence of jobs during the return of the folder from the job.  However, there's nothing to clean up other than that connection.  And the connection will timeout and close itself after 30 seconds.  While there are pathological situations where the gmail IMAP or other connection account limits could temporarily be saturated by this until the timeouts occur, the gmail limit is a relatively high 15.  I'm okay with telling a user who repeatedly initiates and then cancels account creation 16 times in 30 seconds that they should stop doing that.  Or just do it more slowly.

Comment 13

[James Burke [:jrburke]]

Comment on attachment 8582361 [details] [review]
[gaia] rsajdok:bug#950045 > mozilla-b2g:master

Just a small nit in the pull request, but I would like to fix it before landing, otherwise I will wonder in the future why that boolean check is special compared to the other ones. I did test on device though and it is looking good besides that. Also, apologies for the longer review times, I was on a trip, but back now.

Comment 14

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#nwywff9zRyKhH7PdGs1aiA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/a53fd3daff671d461f6a05c242db1c88cfa0a2f4

Comment 16

[helen（leave from Mozilla）]

Attachment: v3-0.3gp

Hi Josh,
This issue has been verified as pass on latest build of Flame v3.0 and Nexus 5 v3.0 by the STR in Comment 0.
And this issue is affected on latest build of Flame v2.2, too. So may I know whether it will be fixed on this version?

Results:
After tapping "Cancel" button while Email app is setting up the email account, Email app will stop setting the email account.

Attachment:v3-0.3gp
Rate:0/15

Device:Flame v3.0(Verified)
Build ID               20150624080416
Gaia Revision          eb0d4aefa62b20420d6fa0642515a110daca5d97
Gaia Date              2015-06-24 01:48:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/4cdc1a95a672
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150624.153831
Firmware Date          Wed Jun 24 15:38:44 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 v3.0 (Verified)
Build ID               20150624010204
Gaia Revision          311c4e59936a407e64509f54fecb440d8a78e3c8
Gaia Date              2015-06-20 20:21:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/be81b8d6fae9
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150624.043116
Firmware Date          Wed Jun 24 04:31:35 EDT 2015
Bootloader             HHZ12f

Comment 17

[Josh Cheng [:josh]]

Setting 2.1/2.2 wontfix as this is not blocking. Will revisit when partner request this fix at device launch.