204162 - -201 error when doing performInstall

Status: VERIFIED FIXED

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

Description

[Henrik Gemal]

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

Comment 1

[Henrik Gemal]

     ** Linky version 1.5.2 being installed on 2003050108
     ** Installation method is 1
     Linky (version 1.5.2.20030501)
     -----

     ** 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.

Comment 2

[Sean Su]

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?

Comment 3

[Henrik Gemal]

Attachment: linky.xpi

Comment 4

[Grace Bush]

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

Comment 5

[Grace Bush]

Attachment: install log

Comment 6

[Henrik Gemal]

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

Comment 7

[Sean Su]

found the problem.  patch coming up.

Comment 8

[Sean Su]

Attachment: 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 9

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 10

[Sean Su]

Comment on attachment 122624 [details] [diff] [review]
patch v1.0

new patch coming up.

Comment 11

[Sean Su]

Attachment: patch v1.1

Comment 12

[Henrik Gemal]

Sean: how did you find the error? By using a debugger or...? Just wanted to know if I see similar problems.

Comment 13

[Sean Su]

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.

Comment 14

[Sean Su]

Attachment: patch v1.2

updated patch that fixes a few other logic broken in that function.

Comment 15

[Daniel Veditz [:dveditz]]

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);

Comment 16

[Sean Su]

Attachment: patch v1.3

updated patch given dveditz's comments

Comment 17

[Sean Su]

*** Bug 204951 has been marked as a duplicate of this bug. ***

Comment 18

[Sean Su]

Comment on attachment 122791 [details] [diff] [review]
patch v1.3

got verbal r=dveditz

Comment 19

[jag (Peter Annema)]

Comment on attachment 122791 [details] [diff] [review]
patch v1.3

sr=jag

Comment 20

[Sean Su]

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 21

[(not reading, please use [e-mail] instead)]

Comment on attachment 122791 [details] [diff] [review]
patch v1.3

a=sspitzer for 1.4 final

Comment 22

[Sean Su]

patch checked in.

Comment 23

[Grace Bush]

looks ok to me- Henrik do you want to verify?

Comment 24

[Henrik Gemal]

v on WinXP