953454 - report startup failures in an understandable way

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect, P5)

Description

[Florian Quèze [:florian]]

*** Original post on bio 3 at 2007-10-09 12:32:00 UTC ***

When the UI starts, it should check that:
1. purplexpcom is loaded
2. libpurple is loaded (check that we can get the version number). Maybe it should check that the libpurple version number is the same as expected.
3. Check that protocol plugins could be loaded (ie that the list of protocol is not empty).

Expected result: A failure in any of these points should be reported by a message box displaying the exact error.

Current behavior: When purplexpcom or libpurple is not loaded, there are strange error messages. When no protocol is loaded, the account wizard gets opened with an empty protocol list.

Comment 1

[Romain BEZUT]

Attachment: First try for a patch

*** Original post on bio 3 as attmnt 74 at 2008-08-24 01:50:00 UTC ***

A first try for this, not sure if that is exactly what you wanted.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8351818 [details] [diff] [review]
First try for a patch

*** Original change on bio 3 attmnt 74 at 2008-08-24 10:09:25 UTC ***

>diff -r 5940feb4b98a instantbird/base/content/instantbird/blist.js
>--- a/instantbird/base/content/instantbird/blist.js	Sat Aug 23 02:25:22 2008 +0200
>+++ b/instantbird/base/content/instantbird/blist.js	Sun Aug 24 03:45:18 2008 +0200
>@@ -175,11 +175,17 @@ var buddyList = {
>       // don't worry too much about this exception.
>     }
> 
>-    initPurpleCore();
>-    buddyList.checkNotDisconnected();
>-    buddyList.checkForIrcAccount();
>-    addObservers(buddyList, events);
>-    this.addEventListener("unload", buddyList.unload, false);
>+
>+    if (initPurpleCore()) {
>+      buddyList.checkNotDisconnected();
>+      buddyList.checkForIrcAccount();
>+      addObservers(buddyList, events);
>+      this.addEventListener("unload", buddyList.unload, false);
>+    }
>+    else {
>+      uninitPurpleCore(true);
>+      window.close();
>+    }

if initPurpleCore fails, you just need to close the window and leave the function (return). You don't want to call uninitPurpleCore, because it wasn't initialized.
You probably have a specific case in initPurpleCore if purpleCoreService.init succeeded but the protocol list is empty. In this case you may want to uninitialize it right there before returning false.

By the way, if you just close the window and return when the initialization fails, you don't need the 'else'.

>+	content/instantbird/core.js		(instantbird/core.js)

You forgot to add this file in the patch, which makes it not really useful ;-).

Comment 3

[Romain BEZUT]

Attachment: 2nd Try

*** Original post on bio 3 as attmnt 75 at 2008-08-24 19:27:00 UTC ***

But when you change a condition in core.js, it does a segfault after window.close().

Comment 4

[Romain BEZUT]

Comment on attachment 8351818 [details] [diff] [review]
First try for a patch

*** Original change on bio 3 attmnt 74 at 2008-08-24 19:27:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8351819 [details] [diff] [review]
2nd Try

*** Original change on bio 3 attmnt 75 at 2008-08-25 01:31:51 UTC ***

>diff -r 5940feb4b98a instantbird/base/content/instantbird/blist.js
>@@ -175,7 +175,12 @@ var buddyList = {
>       // don't worry too much about this exception.
>     }
> 
>-    initPurpleCore();
>+

You don't need 2 empty lines here.

>diff -r 5940feb4b98a instantbird/base/content/instantbird/core.js

