Closed Bug 953585 Opened 10 years ago Closed 10 years ago

Add a preference when creating a new account in case it crashes

Categories

(Chat Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romain, Assigned: romain)

References

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 138 at 2008-10-02 18:14:00 UTC ***

From florian on Bug 953532 (bio 81):

"We should probably special case the automatic connection of accounts at the end of the account wizard, so that we catch the crash and disable automatic connection of the faulty account only at the next start.

Accounts that have autologin activated, are edited and then manually connected
may be considered as new (i.e. it's the first connection with these
parameters)."
Blocks: 953532
Attached patch proposed patch (obsolete) — Splinter Review
*** Original post on bio 138 as attmnt 175 at 2009-08-07 19:46:00 UTC ***

Here is a patch with the required behavior.
I describe above this patch as I read the diff to help the reviewer in his/her task.

- We want to add this feature and set the account as new when the changes affect the connection info of the account.

- We also want to set it when a new account is created (with the account wizard)

- When an account has crashed, the general autologin is not disabled, so the state of autoLoginStatus is AUTOLOGIN_ENABLED, but the value of firstConnectionState has been set to CRASHED by PurpleXPCOM.
The connectNowButton is earlier in the code because the callback is not the same when autologin is enabled.

- Added the case pcs.AUTOLOGIN_ENABLED, in case of crash due to one or a few accounts. The string is a plural string, the warning is kept since it is a crash, and the callback is a dedicated function.
This function just check which accounts are "CRASHED", with autologin enabled, and actually disconnected (we don't want to connect() on an already connected account, or if autologin has been disabled).

- Added in the buddy list an account-updated observer, its goal is to check the state of Autologin to set or unset firstConnectionState.
It is best to put it here since the window is always opened, in case of an addon modifying the autoLogin value for any reason.

- Even if it is AUTOLOGIN_ENABLED, we want to open the account Manager, to do this we need to check whether an account is marked as "CRASHED" at startup.
This function was a bit buggy, I think I have fixed it.
This function now takes an optional parameter isStarting, true only when the buddy list is loading.

- Fixed a typo in the translation of userDisabled.label and added a new notification in case of one or several "new" accounts caused a crash.

- Then I add a new attribute in purpleIAccount, and set the constants.

- On unInit() on each account, we call a function to reset the value of first connection to SET if it is currently different from NONE. (clears pending and crashed status).

- This (private) function checks and sets the value of firstConnection on unInit, Disconnected With reason, sets it to pending on connect(), and sets it to NONE when successfully connected.

- On autologin check, we disable autologin is the pref has PENDING for value (and the account is currently not connecting or not available).
Note that this function is not called in case of disabled autoLogin.

- Then the getter and setter for firstConnectionState
I had to use a cast on the getter as explained in the code.
On the setter, if the attribute is set to NONE, then we delete the preference.
If the asked value is PENDING, then we save it immediately, because it means that the account is trying to connect and can potentially crash.
other preferences are delayed with a timer.

Works as expected on GNU/Linux.
OS: Linux → All
Hardware: x86 → All
Comment on attachment 8351919 [details] [diff] [review]
proposed patch

*** Original change on bio 138 attmnt 175 at 2009-08-10 12:10:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351919 - Flags: review?(florian)
Comment on attachment 8351919 [details] [diff] [review]
proposed patch

*** Original change on bio 138 attmnt 175 at 2009-08-10 20:14:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351919 - Flags: review?(florian) → review-
*** Original post on bio 138 at 2009-08-10 20:14:40 UTC ***

Comment on attachment 8351919 [details] [diff] [review] (bio-attmnt 175)
proposed patch

>diff --git a/instantbird/base/content/instantbird/account.js b/instantbird/base/content/instantbird/account.js
>--- a/instantbird/base/content/instantbird/account.js
>+++ b/instantbird/base/content/instantbird/account.js
>@@ -309,16 +309,19 @@ var account = {
>         }
>         break;
>       default:
>         throw "unknown preference type " + opt.type;
>       }
>     }
> 
>     if (connectionInfoHasChanged) {
>+      /* This is the first time we try to connect with these parameters */
>+      if (this.account.autoLogin)
>+        this.account.firstConnectionState = this.account.FIRST_CONNECTION_SET;
Why do you set this only if autoLogin is enabled for this account?

>diff --git a/instantbird/base/content/instantbird/accounts.js b/instantbird/base/content/instantbird/accounts.js

>+      case pcs.AUTOLOGIN_ENABLED:
Add a comment here to make it obvious that we returned early in case autologin is enabled and there is no crash.

>diff --git a/instantbird/base/content/instantbird/blist.js b/instantbird/base/content/instantbird/blist.js

>-    if (aTopic == "account-connected" || aTopic == "account-disconnected") {
>+    if (aTopic == "account-connected" || aTopic == "account-disconnected" ||
>+        aTopic == "account-updated") {
>       var account = aBuddy.QueryInterface(Ci.purpleIAccount);
>-      if (account.protocol.id == "prpl-irc") {
>+      if (aTopic == "account-updated") {
>+        let newState = (account.autoLogin) ? account.FIRST_CONNECTION_SET
>+                                           : account.FIRST_CONNECTION_NONE;
>+        if (account.firstConnectionState != newState)
>+          account.firstConnectionState = newState;
>+      }
I don't understand this code. Please explain what it does, and why this is needed here rather than done in C++.
By the way, coding style nit: not () around account.autoLogin.


>+      else if (account.protocol.id == "prpl-irc") {
Do you really mean "else" here? Not sure, as I didn't understand the previous 6 lines.


>-  checkNotDisconnected: function bl_checkNotDisconnected() {
>+  checkNotDisconnected: function bl_checkNotDisconnected(isStarting) {

Most parameters have a name that start with a, so aIsStarting is better here. If the rule has been massively ignored in that file, you can keep the name as it is in your patch for consistency with the existing code.

Please add a comment before this function explaining what it does. Explain at least when and how it's called (the 2 different usecases and the fact that the parameter is optional).

>+    if (typeof(isStarting) != "boolean")
>+      isStarting = false;
I don't think you need this. If isStarting is not specified, it will have the value undefined which evaluates to false when tested.


>-    /* In case of connection failure after an automatic reconnection attempt,
>-       we don't want to popup the account manager */
>-    if (!addBuddyItem.disabled)
>+    /* We only display the account manager on startup if an account has crashed
>+       or if all accounts are disconnected */
>+    if (isStarting && (!hasActiveAccount || hasCrachedAccount))
>       menus.accounts();

I know the previous code was bogus (thanks for noticing and fixing this by the way), but it was meant to display the account manager when after a disconnection, the application end up in a situation where no account is connected at all.

I think the test you wanted to write here was:
  if (!hasActiveAccount || (isStarting && hasCrachedAccount))


>diff --git a/purple/purplexpcom/public/purpleIAccount.idl b/purple/purplexpcom/public/purpleIAccount.idl

Add a comment explaining how/when all of this is used. Explain each value, especially, when each value is expected.
I think I would have defined the constants before the attribute, but that doesn't really matter.

>+  attribute short firstConnectionState;
>+
>+  const short FIRST_CONNECTION_NONE = 0;
>+  const short FIRST_CONNECTION_SET = 1;
>+  const short FIRST_CONNECTION_PENDING = 2;
>+  const short FIRST_CONNECTION_CRASHED = 3;

Are we sure we will never want to set multiple states at once?
If in doubt, I suggest you use 4 instead of 3 as the value, so that we can decide later.

>diff --git a/purple/purplexpcom/src/purpleAccount.cpp b/purple/purplexpcom/src/purpleAccount.cpp

> #define PREF_AUTOLOGIN          "autoLogin"
>+#define PREF_FIRSTCONNECTIONSTATE       "firstConnectionState"
> #define PREF_PASSWORD           "password"
> #define PREF_REMEMBER_PASSWORD  "rememberPassword"

FIRSTCONNECTIONSTATE isn't very readable.
Why not FIRST_CONNECTION_STATE, like for REMEMBER_PASSWORD?
And you don't need that many spaces if you want align it anyway.

(yeah, I know AUTOLOGIN is inconsistent here)

 
>+  // If the first connection was pending on quit, we set it back to "set"
>+  CheckAndSetFirstConnectionState(FIRST_CONNECTION_SET);

What does "set" mean? I guess it will be more understandable once you document it in the idl file ;).

> inline void purpleAccount::CheckAndSetFirstConnectionState(const PRInt16 aState)
Add a comment before this function. Explain what it does and why/when someone would want/need to call it.

> nsresult purpleAccount::checkAutoLogin()
> {
>   /* If we don't have a valid protocol, we can't autologin so don't
>      bother checking the value of the pref */
>   if (!mHasValidProtocol)
>     return NS_OK;
> 
>+  /* We previously crashed on a single connect attempt */
>+  PRInt16 currentState;
>+  if (NS_SUCCEEDED(GetFirstConnectionState(&currentState)) && 
>+      currentState == FIRST_CONNECTION_PENDING &&
>+      (!mAccount->gc || mAccount->gc->state != PURPLE_CONNECTING)) {
>+    SetFirstConnectionState(FIRST_CONNECTION_CRASHED);
>+    return NS_OK;
>+  }

Are you 100% sure using mAccount is safe here?
I think I'd add a PURPLE_ENSURE_INIT(INIT_CONDITION); line at the beginning of the method just to be completely sure it won't crash.

Why do you test that the account is "not connecting" here?
(I ask this question just to be sure I understand and to be sure you are actually doing what you think you do. This may be totally right.)


>+/* attribute short firstConnectionState; */
>+NS_IMETHODIMP purpleAccount::GetFirstConnectionState(PRInt16 *aState)
>+{
>+  PURPLE_ENSURE_INIT(!mKey.IsEmpty());
>+
>+  nsresult rv = GetPrefBranch();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // GetIntPref takes a *PRInt32 as second argument and aState is *PRInt16
>+  PRInt32 prefState;
>+  rv = mPrefBranch->GetIntPref(PREF_FIRSTCONNECTIONSTATE, &prefState);
Empty line here (before a comment).
>+  /* If the pref does not exist, this is not the first connection */
>+  if (NS_FAILED(rv))
>+    prefState = FIRST_CONNECTION_NONE;
And here (after a conditional block).
The idea here is that we put an empty line to separate each 'logical' unit. The units tend to be smaller in C++. Often just call something and check or return the result.

>+  *aState = static_cast<PRInt16>(prefState);
>+  return NS_OK;
>+}
>+NS_IMETHODIMP purpleAccount::SetFirstConnectionState(PRInt16 aState)
>+{
>+  PURPLE_ENSURE_INIT(mProtocol);
>+
>+  nsresult rv = GetPrefBranch();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (aState != FIRST_CONNECTION_NONE)
>+    rv = mPrefBranch->SetIntPref(PREF_FIRSTCONNECTIONSTATE, aState);
>+  else
>+    rv = mPrefBranch->DeleteBranch(PREF_FIRSTCONNECTIONSTATE);
>+
You don't need this one.
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // We want to save this pref immediatly when trying to connect
>+  if (aState != FIRST_CONNECTION_PENDING)
>+    initSavePrefsTimer();
>+  else
>+    savePrefsNow(nsnull, nsnull);

I suggest you change the test and put the savePrefsNow call first, so that it is just after the comment that explains it.

>diff --git a/purple/purplexpcom/src/purpleAccount.h b/purple/purplexpcom/src/purpleAccount.h
>--- a/purple/purplexpcom/src/purpleAccount.h
>+++ b/purple/purplexpcom/src/purpleAccount.h
>@@ -91,16 +91,17 @@ private:
>   nsresult storeAccount(PRUint32 aKey, const nsCString& aName, const nsCString& aPrpl);
>   nsresult unstoreAccount();
>   nsresult SetBoolPref(const char *aName, PRBool aValue);
>   nsresult SetIntPref(const char *aName, PRInt32 aValue);
>   nsresult SetStringPref(const char *aName, const char *aValue);
>   inline void GetBoolPref(const char *aName);
>   inline void GetIntPref(const char *aName);
>   inline void GetCharPref(const char *aName);
>+  inline void CheckAndSetFirstConnectionState(const PRInt16 aState);

Or maybe the comment should be here?
Attached patch Patch V2 (obsolete) — Splinter Review
*** Original post on bio 138 as attmnt 183 at 2009-08-10 23:16:00 UTC ***

> Why do you set this only if autoLogin is enabled for this account?
Because if autologin is disabled, then if it crashed right after it won't make it crash on the next startup too.

> I don't understand this code. Please explain what it does, and why this is
> needed here rather than done in C++.
Moved to C++, on SetAutoLogin
It resets the value of firstConnectionState when autologin is changed

> I think the test you wanted to write here was:
>  if (!hasActiveAccount || (isStarting && hasCrachedAccount))
The account manager was opening at every connection retry, so I reused a part of the ancient code here and restored your comment.

> Why do you test that the account is "not connecting" here?
> (I ask this question just to be sure I understand and to be sure you are
> actually doing what you think you do. This may be totally right.)
Added a long comment explaining it, it is a very very special case but if we don't handle it it could result with 2 crashes for the user.
Attachment #8351927 - Flags: review?(florian)
Comment on attachment 8351919 [details] [diff] [review]
proposed patch

*** Original change on bio 138 attmnt 175 at 2009-08-10 23:16:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351919 - Attachment is obsolete: true
Assignee: nobody → romain
Status: NEW → ASSIGNED
Attached patch Patch V3 (obsolete) — Splinter Review
*** Original post on bio 138 as attmnt 186 at 2009-08-12 19:17:00 UTC ***

Changes:

- Automatically select the last crashed account when the account manager is opened.
- Display an error message on each crashed account (ERROR_CRASHED = 43)
- Removing a typo fix (included in a fix on Bug 953659 (bio 213))
Attachment #8351930 - Flags: review?(florian)
Comment on attachment 8351927 [details] [diff] [review]
Patch V2

*** Original change on bio 138 attmnt 183 at 2009-08-12 19:17:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351927 - Attachment is obsolete: true
Attachment #8351927 - Flags: review?(florian)
Attached patch Patch V4 (obsolete) — Splinter Review
*** Original post on bio 138 as attmnt 197 at 2009-08-16 10:56:00 UTC ***

Updated patch to fit the major refactoring (bug 953626 (bio 179)).
I also added two lines to set the focus back on the selected account after a click on "Connect Now".
Attachment #8351941 - Flags: review?(florian)
Comment on attachment 8351930 [details] [diff] [review]
Patch V3

*** Original change on bio 138 attmnt 186 at 2009-08-16 10:56:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351930 - Attachment is obsolete: true
Attachment #8351930 - Flags: review?(florian)
Comment on attachment 8351941 [details] [diff] [review]
Patch V4

*** Original change on bio 138 attmnt 197 at 2009-08-23 11:12:19 UTC ***

>diff --git a/instantbird/base/content/instantbird/account.js b/instantbird/base/content/instantbird/account.js
>     if (connectionInfoHasChanged) {
>+      /* This is the first time we try to connect with these parameters */
>+      if (this.account.autoLogin)
>+        this.account.firstConnectionState = this.account.FIRST_CONNECTION_SET;

>diff --git a/instantbird/base/content/instantbird/accountWizard.js b/instantbird/base/content/instantbird/accountWizard.js

>+    if (autologin)
>+      acc.firstConnectionState = acc.FIRST_CONNECTION_SET;

I don't think the value of the pref should depend on whether autoLogin is enabled for the account.
When autoLogin is disabled, the value won't be used, but the extra code complexity for not computing it in this case is probably not worth it.

Also, when possible, setting the value to FIRST_CONNECTION_SET should be done in purplexpcom, so that extension authors cannot forget it.

>diff --git a/purple/purplexpcom/public/purpleIAccount.idl b/purple/purplexpcom/public/purpleIAccount.idl

>+  /* Set when the account has not autoLogin enabled
>+     or has had a successful connection with the current parameters */
>+  const short FIRST_CONNECTION_NONE = 0;
This comment doesn't seem accurate. I think you wanted something more like:
This is the value when the preference firstConnectionState is not set. It indicates either that autoLogin is not enabled on this account or that the account has already been successfully connected at least once with the current parameters.

Oh, by the way, if we make this not depend on autologin, you can rephrase it again :).

>diff --git a/purple/purplexpcom/src/purpleAccount.cpp b/purple/purplexpcom/src/purpleAccount.cpp

> nsresult purpleAccount::checkAutoLogin()
> {
>+  PURPLE_ENSURE_INIT(INIT_CONDITION);
>+
>   /* If we don't have a valid protocol, we can't autologin so don't
>      bother checking the value of the pref */
>   if (!mHasValidProtocol)
>     return NS_OK;
> 
>+  /* We previously crashed while connecting an account individually */
>+  /* We check that we are not currently connecting, because an addon or something
>+     can call processAutoLogin while an account is currently connecting
>+     (and pending) but not previously crashed.
>+     So if we don't do this check, and a crash occurs, it would recrash at startup
>+     because the state would already be FIRST_CONNECTION_CRASHED. */
>+  PRInt16 currentState;
>+  if (NS_SUCCEEDED(GetFirstConnectionState(&currentState)) && 
>+      currentState == FIRST_CONNECTION_PENDING &&
>+      (!mAccount->gc || mAccount->gc->state != PURPLE_CONNECTING)) {
>+    SetFirstConnectionState(FIRST_CONNECTION_CRASHED);
>+    mConnectionErrorReason = (PurpleConnectionError) ERROR_CRASHED;
>+    return NS_OK;
>+  }

This is confusing. I think when the account is already connecting, you should just make that method return early.

I pointed out several very minor details by private IM while I was reviewing this patch. Mostly comment changes.

Looks very good otherwise!
Attachment #8351941 - Flags: review?(florian) → review-
Attached patch Patch V5 (!) (obsolete) — Splinter Review
*** Original post on bio 138 as attmnt 209 at 2009-08-23 22:18:00 UTC ***

Now available for all accounts, made the changes so that it is possible (fixing strings per example).

The account manager is displayed only if an account has crashed with autoLogin, but the message remains displayed on all accounts.

Added a crash for nullprpl when the account full name is "crash@test".

A crashed accounts remains crashed until he has a successfully connection or a change in its settings (previously reset the state on a successful quit of the application).

The changes in settings of a crashed account does not auto-connect this account anymore (no more than changes made in an account with no protocol, fixing bug 953667 (bio 221)).

The preference doesn't change anymore when changing the state of autoLogin.

This list is not exhaustive as there were many changed in this version.
Attachment #8351953 - Flags: review?(florian)
Comment on attachment 8351941 [details] [diff] [review]
Patch V4

*** Original change on bio 138 attmnt 197 at 2009-08-23 22:18:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351941 - Attachment is obsolete: true
Comment on attachment 8351953 [details] [diff] [review]
Patch V5 (!)

*** Original change on bio 138 attmnt 209 at 2009-08-23 22:50:18 UTC ***

>diff --git a/instantbird/base/content/instantbird/account.js b/instantbird/base/content/instantbird/account.js

>+      if (this.account.disconnected &&
>+          this.account.connectionErrorReason != Ci.purpleIAccount.NO_ERROR &&
>+          this.account.connectionErrorReason != Ci.purpleIAccount.ERROR_CRASHED &&
>+          this.account.connectionErrorReason != Ci.purpleIAccount.ERROR_UNKNOWN_PRPL) {

Store this.account.connectionErrorReason in a variable.

>diff --git a/instantbird/base/content/instantbird/accounts.js b/instantbird/base/content/instantbird/accounts.js
>--- a/instantbird/base/content/instantbird/accounts.js
>+++ b/instantbird/base/content/instantbird/accounts.js
>@@ -55,28 +55,36 @@ const events = [
> 
> var gAccountManager = {
>   // Sets the delay after connect() or disconnect() during which
>   // it is impossible to perform disconnect() and connect()
>   _disabledDelay: 500,
>   disableTimerID: 0,
>   load: function am_load() {
>     this.accountList = document.getElementById("accountlist");
>+    let defaultIndex = 0;
>     for (let acc in this.getAccounts()) {
>       var elt = document.createElement("richlistitem");
>       this.accountList.appendChild(elt);
>       elt.build(acc);
>+      if (typeof(defaultIndex) == "number" &&
>+          acc.firstConnectionState == acc.FIRST_CONNECTION_CRASHED)
>+        defaultIndex = acc.id;
>     }
>     addObservers(this, events);
>     if (!this.accountList.getRowCount())
>       // This is horrible, but it works. Otherwise (at least on mac)
>       // the wizard is not centered relatively to the account manager
>       setTimeout(function() { gAccountManager.new(); }, 0);
>+    else if (typeof(defaultIndex) == "number") {
>+      this.accountList.selectedIndex = defaultIndex;
>+      this.accountList.ensureSelectedElementIsVisible();
>+    }
>     else
>-      this.accountList.selectedIndex = 0;
>+      this.selectAccount(defaultIndex);

The usage of defaultIndex is confusing here. Take a variable which will contain the id of the account to select. Initialized it with null. And if it evaluates to false at the end, select the account with index 0.
Attachment #8351953 - Flags: review?(florian) → review-
Attached patch Patch V6 (!!)Splinter Review
*** Original post on bio 138 as attmnt 210 at 2009-08-23 22:51:00 UTC ***

Fixing a few coding style problems and optimizations as seen with florian on IM.
Attachment #8351954 - Flags: review?(florian)
Comment on attachment 8351953 [details] [diff] [review]
Patch V5 (!)

*** Original change on bio 138 attmnt 209 at 2009-08-23 22:51:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351953 - Attachment is obsolete: true
Comment on attachment 8351954 [details] [diff] [review]
Patch V6 (!!)

*** Original change on bio 138 attmnt 210 at 2009-08-23 23:15:54 UTC ***

Looks very good, r=me!
Thanks!
Attachment #8351954 - Flags: review?(florian) → review+
*** Original post on bio 138 at 2009-08-23 23:21:33 UTC ***

https://hg.instantbird.org/instantbird/rev/33f82f934962
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.2b1
*** Original post on bio 138 at 2009-08-24 00:00:13 UTC ***

Follow up pushed at:
https://hg.instantbird.org/instantbird/rev/84fb76b3275b

When the first account was selected (no crashed account), the box was scrolled in a weird way.
ensureSelectedItemIsVisible() caused this bug (and was not necessary since the first element is visible by default).
You need to log in before you can comment on or make changes to this bug.