sunny Reporter
	Description • 9 years ago

[1.Description]:
[Nexus5 v2.2] [Flame v2.2 ]Tap drawer button on mail main view, tap  triangle symbol to expand account list, then select none current account to switch to another account, it go back to mail main view, but it cannot change accounts successfully.
Time:18:40
See attachment:logcat_1840.txt and 1840.mp4.

[2.Testing Steps]: 
Precondion:The device has two or more E-mail accounts logged in.
1.Tap "E-Mail' app.
2.Tap the button on the left top of the device.
3.Tap  triangle symbol to expand account list.
4.Select none current account.
5.Repeat step2 ~step4 several times.

[3.Expected Result]: 
4.You could change accounts successfully.

[4.Actual Result]: 
4.You cannot change accounts successfully and menu disappeared.

[5.Reproduction build]: 
N5 v2.2 build (Affected):
Build ID               20150407162504
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f86ddb7f719
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150407.195148
Firmware Date          Tue Apr  7 19:52:02 EDT 2015
Bootloader             HHZ12d

Nexus5 v3.0 (Unaffected)
Build ID               20150407160201
Gaia Revision          84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gaia Date              2015-04-07 17:33:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150407.192944
Firmware Date          Tue Apr  7 19:30:00 EDT 2015
Bootloader             HHZ12d

Flame v2.2 (Affected):
Build ID               20150407162504
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f86ddb7f719
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.195227
Firmware Date          Tue Apr  7 19:52:39 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
occasionally Recurrence,5/10

[7.TCID]: 
Free test

	Eric Chang [:ericcc] [:echang]
	Comment 3 • 9 years ago

FWD to PROD QA

	Andrew Sutherland [:asuth] (he/him)
	Comment 4 • 9 years ago

The log indicates the problem is:
04-08 18:40:35.151 I/GeckoDump( 1275): ERR: onerror reporting: TypeError: d.foldersSlice is null @ app://email.gaiamobile.org/js/mail_app.js : 2282


It seems like there is a window of vulnerability in model.changeAccount.  this._dieFolders() is called which nulls out foldersSlice.  foldersSlice is only re-assigned when foldersSlice.oncomplete is called.

It arguably would be better for the transition to be either:
- fully populated old account => fully populated new account (so change on the oncomplete of the new one)
- assign to this.foldersSlice immediately.  The slice exists and is valid at the time of creation, it just has no entries.

Note that while v2.2 is still using the async emitter, since it uses promises, that's unlikely to be involved in the race/probabilistic nature of this since there's no way for user input to be processed between a call to emit() and when it actually fires.  This is just the asynchrony of the main thread and the worker thread,

It's also possible we should guard the actual site of the exception, but in general I think it makes sense to always have a foldersSlice around and then we can avoid the guards.  The slice will generate events as things are actually added; oncomplete is just a nicety.

	James Burke [:jrburke]
	Comment 5 • 9 years ago

This response is in two parts: first part is to :asuth, second part is about this bug, which I think is different than what asuth found with the logcat message:

---
:asuth:

Line 2282 in the optimized mail_app on latest v2.2 branch is in message_list.onNewMail, the first line:

    var inboxFolder = model.foldersSlice.getFirstFolderWithType('inbox');

onNewMail is triggered from this sequence:

sync: does a sync and finds new messages:
    if (currentAccount.id === result.id && !document.hidden) {
      model.notifyInboxMessages(result);
      return;
    }

which leads to model.notifyInboxMessages:
      if (accountUpdate.id === this.account.id) {
        model.emit('newInboxMessages', accountUpdate.count);
      }

and since message_list does:
    model.on('newInboxMessages', this.onNewMail);

Leading to the error in the report.

So my initial thought is that in model.changeAccount, we immediately assign the foldersSlice via this sort of change

  var foldersSlice = this.api.viewFolders('account', account);
+   this.foldersSlice = foldersSlice;
  foldersSlice.oncomplete = (function() {
-    this.foldersSlice = foldersSlice;
    this.foldersSlice.onchange = this.notifyFoldersSliceOnChange.bind(this);
    this.selectInbox(callback);
    this._callEmit('foldersSlice');
  }).bind(this);

will still have problems since message_list.onNewMail wants:

    var inboxFolder = model.foldersSlice.getFirstFolderWithType('inbox')

to synchronously return the first inbox folder. However, the foldersSlice was just created and may not have a value yet. The next line in onNewMail accesses inboxFolder.id, but that will fail since getFirstFolderWithType from FoldersViewSlice can return null.

By waiting to assign this.foldersSlice to when the slice was complete, it was then assumed these sorts of sync APIs would work out well.

With the evt.js sync emit change that is on master, I do not believe this pathway would error out, since the steps above would happen synchronously, and the current account/folder would still be active for the check, since the sync module checks currentAccount.id to start the emit.

So definitely a data point to reinforce sync emitting and not the silly async thing I had in my head before.

---

However, I think that logcat error is just a happy coincidence and not tied to the issue reported in this bug. If that onNewMail notification path errors out, it only affects that sync notification, and given that the emit is async, not affecting sync completion or for the purposes of this bug, not breaking account switching.

Further backing evidence: sync is a very periodic thing and would not explain a 5/10 repro rate reported for this bug.

