Last Comment Bug 759758 - Thunderbird sometimes switches back to inbox tab after opening the Account Provisioner tab
: Thunderbird sometimes switches back to inbox tab after opening the Account Pr...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: 13 Branch
: x86 All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on:
Blocks: AccountProvisioner
  Show dependency treegraph
 
Reported: 2012-05-30 08:01 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-06-19 13:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
affected
-
affected


Attachments
Patch v1 (8.37 KB, patch)
2012-05-30 11:52 PDT, Mike Conley (:mconley) - (needinfo me!)
mozilla: review+
bwinton: feedback+
squibblyflabbetydoo: feedback+
mozilla: feedback+
Details | Diff | Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-05-30 08:01:28 PDT
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) - (needinfo me!) 2012-05-30 10:32:47 PDT
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) - (needinfo me!) 2012-05-30 11:52:22 PDT
Created attachment 628404 [details] [diff] [review]
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) 2012-05-30 23:02:01 PDT
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) (:☕️) 2012-05-31 11:39:29 PDT
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 2012-05-31 12:01:54 PDT
> > 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 2012-06-13 12:36:15 PDT
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) - (needinfo me!) 2012-06-18 07:12:21 PDT
(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 2012-06-18 07:22:39 PDT
(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) - (needinfo me!) 2012-06-18 07:26:30 PDT
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) - (needinfo me!) 2012-06-18 07:39:00 PDT
comm-central: https://hg.mozilla.org/comm-central/rev/cff415a502cb
Comment 11 Mark Banner (:standard8) 2012-06-19 13:02:56 PDT
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) - (needinfo me!) 2012-06-19 13:03:37 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.