759758 - Thunderbird sometimes switches back to inbox tab after opening the Account Provisioner tab

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This happens randomly it seems, but here are the STR:

1. Create a new Thunderbird profile
2. When the Account Provisioner dialog comes up, enter a search, and then choose an address when you find one.
3. Wait for the order form tab to open

What happens (sometimes)?

The inbox tab gets switched to suddenly, and the order form tab goes into the background.

What's expected?

We should fully focus the order form tab.

I can't seem to reproduce this reliably, so my guess is that this is a race of some kind.

I tried fixing this in an earlier patch by increasing the delay time for when the Account Provisioner dialog opens to 200ms to give the 3pane time to spin, but I guess that's sometimes not enough.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Ok, I think I see how this race is happening:

OnLoadMessenger calls verifyAccounts, which then calls AutoConfigWizard, which hooks up a 200ms setTimeout to spawn the Account Provisioner dialog.

Once that's all done, LoadPostAccountWizard gets called, which at its conclusions hooks up yet *another* setTimeout (this time for 0ms) to run loadStartFolder.

loadStartFolder then calls atStartupRestoreTabs, which calls tabmail's restoreTabs, which automatically focuses the 0'th tab.

So those setTimeout's are where we're seeing the race - sometimes the Account Provisioner dialog opens *before* atStartupRestoreTabs is called, and sometimes after. When it opens before, atStartupRestoreTabs is blocked, so that Account Provisioner can open the order form tab before atStartupRestoreTabs gets fired.

And that's when the focus switch happens.

Investigating possible solutions...

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So here's my first shot at this.

Instead of setting some arbitrary timeout, I've embedded a new observer notification "mail-tabs-session-restored" that's fired once the tabs session has been restored.

When we call NewMailAccountProvisioner, instead of opening the modal dialog immediately, we register an observer to fire on the mail-tabs-session-restored notification that opens the dialog.

That way, if the dialog decides to open an order form tab, we know that the tabs have already been restored from before, and we're not going to accidentally switch focus.

So, a few feedback questions:

1)  Is the nsIObserver notification the way to go? That seems to be the way things are done in msgMail3PaneWindow.js, but I think I prefer the method that squib used in the compose window for Filelink - firing local document events as opposed to global nsIObserver notifications.

Or should we just stick with the pre-existing pattern in msgMail3PaneWindow.js?

2) In order for the tabs to get restored, I need to fire the okCallback that gets passed to AutoConfigWizard (which is LoadPostAccountWizard).

Is that OK? Are bad things possible if we fire LoadPostAccountWizard before any accounts have been set up?

Are bad things possible if the user switches from the Account Provisioner to the Existing Account Wizard to create an account, and LoadPostAccountWizard has already fired?

Comment 3

[Jim Porter (:squib)]

Comment on attachment 628404 [details] [diff] [review]
Patch v1

I can't say much about the account provisioner code, as I'm not familiar with it, but the other parts look sane to me. I'm always a fan of more events/notifications to help simplify our monolithic onLoad functions. :)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 628404 [details] [diff] [review]
Patch v1


> 1)  Is the nsIObserver notification the way to go? That seems to be the way things are done in
> msgMail3PaneWindow.js, but I think I prefer the method that squib used in the compose window for
> Filelink - firing local document events as opposed to global nsIObserver notifications.
> Or should we just stick with the pre-existing pattern in msgMail3PaneWindow.js?

I really don't know.  Perhaps Bienvenu has an opinion on this?

> 2) In order for the tabs to get restored, I need to fire the okCallback that gets passed to
> AutoConfigWizard (which is LoadPostAccountWizard).
> 
> Is that OK? Are bad things possible if we fire LoadPostAccountWizard before any accounts have been
> set up?

I certainly don't think it intended for that, and the migration code in it makes me worry a little.

> Are bad things possible if the user switches from the Account Provisioner to the Existing Account
> Wizard to create an account, and LoadPostAccountWizard has already fired?

I think you'ld be safe here, since we already handle that case when we launch the Account Provisioner or Account Wizard from the menu.

So, I guess f=me for the parts that I can comment on, but I'm going to mostly pass the buck, too.

Later,
Blake.

Comment 5

[David :Bienvenu]

> > Or should we just stick with the pre-existing pattern in msgMail3PaneWindow.js?
> 
> I really don't know.  Perhaps Bienvenu has an opinion on this?

I think either way is fine - local document events might be somewhat cleaner but I'd say it's not a big deal either way.
> 
> > 2) In order for the tabs to get restored, I need to fire the okCallback that gets passed to
> > AutoConfigWizard (which is LoadPostAccountWizard).
> > 
> > Is that OK? Are bad things possible if we fire LoadPostAccountWizard before any accounts have been
> > set up?
I think we're allowed to not have any accounts post account wizard...

Comment 6

[David :Bienvenu]

Comment on attachment 628404 [details] [diff] [review]
Patch v1

looks OK...I did see a test failure in the newmailaccount tests (crash on exit, I think) but I can't reproduce it.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to David :Bienvenu from comment #6)
> Comment on attachment 628404 [details] [diff] [review]
> Patch v1
> 
> looks OK...I did see a test failure in the newmailaccount tests (crash on
> exit, I think) but I can't reproduce it.

Hm...does that warrant more investigation then?

Comment 8

[David :Bienvenu]

(In reply to Mike Conley (:mconley) from comment #7)
> (In reply to David :Bienvenu from comment #6)
> > Comment on attachment 628404 [details] [diff] [review]
> > Patch v1
> > 
> > looks OK...I did see a test failure in the newmailaccount tests (crash on
> > exit, I think) but I can't reproduce it.
> 
> Hm...does that warrant more investigation then?

I don't think it's specific to this patch. I've seen it with other patches.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 628404 [details] [diff] [review]
Patch v1

I'm a little hesitant to land something that re-organizes our start-up on aurora or beta... can we live with this bug until TB 16?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/cff415a502cb

Comment 11

[Mark Banner (:standard8)]

Per drivers meeting today, as this affects startup, we'll let it ride the trains. AFAIK we've not seen reported instances of it.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mark Banner (:standard8) from comment #11)
> Per drivers meeting today, as this affects startup, we'll let it ride the
> trains. AFAIK we've not seen reported instances of it.

Cool.