221994 - Patch for XPInstaller to help diagnose install error -239 during chrome registration

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[DocWilco]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.5a) Gecko/20031009 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.5a) Gecko/20031009 Mozilla Firebird/0.6.1

See bug 109044
Error -239 during registration of chrome can a be very tough nut to crack.
Attached to this report will be a patch to help diagnose the error. With the
patch nsRegisterItem.cpp will write extra information to install.log when a -239
occurs.

New bug created as per <a href="mailto:[e-mail]">Henrik Gemal</a>'s
request.

Reproducible: Sometimes

Steps to Reproduce:

Comment 1

[DocWilco]

Attachment: patch for mozilla/xpinstall/src/nsRegisterChrome.*

Comment 2

[Henrik Gemal]

Daniel:
any chance we could get a r= and sr= ?

I see:
c:/mozilla/mozilla/xpinstall/src/nsRegisterItem.h:57: warning: extra
   qualification `nsRegisterItem::' on member `LogFilename' ignored

can we get around this warning?

Comment 3

[HJ]

CC'ing myself because sometimes people ask me about the -239 installation error,
when they try to install MultiZilla, so I would also like to see this implemented.

Comment 4

[DocWilco]

Attachment: updated patch for mozilla/xpinstall/src/nsRegisterChrome.*

updated patch for mozilla/xpinstall/src/nsRegisterChrome.*
fixed the compile warning Henrik Gemal reported.

Comment 5

[Benjamin Smedberg]

Comment on attachment 134906 [details] [diff] [review]
updated patch for mozilla/xpinstall/src/nsRegisterChrome.*

I like the general concept, but this patch needs a little work. It would be a
good idea to actually print the nsresult code in some form, instead of the
generic "blah failed".

Imagine a helper function of the form:
void nsRegisterItem::LogError(nsAString aMessage, nsresult code)
{
  char resultString[12];
  PR_snprintf(resultString, 12, "%lx", code)
  mInstall->LogMessage(aMessage +
		       NS_LITERAL_STRING(" - nsresult code: ")	+
		       NS_ConvertASCIItoUTF16(resultString));
}


>@@ -240,18 +250,31 @@
>     PRInt32 result = nsInstall::SUCCESS;
>     PRBool  isProfile = mChromeType & CHROME_PROFILE;
>     nsIXULChromeRegistry* reg = mInstall->GetChromeRegistry();
>+    nsAutoString logMsg;
> 
>     if ( reg && !(mChromeType & CHROME_DELAYED) )
[snip]
>+        if (NS_FAILED(rv)) {
>+            logMsg = NS_LITERAL_STRING("InstallSkin() failed.");
>+            mInstall->LogComment(logMsg);
>+        }

Why do it this way? You shouldn't need "logMsg" at all. If you aren't going to
use the helper method above, just do
mInstall->LogComment(NS_LITERAL_STRING("InstallSkin

>+        if(NS_FAILED(rv))
>+        {
>+            logMsg = NS_LITERAL_STRING("do_QueryInterface() failed.");
>+            mInstall->LogComment(logMsg);
>+            LogFilename(startupFile);

this seems like overkill; is there any semi-ordinary situation where this might
fail?

Also, when you repost the patch, please use -pu8... I like lots of context in
C++ patches.

Comment 6

[DocWilco]

1) nsresult code is a good idea indeed. 
2) I'm wondering why I used the logMsg tmp var now...
3) I know it might be overkill, but -239 is so furtive I thought I might as well
include that one.

Comment 7

[HJ]

Can you dump nsresult on the console, is that possible?

Comment 8

[DocWilco]

Console or TTY?

TTY would be easy (stdout/stderr), but I was under the impression that that
wasn't really done anymore. Console would be breaking several laws I'd say =)

Currently the messages are written to install.log, and so will the nsresult when
I'm done redoing the patch per bsmedberg's suggestions. Isn't that enough?

Comment 9

[HJ]

Well, writing to install.log is fine by me, happy to solve those installation
problems with the help of this output.

Thanks for your work guys :)

Comment 10

[DocWilco]

Attachment: patch for xpinstall/src/nsInstall.* and xpinstall/src/nsRegisterItem.*

