195109 - XPInstall Pre-Checkin Trigger smoketest fails

Status: VERIFIED FIXED

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

Description

[Tracy Walker [:tracy]]

seen on mac mozilla trunk build 2003-02-26-03-trunk
 
-go to the test URL above
-click on the link to run the Xpinstall test
-click Install in the pop up dialog

expected result: test pops a a confirm dialog with PASS

tested results: the pop up says XPinstall test -202: Failed

from the javascript console:
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
 location: "JS frame :: chrome://navigator/content/nsBrowserStatusHandler.js ::
anonymous :: line 350"  data: no]
Source File: chrome://navigator/content/nsBrowserStatusHandler.js
Line: 350

Comment 1

[Samir Gehani]

Sean and I are looking at this now.

Comment 2

[Samir Gehani]

Works on a 2003-02-25-03-trunk Mac OS X build for us.  Tracy reports now all
builds as far back as 2003-02-10 are failing for him.  Proceeding to his machine
to investigate.

Comment 3

[Samir Gehani]

This only affects replace, not adding of files.  While this is a severe
regression we don't believe it should the tree should remain closed for this
bug.  Downgrading severity from smoketest blocker to critical.

Comment 4

[Tracy Walker [:tracy]]

reduced to critical per samir

Comment 5

[Sean Su]

nominating

Comment 6

[Samir Gehani]

installer triage:  nsbeta1+ adt2

Comment 7

[Tracy Walker [:tracy]]

I am no longer seeing this as of commercial trunk build 2003-03-05-07-trunk

marking wfm

Comment 8

[Grace Bush]

for me too

Comment 9

[Sean Su]

reopening bug because I'm still seeing it.

Comment 10

[Sean Su]

Here's the test that will cause xpinstall to either return ACCESS_DENIED (-202)
or REBOOT_NEEDED (999).  I'm not sure why it returns one vs. the other depending
on which machine it is run on.  I just know that it should not be returning
either of these errors.

You have to run the test at least twice (2x).  The first time should run fine
because there's no test file installed in the temp dir by the test.  The 2nd
time is when it fails under OSX.  Windows runs fine:
  http://slip.mcom.com/projects/seamonkey/smoketests/pre_checkin_trigger.html

The xpinstall code in question is:
  http://lxr.mozilla.org/seamonkey/source/xpinstall/src/ScheduledTasks.cpp#277

However, it is this function here that is the cause of the problem:
  http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileOSX.cpp#593

This is because OSX's implementation of MoveToNative() does not delete the file
out of the way first before performing a raname.  This is actually correct given
the following declaration:
  http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIFile.idl#156

This will fix part of the problem to bug 195788 (see comment #4 in that bug).

Not exactly sure who to give this bug to.

Comment 11

[Doug Turner (:dougt)]

the bug is in conrad's code.  if he can't fix it due to time, send it to me.

Comment 12

[Conrad Carlen (not reading bugmail)]

> the bug is in conrad's code.

This bit: http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIFile.idl#156
was added after the implementation of nsLocalFileOSX's MoveToNative was written.
The semantics of the iface changed but the implementation wasn't.

Comment 13

[Conrad Carlen (not reading bugmail)]

OK - maybe the semantics weren't changed, they just didn't specify that
behavior.  Now that they do, I'll change make the OSX impl match.

Comment 14

[Conrad Carlen (not reading bugmail)]

Attachment: makes moveTo[Native] overwrite existing file

OK - now it behaves according to the (relatively new) API comment. I combined
both the simple rename case with the general move/rename case in order to do
the check for an existing file once.

Comment 15

[Conrad Carlen (not reading bugmail)]

2 things I noticed about ReplaceFileNow():
(1) After "result" is inited to nsInstall::ACCESS_DENIED, and everything in this
function succeeds, it's never to set indicate success.
(2) The comment here:
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/ScheduledTasks.cpp#281 has
never been true for the nsLocalFileOSX impl. Now that the old impl is dead,
maybe this code could be simplified?

Comment 16

[Conrad Carlen (not reading bugmail)]

On further testing, this patch doesn't not fix the problem for xpinstall.
MoveToNative() did what it should, but a following call to Exists() returned
TRUE when it should not. Looking at that now...

Comment 17

[Conrad Carlen (not reading bugmail)]

Attachment: XP_MACOSX workaround patch for xpinstall

The problem is explained in the comment. The removed chunk of code was there
only for the sake of the old Mac file impl. The new one doesn't have that
problem - quite the opposite one :-/

Comment 18

[Daniel Veditz [:dveditz]]

So if I understand this patch you've explicitly broken Classic Mac -- is it
really and truly dead? I thought is was simply demoted to "ports" status. Could
you add back the hack for classic inside #ifdef XP_MAC ?

Comment 19

[Conrad Carlen (not reading bugmail)]

Dan, the classic Mac builds are really and truly dead. They're no longer on the
ports page.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 117013 [details] [diff] [review]
XP_MACOSX workaround patch for xpinstall

in that case r/sr=dveditz

Comment 21

[Sean Su]

Comment on attachment 117013 [details] [diff] [review]
XP_MACOSX workaround patch for xpinstall

r=ssu

Comment 22

[Conrad Carlen (not reading bugmail)]

Both patches checked in.

Comment 23

[Grace Bush]

verified Mac build 2003032403

Comment 24

[Sean Su]

both patches merged to 1.3 branch.