>+function initPurpleCore()
>+{
>+  var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                          .getService(Components.interfaces.nsIPromptService);

You only need this in the (very) unlikely case of an error.
I suggest you create a separate function to display error messages.
This function can also take care of localization of messages (you give only a message key as an argument of the function).

>+  if (Ci.purpleICoreService == null) {

Use the '!' operator instead of testing '== null'.

>+    prompts.alert(null, "Loading error", "An error occured : The file \"purple.xpt\" is not correctly loaded.");

No space before ':' in English. For the error message, I suggest:
The file "purplexpcom.xpt" is missing or corrupted.

>+  if (Components.classes["@instantbird.org/purple/core;1"] == null) {
>+    prompts.alert(null, "Loading error", "An error occured : purplexpcom component is not correctly loaded.");

XPCOM registration of the purplexpcom component failed.

>+  try {
>+    var pcs = Components.classes["@instantbird.org/purple/core;1"]
>+                        .getService(Ci.purpleICoreService);
>+    pcs.init();
>+  }
>+  catch (e) {
>+    prompts.alert(null, "Loading error", "An error occured while initializing purplexpcom : " + e);

occurred (2 'r')

To handle this case (a formatted string) with a single function displaying error messages, I think you can add another argument to the function. If it is null then the string is unformatted, if the value of this parameter is not null, then it's a formatted string.

>+    return false;
>+  }
>+
>+  if (!pcs.version) {
>+    prompts.alert(null, "Loading error", "An error occured while checking whether libpurple is loaded.");

An error occurred while checking whether libpurple is usable.
(if libpurple is not loaded at all, then I'm pretty sure the XPCOM registration will fail and we will display one of the previous error messages. This error message is probably the less likely to appear. It could be that the loaded libpurple is not compatible with the one purplexpcom was linked with, but in this case we are likely to crash...)

>+    return false;
>+  }
>+
>+  if (!pcs.getProtocols().hasMoreElements()) {
>+    prompts.alert(null, "Protocol error", "An error occured : No protocol found.");

No protocol plugin could be loaded.

>+function uninitPurpleCore()
>+{
>+  try {
>+    var pcs = Components.classes["@instantbird.org/purple/core;1"]
>+                        .getService(Ci.purpleICoreService);
>+    pcs.quit();
>+  }
>+  catch (e) {
>+    var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                            .getService(Components.interfaces.nsIPromptService);
>+
>+    prompts.alert(null, "Unload Error", "An error occured while unloading PurpleCore : " + e);

An error occurred while shutting down purplexpcom:

When an error occurs while starting up Instantbird, we should probably include in the error message some kind of apologizes, and explain that the application is going to be completely useless, and close itself...


For the localization, you probably don't want to attach a string bundle to blist.xul (because these strings are completely unrelated to the buddy list). You can use something like:
         Components.classes["@mozilla.org/intl/stringbundle;1"]
                   .getService(Components.interfaces.nsIStringBundleService)
                   .createBundle("chrome://instantbird/locale/core.properties")
                   .GetStringFromName("purplexpcomXptMissing");

Even though I gave a lot of comments on this patch, it goes in the right direction ;-).

Comment 6

[Romain BEZUT]

Attachment: patch V3

*** Original post on bio 3 as attmnt 76 at 2008-08-25 12:24:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[Romain BEZUT]

Comment on attachment 8351819 [details] [diff] [review]
2nd Try

*** Original change on bio 3 attmnt 75 at 2008-08-25 12:24:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8351820 [details] [diff] [review]
patch V3

*** Original change on bio 3 attmnt 76 at 2008-08-25 12:44:58 UTC ***

This looks pretty good!

>+  if (!aMessage) {
>+    prompts.alert(null, bundle.GetStringFromName("startupFailure.title"),
>+                  bundle.GetStringFromName("startupFailure.apologize") + "\n\n" +
>+                  bundle.GetStringFromName(aKeyString));
>+  }
>+  else {
>+    prompts.alert(null, bundle.GetStringFromName("startupFailure.title"),
>+                  bundle.GetStringFromName("startupFailure.apologize") + "\n\n" +
>+                  bundle.formatStringFromName(aKeyString, [aMessage], 1));
>+  }
>+}
>\ No newline at end of file

Minuscule nit: The code is a little redundant here. I think I'll push it like this:
 var title = bundle.GetStringFromName("startupFailure.title");
 var message = bundle.GetStringFromName("startupFailure.apologize") + "\n\n";
 message += aMessage ? bundle.formatStringFromName(aKeyString, [aMessage], 1)
                     : bundle.GetStringFromName(aKeyString);
 prompts.alert(null, title, message);

And I'll also add the missing newline at the end of the file.

Thanks!

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 3 at 2008-08-25 13:08:10 UTC ***

Pushed as 286:60fd0e14b40f.