- fixed use of temp var by fixing nsInstall::LogComment's interface.
- using new method and including nsresult in output
- diff'd with -pu8

Comment 11

[[e-mail]]

Comment on attachment 135310 [details] [diff] [review]
patch for xpinstall/src/nsInstall.* and xpinstall/src/nsRegisterItem.*

>Index: xpinstall/src/nsRegisterItem.h
>===================================================================
>     private:
>         nsresult GetURLFromIFile(nsIFile *aFile, char **aOutURL);
>+	void LogError(const nsAString& aMessage, nsresult code);
>+	void LogError(const nsAString& aMessage, nsresult code, nsILocalFile *localFile);

you used tabs here. please switch to spaces

>Index: xpinstall/src/nsRegisterItem.cpp
>===================================================================
>+void nsRegisterItem::LogError(const nsAString& aMessage, nsresult code)
>+{
>+    char resultString[12];
>+    nsAutoString logMsg;
>+
>+    PR_snprintf(resultString, 12, "%lx", code);
>+    logMsg = aMessage + NS_LITERAL_STRING(" - nsresult code: ")
>+             + NS_ConvertASCIItoUTF16(resultString);
>+    mInstall->LogComment(logMsg);
>+    mInstall->LogComment(aMessage + NS_LITERAL_STRING(" - nsresult code: ")
>+                         + NS_ConvertASCIItoUTF16(resultString));
>+}

looks like you can remove the nsAutoString and the first LogComment call?

Comment 12

[Benjamin Smedberg]

Comment on attachment 135310 [details] [diff] [review]
patch for xpinstall/src/nsInstall.* and xpinstall/src/nsRegisterItem.*

This is very close... what dwitte said, plus a few nits:

>+	void LogError(const nsAString& aMessage, nsresult code, nsILocalFile *localFile);

Although method overloading is appropriate in some cases, not here: use a
descriptive name like LogErrorWithFilename. Also, please add a little comment
describing what both error methods do. In addition, please do an xpinstall with
your patch to make sure that it creates the expected output.

>+void nsRegisterItem::LogError(const nsAString& aMessage, nsresult code)
>+{
>+    char resultString[12];
>+    nsAutoString logMsg;
>+
>+    PR_snprintf(resultString, 12, "%lx", code);

Does the %lx format print a 0x before the number? If not, you should include 0x
in the string directly below.

>+    logMsg = aMessage + NS_LITERAL_STRING(" - nsresult code: ")
>+             + NS_ConvertASCIItoUTF16(resultString);
>+    mInstall->LogComment(logMsg);
>+    mInstall->LogComment(aMessage + NS_LITERAL_STRING(" - nsresult code: ")
>+                         + NS_ConvertASCIItoUTF16(resultString));
>+}

Nit: in mozilla coding style, operators belong at the end of the line, not the
beginning, so the + belongs on the previous line. (and you accidentally
duplicated the code, as dwitte said).

>+        if(NS_FAILED(rv))
>+        {
>+            LogError(NS_LITERAL_STRING("do_QueryInterface() failed."), rv, startupFile);
>+        }

I really think this is a waste, please take it out.

>+                            LogError(NS_LITERAL_STRING("writing to installed-chrome.txt faile
>+d."), rv);

Bad wrapping? this happens twice more.

--BDS

Comment 13

[DocWilco]

Attachment: patch for xpinstall/src/nsInstall.* and xpinstall/src/nsRegisterItem.*

- fixed tabs (spaces now)
- added small comments to nsRegisterItem.h
- fixed duplicate code
- renamed LogError w/localfile to LogErrorWithFilename
- added 0x to nsresult printing
- fixed coding style for +
- removed QI error
- fixed bad wrapping

Comment 14

[Benjamin Smedberg]

.

Comment 15

[Benjamin Smedberg]

Checked in on trunk. Thank you Doc Wilco!

--BDS

Comment 16

[Henrik Gemal]

I just got the -239 chrome error when I tried to install an xpi package.
I've opened bug http://bugzilla.mozilla.org/show_bug.cgi?id=226181
about it.
Now we should have a little more info on the -239 thing