Looking at the video, I am not sure that video shows the problem reported here? I see it switch accounts back and forth between two accounts:

Account 1: starts with "testforme", first two emails: "You" and "HTC"
Accoutn 2: starts with "guang" first two emails: "You" and "You"

So account one has an HTC email in it, and that is how I tracked the account change in the video.

The video seems to show switching account successfully. Maybe the issue is this:

There is definitely a tap target hazard with the "Inbox" being just below the account settings. A visual, where the > is the currenct account shown:

Accounts
---------------
> account 1
---------------
 account 2
---------------
Inbox (if this is tapped, will go to account 1 inbox)
---------------
Drafts
---------------
...


If the current account is the first account in the list, and the user goes to tap on the second account, a slight tap below account 2 will just hit the Inbox for account 1, and it will appear as if it did not change, the message_list will continue to show Account 1's Inbox as that is where the tap was registered.

This can be confirmed by going back to the Menu icon and showing the accounts again, it should show that account 1 is still the right account.

Looking at the video, the taps for account 2 do seem positioned low where they could just register as the Inbox item tap.

Asking the original reporter if this is perhaps the issue.

If so, a possible fix might be to just not close the menu if the tapped folder is the same as the one currently being shown. I am not sure that is enough feedback to the user though, maybe they will then think the folder picker is broken? Probably OK though, but would want UX to weigh in for final answer.

This would be an issue that affects both master and 2.2, and while I would like these interactions to be as clear as possible, also not something of a regression as this is how it has operated for a while, so not sure if it qualifies as blocking 2.2.

	James Burke [:jrburke]
	Comment 6 • 9 years ago

Forgot the ni? on original reporter, please see comment 5

	Andrew Sutherland [:asuth] (he/him)
	Comment 7 • 9 years ago

Oh, then this might just be another variant of bug 1148350 where the wrong folder is selected on v2.2 and not v3.0, presumably due to differences in APZ.  There's some preliminary bisection in bug 1148350 comment 3 to start from.

jrburke, Sorry for the red herring log analysis!  And yeah, the "the folder doesn't exist yet" stuff is a doozy.  The convoy branch has promises for that though, hooray!

	Shally Li（leave from Mozilla）
	Comment 8 • 9 years ago

(In reply to James Burke [:jrburke] from comment #5)
> 
> Asking the original reporter if this is perhaps the issue.

Hi James,

    You can see video at 00:13~00:16 where the bug said:  When tapping the account name in menu list (or triangle symbol), the accounts list does not unfold, and menu list retracts directly to backs to "Inbox" page of current account (not 100%). The accounts list should unfold when user want to choose another account.

Thank you very much.

	James Burke [:jrburke]
	Comment 9 • 9 years ago

OK, so the issue is that tapping on the account name may not expand the list to show the list of accounts and change that section to start with "Accounts". Instead, it just closes the menu.

I believe then that this is an APZ issue then, similar to bug 1148350. I confirmed in the dev tools the onclick listener is on the whole account list item (the white area showing the account name or "Accounts" when opened).

	James Burke [:jrburke]
	Comment 10 • 9 years ago

Going to pass this over to the APZ folks for some investigation. Perhaps the issue is related to the slide down animation and the tap, if timed right, seems to be registered for the the item below the account item, and on the Inbox item. This is for 2.2.

In comment 7, :asuth mentions a preliminary bisection in the related bug 1148350 comment 3 to start from.

	Botond Ballo [:botond]
	Comment 11 • 9 years ago

I requested a fix window in bug 1148350 - let's see what that turns up and take it from there.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 12 • 9 years ago

triage: basic function break

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 9 years ago

ni? reporter to see if this happens in the next B2G nightly build. I suspect it should be fixed by bug 1148350 which just got merged to m-c.

	sunny Reporter
	Comment 14 • 9 years ago

Now this bug can be still repro. Bug 1148350 has been verified in master, and once it is fixed in v2.2, I will see this bug again whether it is reproducible or not.

	Bobby Chien
	Comment 15 • 9 years ago

waiting verification in bug 1148350.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 16 • 9 years ago

Assigning while awaiting the verification of bug 1148350.

	Shally Li（leave from Mozilla）
	Comment 17 • 9 years ago

After the bug 1148350 had fixed and verified, the current bug can not be reproduced anymore on latest build of Flame v2.2 and Nexus5 v2.2 by the STR in Comment 0.

Actual results: The accounts list can be unfolded and then can change to "Inbox" folder of another account.
See attachment: verified_v2.2.mp4
Reproduce rate: 0/5

------------------------------------------------------------
Device: Flame 2.2 build(Pass)
Build ID               20150506002501
Gaia Revision          772a9491909abd02dc67278dd453746e2dd358a8
Gaia Date              2015-05-05 02:02:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3af6a0a79227
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150506.040209
Firmware Date          Wed May  6 04:02:20 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 v2.2 build (Pass)
Build ID               20150506002501
Gaia Revision          772a9491909abd02dc67278dd453746e2dd358a8
Gaia Date              2015-05-05 02:02:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3af6a0a79227
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150506.040123
Firmware Date          Wed May  6 04:01:41 EDT 2015
Bootloader             HHZ12f

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 19 • 9 years ago

Great, thanks!