18.46 KB, application/x-xpinstall
1.98 KB, text/plain
13.19 KB, patch
Sean Su: review+
jag (Peter Annema): superreview+
(not reading, please use firstname.lastname@example.org instead): approval1.4+
|Details | Diff | Splinter Review|
when I try to install my mozilla extenstion I'm begging to see an -201 error. To install the xpi file I have to exit mozilla and restart it and the it works fine. seems to happen when I install from my local file system (file://blabla.xpi) 20030501
** Linky version 1.5.2 being installed on 2003050108 ** Installation method is 1 Linky (version 126.96.36.19930501) ----- ** ok adding jar file. ** ok registering the content chrome. ** ok registering the locale chrome. [1/5] Installing: C:\Program Files\mozilla.org\nightly\bin\defaults\pref\linky.js [2/5] Replacing: C:\Program Files\mozilla.org\nightly\bin\chrome\linky.jar ** Problem performing install. Error code: -201 it seems to be related to replacing the jar file.
You mean this only happens if you're installing ontop of a previous instance of your extension, or on the first time? Can you also post your script file?
I was able to install Linky and have it work - however when I tried a second time and was replacing- I got the same error- will attach my log
if you remove the "linky.jar" from the chrome you can install it again. So the -201 error seems to have something to do with replacing the .jar file. Does a debug build contain more info about what's going wrong? I've filed bug 204575 about the missing NSPR log option in xpi install engine
found the problem. patch coming up.
Created attachment 122624 [details] [diff] [review] patch v1.0 The problem was that if MoveToNative() failed, we stop with error -201. This is not right. MoveToNative() failing simply means that the file is locked in memory, and that we need to replace it on browser restart or system restart. linky.xpi installs fine now on the 2nd+ time(s). it also now informs the user that a restart is necessary.
Comment on attachment 122624 [details] [diff] [review] patch v1.0 new patch coming up.
Created attachment 122631 [details] [diff] [review] patch v1.1
Sean: how did you find the error? By using a debugger or...? Just wanted to know if I see similar problems.
to debug this, do the following: 1) run mozilla.exe in vc6 2) make sure that linky.xpi has been installed at least once. 3) drag and drop linky.xpi into a browser window 4) before clicking [Install], in VC6, open mozilla\xpinstall\src\ScheduledTasks.cpp and place a breakpoint in line #287: if (NS_FAILED(rv)) result = nsInstall::UNEXPECTED_ERROR; -> rv = renamedDoomedFile->MoveToNative(parent, uniqueLeafName); if (NS_FAILED(rv)) return nsInstall::UNEXPECTED_ERROR; You it will fail on the 2nd time the breakpoint gets triggered because that's when ...\chrome\linky.jar is installed. The first time should be ...\Defaults\Pref\linky.js. MoveToNative() fails, but should not return UNEXPECTED_ERROR. Is should continue and return ACCESS_DENIED which flags the file to be replaced at restart of the browser. ps. you can preload the file that ScheduledTasks.cpp belongs to in VC6 by adding: ...\mozilla\dist\bin\components\xpinstal.dll in Project|Settings|Debug|Additional DLLS|Local Name. This allow you to set the breakpoint immediately after the browser starts up.
Created attachment 122717 [details] [diff] [review] patch v1.2 updated patch that fixes a few other logic broken in that function.
Comment on attachment 122717 [details] [diff] [review] patch v1.2 > PRBool flagExists, flagIsEqual; >+ nsCAutoString doomedFilePath; >+ nsCOMPtr<nsILocalFile> doomedFileLocal = do_QueryInterface(aDoomedFile); These should go back in the #ifdef XP_MACOSX -- for one you don't want to entangle your code with that hack because one day that'll be removed, and more importantly it's aReplacementFile you need to put back, not the doomed file. I checked and no one who calls ReplaceFileNow() directly uses the replacement nsIFile after the call, but it is an argument to ReplaceFileNowOrSchedule() passed through and two callers of that--nsInstallFile and nsInstallPatch--use that replacement file to clean up a temp file in the case of errors. They pass in an internal member object and clearly don't expect it to get changed. > #ifdef XP_MACOSX > // If we clone an nsIFile, and move the clone, the FSRef of the *original* > // file is not what you would expect - it points to the moved file. This > // is despite the fact that the two FSRefs are independent objects. Until > // the OS X file impl is changed to not use FSRefs, need to do this (see > // bug 200024). >- nsCOMPtr<nsILocalFile> doomedFileLocal(do_QueryInterface(doomedFile)); >- nsCAutoString doomedFilePath; As mentioned above, these should come back. > if (NS_FAILED(rv)) result = nsInstall::UNEXPECTED_ERROR; I know this wasn't yours, but please split into two lines. > tmpLocalFile->GetParent(getter_AddRefs(parent)); //get the parent for later use in MoveTo [...] > rv = renamedDoomedFile->MoveToNative(parent, uniqueLeafName); you could skip the GetParent() and pass nsnull as the first arg to MoveToNative() here. Slight bloat removal. >+ // renamedDoomedFile needs to be reset because it's used later on in this >+ // function. >+ rv = renamedDoomedFile->SetNativeLeafName(uniqueLeafName); >+ if (NS_FAILED(rv)) >+ return nsInstall::UNEXPECTED_ERROR; This return should be "result = nsInstall::UNEXPECTED_ERROR;" instead, so the #ifdef XP_MACOSX hack that follows gets a chance to do its thing. That's followed by an if() that will detect and return this error. >+ nsCAutoString leafname; This would be clearer as something like doomedLeafname >+ // aReplacementFile needs to be updated because it could be used when returned >+ // from this function. >+ nsCOMPtr<nsILocalFile> replacementFileLocal(do_QueryInterface(aReplacementFile)); >+ rv = doomedFileLocal->GetNativePath(doomedFilePath); >+ if ( NS_SUCCEEDED(rv) ) >+ rv = replacementFileLocal->InitWithNativePath(doomedFilePath); This needs to be the original replacement file path, not doomed file, which means you'll have to move the path capturing part before the MoveTo. One major problem with restoring all this stuff at the end is what if it fails? You've accomplished your main goal, do we really want to return errors and back stuff out? Moving the capture all the way to the beginning solves that problem, and you could probably ignore the rv on InitWithNativePath so we don't end up doing >+ if (NS_FAILED(rv)) >+ result = nsInstall::UNEXPECTED_ERROR; In fact, safest of all would be to clone aReplacementFile into replacementFile up at the top, then not worry about it (except for the MacOSX bug, but since the result of that bug in this case would be that a cleanup might fail in a rare error case that seems worth it). This whole block down here below the final MoveTo could go away. >+ if ( NS_SUCCEEDED(rv) && result != nsInstall::UNEXPECTED_ERROR ) If you did all that you probably wouldn't need the result check in this if(). >+ renamedDoomedFile->GetParent(getter_AddRefs(parent)); >+ renamedDoomedFile->MoveToNative(parent, leafname); These two lines could be combined for a minor bloat win: >+ renamedDoomedFile->MoveToNative(nsnull, leafname);
Created attachment 122791 [details] [diff] [review] patch v1.3 updated patch given dveditz's comments
*** Bug 204951 has been marked as a duplicate of this bug. ***
Comment on attachment 122791 [details] [diff] [review] patch v1.3 got verbal r=dveditz
Comment on attachment 122791 [details] [diff] [review] patch v1.3 sr=jag
Comment on attachment 122791 [details] [diff] [review] patch v1.3 tested under win32, osx, and linux with linky.xpi, venkman.xpi, and browser.xpi. All .xpi files were installed multiple times as well. seeking a=
Comment on attachment 122791 [details] [diff] [review] patch v1.3 a=sspitzer for 1.4 final
patch checked in.
looks ok to me- Henrik do you want to verify?
v on WinXP