Closed Bug 221994 Opened 21 years ago Closed 21 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.6beta

People

(Reporter: bugzilla, Assigned: bugzilla)

Details

Attachments

(1 file, 3 obsolete files)

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:bugzilla@gemal.dk">Henrik Gemal</a>'s request. Reproducible: Sometimes Steps to Reproduce:
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #133187 - Flags: review?(dveditz+bmo)
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?
Assignee: xpi-engine → bugzilla
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.
updated patch for mozilla/xpinstall/src/nsRegisterChrome.* fixed the compile warning Henrik Gemal reported.
Attachment #133187 - Attachment is obsolete: true
Attachment #133187 - Flags: review?(dveditz+bmo)
Attachment #134906 - Flags: review?(dveditz+bmo)
Attachment #134906 - Flags: review?(dveditz+bmo) → review?(bsmedberg)
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.
Attachment #134906 - Flags: review?(bsmedberg) → review-
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.
Can you dump nsresult on the console, is that possible?
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?
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 :)
- fixed use of temp var by fixing nsInstall::LogComment's interface. - using new method and including nsresult in output - diff'd with -pu8
Attachment #134906 - Attachment is obsolete: true
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 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
Attachment #135310 - Flags: review-
- 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
Attachment #135310 - Attachment is obsolete: true
Attachment #135319 - Flags: review?(bsmedberg)
Attachment #135319 - Flags: superreview?(bryner)
Attachment #135319 - Flags: review?(bsmedberg)
Attachment #135319 - Flags: review+
Attachment #135319 - Flags: superreview?(bryner) → superreview+
.
Assignee: bugzilla → bugzilla
Target Milestone: --- → mozilla1.6beta
Checked in on trunk. Thank you Doc Wilco! --BDS
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: