-201 error when doing performInstall

VERIFIED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
VERIFIED FIXED
14 years ago
2 years ago

People

(Reporter: Henrik Gemal, Assigned: Sean Su)

Tracking

Trunk
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(3 attachments, 3 obsolete attachments)

18.46 KB, application/x-xpinstall
Details
1.98 KB, text/plain
Details
13.19 KB, patch
Sean Su
: review+
jag (Peter Annema)
: superreview+
(not reading, please use seth@sspitzer.org instead)
: approval1.4+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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
(Reporter)

Comment 1

14 years ago
     ** 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.
(Assignee)

Comment 2

14 years ago
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?
(Reporter)

Comment 3

14 years ago
Created attachment 122311 [details]
linky.xpi

Comment 4

14 years ago
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

14 years ago
Created attachment 122576 [details]
install log
(Reporter)

Comment 6

14 years ago
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
(Assignee)

Comment 7

14 years ago
found the problem.  patch coming up.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

14 years ago
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.
(Assignee)

Updated

14 years ago
Keywords: nsbeta1
(Assignee)

Updated

14 years ago
Attachment #122624 - Flags: review?(dveditz)

Comment 9

14 years ago
adt: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
(Assignee)

Comment 10

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

new patch coming up.
Attachment #122624 - Flags: review?(dveditz)
(Assignee)

Comment 11

14 years ago
Created attachment 122631 [details] [diff] [review]
patch v1.1
Attachment #122624 - Attachment is obsolete: true
(Reporter)

Comment 12

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

Comment 13

14 years ago
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.
(Assignee)

Comment 14

14 years ago
Created attachment 122717 [details] [diff] [review]
patch v1.2

updated patch that fixes a few other logic broken in that function.
Attachment #122631 - Attachment is obsolete: true
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);
(Assignee)

Comment 16

14 years ago
Created attachment 122791 [details] [diff] [review]
patch v1.3

updated patch given dveditz's comments
Attachment #122717 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #122791 - Flags: review?(dveditz)
(Assignee)

Comment 17

14 years ago
*** Bug 204951 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 18

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

got verbal r=dveditz
Attachment #122791 - Flags: superreview?(jaggernaut)
Attachment #122791 - Flags: review?(dveditz)
Attachment #122791 - Flags: review+

Comment 19

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

sr=jag
Attachment #122791 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Comment 20

14 years ago
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=
Attachment #122791 - Flags: approval1.4?
Comment on attachment 122791 [details] [diff] [review]
patch v1.3

a=sspitzer for 1.4 final
Attachment #122791 - Flags: approval1.4? → approval1.4+
(Assignee)

Comment 22

14 years ago
patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 23

14 years ago
looks ok to me- Henrik do you want to verify?
(Reporter)

Comment 24

14 years ago
v on WinXP
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.