1689724 - Folder Pane is not enabled after a successful account setup on a clean profile

Status: RESOLVED FIXED

(Thunderbird :: General, defect, P1)

Description

[Alessandro Castellani [:aleca]]

After concluding a successful first configuration, the email wizard dialog closes but the window remains on the Account Central tab.
The Folder Pane remains collapsed and the UI doesn't updated until the next restart.

This is the error message when completing the "Get new email address" setup:

JavaScript error: chrome://messenger/content/accountUtils.js,
line 422: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIMsgMailSession.topmostMsgWindow]

Comment 1

[Alessandro Castellani [:aleca]]

More info:

• The issue seems to happen only after the creation of a new email.
• When using an existing email, the UI updates but the selection of the Inbox doesn't trigger the correct loading of the remote folders.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

I think I found the problems, but I'm not sure if the possible solutions are correct.

UI not loading when an existing account is created on first run
The issue was that the Inbox folder was getting selected before the proper eventListener were set in place, so the selection wasn't triggering any loading of the remove folders or messages.

By removing those setTimeout(..., 0); the startup sequence works as expected.
Are those timeouts necessary?

UI not loading when creating a new email account
The problem is that, after the Account Wizard closes, this condition will always return false (https://searchfox.org/comm-central/rev/27183de94cdae57d6b19f02ca615726d6601c911/mail/base/content/msgMail3PaneWindow.js#757) since we're not passing through the okCallback() method expected from the emailWizard

What should we do here?

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9200161 [details] [diff] [review]
1689724-account-setup-after.diff

Review of attachment 9200161 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgMail3PaneWindow.js
@@ -949,3 @@
>    }
> -
> -  setTimeout(completeStartup, 0);

IIRC this timeout was to allow the main window to come up before the system integration dialog and such. 
I didn't try the patch yet, but as long as things still work properly I'm all for removing setTimeouts.

Ideally showing this dialog would get triggered way later. No real need to do it instantly on startup.

@@ +979,5 @@
>      Services.tm.dispatchToMainThread(() => loadStartFolder(startFolderURI));
>    }
>  
> +  reportAccountTypes();
> +  reportAddressBookTypes();

For these, I think you could put them in a idleDispatchToMainThread instead of setTimeout

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

This patch fixes the problem with UI not loading after an existing email account is configured on first run.

I also fixed the issue when a new email is created from the Account Provisioner, but there were some caveats.

• With Andrei, we created https://github.com/thundernest/static-test, so we can update the pref to point to https://static-test.thunderbird.net/providerList to simulate a new email creation process locally.
• When reaching this part: https://searchfox.org/comm-central/rev/4eb14a03690b8d6bbe53853a33b6e09d330a05ea/mail/components/newmailaccount/content/uriListener.js#189-196, the listener fails returning this error: client config xml = {"parsererror":{"sourcetext":"\u001f\b","$sourcet
• If I manually write the config.xml file like we do in the tests, the whole process completes successfully.

I suspect that we might need to update something on that new test site we created to ensure a good response, but I might be wrong and an underlying issue might be present.
Do we know of anyone actually using Ghandi and completing the process?

Since the Account Provisioner overrides the okCallback() of the emailWizard, we don't pass the verifyAccount() condition that would trigger the initialization of the UI. To fix that temporarily, I'm checking if the account provisioner is opened with the args.success attribute, meaning a new email has been created, and if the user has any account currently configured but the folders pane is still collapsed, I trigger the LoadPostAccountWizard() account.

This works only if bug 1689079 is applied.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9200572 [details] [diff] [review]
1689724-account-setup-after.diff

Review of attachment 9200572 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgMail3PaneWindow.js
@@ +902,5 @@
>    }
>  
> +  // Check whether we need to show the default client dialog.
> +  // First, check the shell service.
> +  if (Ci.nsIShellService) {

I don't think this check is a thing. Seems like a leftover from some old code. The nsIShellService interface always exists, but if there's no shell service can be created through the interface.

@@ +933,5 @@
> +        !SearchIntegration.osVersionTooLow &&
> +        !SearchIntegration.osComponentsNotRunning &&
> +        !SearchIntegration.firstRunDone)
> +    ) {
> +      window.openDialog(

For a normal account setup I now get the integration dialog on top of the account setup dialog.
I don't think this used to happen.

It seems all of this could well happen on idle (or even later) as there's no urgency at all to do it directly.

@@ +987,5 @@
> +  Services.tm.dispatchToMainThread(() => {
> +    loadStartFolder(startFolderURI);
> +    reportAccountTypes();
> +    reportAddressBookTypes();
> +  });

No need to do the telemetry reporting on any urgency, so why not the idleDispatchToMainThread?

Comment 6

[Alessandro Castellani [:aleca]]

I'm stumbling upon a problem where, after a successful account setup, the Inbox folder is selected by the messages and the other folders are not loaded. Clicking on another folder and then selecting again Inbox triggers the loading from the server.

Setting a timeout before letting the code select the Inbox folder solves this problem, but of course that's not good.
That means we probably have a race issue where something is getting configured and we select the Inbox folder too soon.

After trying to trace all the startup calls, I found out that the code stops here on first run: https://searchfox.org/comm-central/rev/19d26c50df73a50d1d26fc568f75f614dec9ddf5/mail/base/content/mailWindowOverlay.js#1940

Any suggestion?

Comment 7

[Alessandro Castellani [:aleca]]

I'm a bit out of my depth here, and it seems that the startup code handling the triggering of the email wizard is shared with seamonkey, so I don't know how much I can change.

I'm starting to think that, since this whole thing with dialogs will be dropped with the account wizard in a tab, maybe we should temporarily ignore this problem, pushing for the new wizard before the next esr.

What do you think?

Comment 8

[Magnus Melin [:mkmelin]]

If it works for this case to put MsgGetMessagesForAllServers on a timeout, that seems ok. Also add a comment that it's for a clean account so that case doesn't get missed in the future.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

All right, this works properly for both Account setup and New Email setup on first run.
I solved the issues of the system integration dialog popping on top of the emailWizard dialog, and the fodler selection happening too early.

Now the UI loads correctly for every scenario, I think.
This whole timeout based process will be dropped entirely after we implement the Account Creation in a Tab, so this is mostly temporary just to not ship glitchy betas when setting up a new account.

Here's a try run to see if I destroyed something: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e73c35d8650cc1d9512bacc4f0fe68770b6be7bf

Comment 10

[Alessandro Castellani [:aleca]]

As I expected, lots of test failures.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

I fixed all the tests failing, at least locally.
Let's see: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a11707574fde499391748cb0458edb4bf2ee7f64

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9202192 [details] [diff] [review]
1689724-account-setup-after.diff

Review of attachment 9202192 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like  comm/mail/test/browser/newmailaccount/browser_newmailaccount.js fails. Maybe comm/mail/test/browser/quick-filter-bar/browser_displayIssues.js as well

::: mail/base/content/msgMail3PaneWindow.js
@@ +887,5 @@
> +  // event liteners, otherwise we risk to select the Inbox folder without
> +  // triggering the loading of the rest of the account on first startup.
> +  setTimeout(selectFirstFolder, 0);
> +
> +  // FIX ME - later we will be able to use onload from the overlay.

I think we can remove this. Have no idea what it's supposed to refer to anymore.

@@ +914,5 @@
> +/**
> + * Check if we need to show the system integration dialog before notifying the
> + * application that the startup process is completed.
> + */
> +function completeStartup() {

we could rename this to be about the system integration for clarity.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

So, apparently the application needs at least a loadStartFolder(null) on startup in order to function, that's why those tests were failing.
I was trying to be smart and interrupt the selection of the start folder in case we didn't pass some conditions, but without that, no proper initial focus/selection is triggered.

Patch updated to keep the same condition structure to select the initial folder.

Another try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=659d5e9774151d77b5e93ec9b89d328a156fb127

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9202482 [details] [diff] [review]
1689724-account-setup-after.diff

browser_newmailaccount.js is still failing

Comment 15

[Magnus Melin [:mkmelin]]

Just to note that the test case added in bug 1693883 had a TODO item to test this bug (at the TODO item, the folder pane should again start showing)

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

I'm a bit lost with this.
I can't understand why that browser_newmailaccount.js test fails with:
Found an unexpected alert:alert at the end of test run
I'm not able to make the test fail locally, and it passes on macOS in the try-server.

I also run the test with --verify and that had a bunch of different failures, which I fixed, except for a failure that involves the missing Services.logins on a second run.
Removing the Services.logins.removeAllLogins(); method added in bug 1693883 makes the test succeed with --verify but I'm not sure if we should remove it.

I'm not sure where to look and what to try here.

Comment 17

[Alessandro Castellani [:aleca]]

I think I found the possible issue.
I just noticed that my Notification panel in Ubuntu showed multiple notifications with Daily failed to connect to server imap-provisioned.example.com.

I suspect this is what that alert:alert reported in the test failure is about, as I think on Linux and Windows those are recognized as system alerts, while on macOS are simply ignored as notifications.

How do we solve this?

Comment 18

[Alessandro Castellani [:aleca]]

I was able to fix the failing tests with the alert:alert issue by implementing a MockAlertService, bu now I'm stumbling up on an old test failure fixed in bug 1663209.

Upon investigating, I found out that the changes in that bug are what caused this regression.
Without the setTimeout() set to 0, the folder is selected too soon after a successful account setup and the initial download of the inbox is not properly triggered.

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

Hey Geoff, I'm pinging you to see if you have insights on this.

As I wrote before, removing the setTimeout wrapper for the selectFirstFolder() causes the app to select the first folder without triggering the loading of the messages or the server folders on first run.

With this patch, the startup process after a successful setup completes correctly, but of course it causes an issue in this test that you previously fixed: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=133a6cc889b62408e66caccd1eb942f8f55a37e8&selectedTaskRun=VapsGTWvSMGGjEVy3JDgaA.0

I'm a bit stuck with this patch and I'm not sure what to do here as it seems that the FolderPaneSelectionChange() method doesn't do its job if the Inbox folder is selected too soon on a fresh install.

Comment 20

[Geoff Lankow (:darktrojan)]

Replace:

setTimeout(selectFirstFolder, 0);

with:

Services.tm.dispatchToMainThread(() => selectFirstFolder());

That seems to do the trick. I've had a fight with setTimeout here before (bug 1663209) but I've never really understood the problem.

Comment 21

[Alessandro Castellani [:aleca]]

I tried that, sorry, I should have wrote that in my previous message.
Using Services.tm.dispatchToMainThread(() => selectFirstFolder()); doesn't properly trigger the load of the server at the end a new account setup.

Comment 22

[Geoff Lankow (:darktrojan)]

Found it! There are two things calling getNewMessages and the back end is getting confused.

• Here in the email wizard.
• Here in loadStartFolder.

Of the two, I'd remove the first because it shouldn't be up to the wizard to fetch new mail.

Comment 23

[Geoff Lankow (:darktrojan)]

Comment on attachment 9205289 [details] [diff] [review]
1689724-account-setup-after.diff

Review of attachment 9205289 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't looked at this all that closely, but enough to figure out what you're up to, and it seems okay to me.

::: mail/base/content/msgMail3PaneWindow.js
@@ +836,5 @@
> + * in case a user configures a new email account on first run.
> + *
> + * @param {boolean} isFromProvisioner - True if the method was called from the
> + *   New Account Provisioner. This is used to avoid triggering the system
> + *   integration dialog since the New Account Provinsioner uses a secondary

Provinsioner! :-)

Comment 24

[Alessandro Castellani [:aleca]]

(In reply to Geoff Lankow (:darktrojan) from comment #22)

Found it! There are two things calling getNewMessages and the back end is getting confused.

• Here in the email wizard.
• Here in loadStartFolder.

Of the two, I'd remove the first because it shouldn't be up to the wizard to fetch new mail.

Ah! Here's the culprit.

The email wizard one doesn't really do anything as it doesn't actually trigger the loading of new folders and messages.
We need to keep that as it's necessary for 2 scenarios:

• On first startup when no accounts are configured, the email wizard needs to trigger the fetching after a successful setup because selecting the newly created Inbox folder doesn't load anything without a timeout.
• If the user already has an account and creates a new one, we won't go through the loadStartFolder.

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=92cb37a9dd17fe375e106fe5f03cdea6f7b54ac7

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

A tiny fix to properly load folders and messages of newly created accounts.
Tested this by manually creating 3 accounts in a row and everything seems to work like a charm.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9206534 [details] [diff] [review]
1689724-account-setup-after.diff

Review of attachment 9206534 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgMail3PaneWindow.js
@@ +875,5 @@
> +    setTimeout(showSystemIntegrationDialog, 0);
> +  }
> +
> +  let safeMode = document.getElementById("helpSafeMode");
> +  let appSafeMode = document.getElementById("appmenu_safeMode");

would move these down into the if, where they are used

@@ +924,5 @@
> +  } catch (ex) {}
> +  let defaultAccount = accountManager.defaultAccount;
> +
> +  // Try loading the search integration module.
> +  // We'll get a null SearchIntegration if we don't have one.

I don't think this is true. We ship SearchIntegration.jsm for all platforms. It doesn't DO anything on linux.
if the module is not there the import will throw.

::: mail/test/browser/newmailaccount/browser_newmailaccount.js
@@ +77,5 @@
>  var kSuggestFromNamePref = "mail.provider.suggestFromName";
>  var kProviderListPref = "mail.provider.providerList";
>  var kDefaultServerPort = 4444;
>  var kDefaultServerRoot = "http://localhost:" + kDefaultServerPort;
> +var kDefaultEngine;

use g prefix for when you're just storing a global variable that is not a constant

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1689724-account-setup-after.diff

Thanks for the review.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/853c656dce05
Fix UI not updating after a new account setup. r=mkmelin