80383 - Install must delete component.reg and xpti.dat on upgrade

Status: VERIFIED FIXED

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

Description

[rossi]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9+) Gecko/20010511
BuildID:    2001051104

something like InstallTrigger.installChrome(...) results in javascript error 
"InstallTrigger is not defined"

Reproducible: Always
Steps to Reproduce:
1. open the url
2. click on "import"
3. open javascript console and read the error message

Actual Results:  javascript error "InstallTrigger is not defined"

Expected Results:  popup with question whether to install theme or not

Comment 1

[Phil Schwartau]

Browser, not engine. Reassigning to Installer component - 

Comment 2

[Daniel Veditz [:dveditz]]

This is the sort of thing that could have gotten broken by the xpcdom landing, 
however this particular case WORKSFORME on windows NT in the same 2001051104 
build.

Could we get a confirmation of this bug on Win9x/ME please?

Comment 3

[Johnny Stenback (:jst)]

This was most likely caused by the XPCDOM landing, but I don't know exactly what
the deal is. I know that I saw this, then I removed components/xpti* and
component.reg and things started working, why I don't know.

Comment 4

[rossi]

i can confirm the bug with 2001051308 on a totally different pc with windoze me
and i can also confirm the workaround from Johnny Stenback (delete component.reg
and components/xpti*)...

Comment 5

[Johnny Stenback (:jst)]

Confirming, this needs to be addressed if people are supposed to be able to
update a Netscape 6.0(1) installation to the next release. I saw this on Win2k.

Comment 6

[Daniel Veditz [:dveditz]]

This bug manifested itself as our InstallTrigger object not defined. The 
install scripts (all platforms) have to delete component.reg and xpti.dat files 
to force them to be rebuilt

Comment 7

[Sean Su]

taking this bug

Comment 8

[Sean Su]

Attachment: patch #1 (ns tree)

Comment 9

[Sean Su]

Attachment: patch #1 (moz tree)

Comment 10

[Sean Su]

Attachment: patch #1 (ns tree for mac and unix)

Comment 11

[Sean Su]

Attachment: patch #1 (moz tree for mac and unix)

Comment 12

[Sean Su]

got r=sgehani for all patches.

Comment 13

[Scott MacGregor]

sr=mscott

Comment 14

[Sean Su]

patches checked in.

Comment 15

[Jimmy Lee]

Build: 2001-05-23-08-trunk(LINUX), 2001-05-23-06-trunk(WIN), 
2001-05-23-08-trunk(MAC)

Looks good on all platforms.  Marking Verified!

Comment 16

[Jimmy Lee]

Reopening bug :-(

This appears to work fine if for the wizard installer.  There appears to be a 
problem when upgrading from website say like SmartUpdate.  The component.reg 
does not seem to get deleted.

There may be a conflict with Bug 65678.  That fix is attempting to cleanup, and 
component.reg appears to still be held open thus being unable to complete its 
task.

Comment 17

[Sean Su]

reassigning to Don.  He seems to have found the problem.

Comment 18

[dbragg]

Ok, here's the deal. In nsRegistry.cpp we call nsRegistry::OpenWellKnownRegistry
which opens component.reg using NR_RegOpen().  This file is open for the entire
duration of the program.  NR_RegClose() is only called when the nsRegistry
destructor is called.  For some reason either this isn't getting called or the
OS thinks that since a "child" process was spawned that files open in the parent
process will remain open.

It's weird.

Comment 19

[dbragg]

I was right with the second case (OS keeping the file open).  It's not really
keeping the file open it's just that PR_CreateProcess passes the file
descriptors from the parent to the child.  This cannot be controlled by the
client calling PR_CreateProcess.

This bug is not really an xpinstall or cleanup bug.  It's really a limitation of
nspr's CreateProcess but I'll continue to try to find a way to fix it.

Here are some possible ways to fix it in order of complexity:
1. Change NSPR's PR_CreateProcess (windows implementation) to NOT pass the file
descriptors to the child process.  This is the easiest fix but there might be
some ramifications.
2. Add a new PR_CreateProcessNoDescriptors (or something more elegant) that
doesn't pass the file descriptors.  This would require changing nsIProcess and
it's implementation.
3. Add a variable to PR_CreateProcess to allow the client's to specify if they
want to pass file descriptors.  Same as #2. You'd want to make this a default
value so you wouldn't have to change everyone calling PR_CreateProcess.
4. Figure out how to close the registry file early.  This solution would really
only fix this particular case.  There's the general case of the files being held
open that could easily come up again.
5. Figure out how to get a list of the file descriptors in the child process and
close them.  This would be a windows-only solution.  It would work for
xpicleanup because each of the utilities is platform-specific.  In reading the
MSVC docs it looks like you could do this in 16-bit windows but I haven't found
how to do it in 9x and NT.  Need a Windows guru to help.  I feel this one is a hack.
6. Fix the problem correctly that deleting component.reg worksaround.  I don't
know what that problem is but I hear it's a big one.

 

Comment 20

[Syd Logan]

Actually, this is the kind of thing you have to do with fcntl(). In Unix you can 
call fcntl() on an fd and make it not inheritable by a child process:

F_SETFD
     Set the file descriptor flags defined in <fcntl.h>, that are associated 
with fildes, to the third argument, arg, taken as
     type int. If the FD_CLOEXEC flag in the third argument is 0, the file will 
remain open across the exec() functions;
     otherwise the file will be closed upon successful execution of one of the 
exec() functions.

Looks like nspr has picked up on the concept:

<snip>
extern PRStatus _MD_set_fd_inheritable(PRFileDesc *fd, PRBool inheritable);
#define _MD_SET_FD_INHERITABLE _MD_set_fd_inheritable
</snip>

If "inheritable" is PR_FALSE, the above mention fcntl is invoked.

For the less important OSes, we also have:

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/windows/ntio.c#2660
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/windows/w95io.c#889

Unfortunately, the win95 call will fail, as the function that it blindly calls, 
SetHandleInformation(), is not supported on 95/98. On the otherhand, the Win32 
CreateProcess() function does support an argument, inherithandles, that we are 
blindly setting to TRUE when we create a process, see:

http://msdn.microsoft.com/library/psdk/winbase/prothred_9dpv.htm

for a description of create process, and

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/windows/ntmisc.c#345

for our implementation.

From what I see, since you need a fcntl() in Unix and the best win32 way is to 
specify at process creation time, that there is not going to be a really clean 
way of doing this. Well, perhaps the createprocessnoinherit idea might work if 
on winblows platforms you set the flag on CreateProcess correctly, and for Unix 
you walked the descriptor table and called fcntl on all open descriptors before 
calling fork/exec. Again, on Mac, who cares :-)

I don't know, perhaps we should #ifdef XP_UNIX and do the fcntl() thing on all 
files we open, and #ifdef XP_WIN and just call CreateProcess() ourselves and set 
the flag as we need to so that file descriptors are not inherited. And 
leave everything else alone. Until nspr comes up with a more elegant solution.

cc'ing wtc

Comment 21

[Syd Logan]

OK, I see we are calling nsIProcess for this. So, I was thinking we could add an 
argument to nsIProcess::Init to specify that files should (not) be inherited. If 
this flag is set at process creation time, we can walk the fds on Unix and call 
fcntl() to set the non-inheritance flag (via the nspr abstraction) until we hit 
some max fd (1024 and 4096 are usual system limits) or until we get a EBADF 
error return. For windows, we can replace the call to nspr's process creation 
routine with our own call to win32 CreateProcess() which has the inherit handles 
flag set the way we want it to be set. But I looked at the size of 
_PR_CreateWindowProcess() and associated helper routines and I think a better 
thing would be to get a contribution from nspr, I just don't feel right cloning 
all of that code. If that can't happen then we will roll our own...

Comment 22

[Syd Logan]

Attachment: Outline of a potential solution to shared fds problem

Comment 23

[dbragg]

Syd, Is the philosophy here to get this running for beta and then change it
afterwards?  I'm sure you don't like that solution as much as I don't.  I posted
to the nspr newsgroup about possibly changing the implementation of
CreateProcess on Windows to "blindly" pass FALSE instead of TRUE for the inherit
param.
No reponses yet.  Is wtc the master of that domain?

I believe this is as much (or more)of an NSPR issue than an installer issue.
I'd like to escalate this to determine the right course of action before I
blindly start duping a bunch of NSPR code in xpcom's nsIProcess.  Who do we need
to get on this discussion?

Comment 24

[Syd Logan]

Yeah, the parts of the patch that imply pasting could very well be calls to the 
new and improved nspr functions. Just want to make that clear.

I agree we best have nspr fix this but if we are talking about a stop ship bug 
and nspr is not able to assist, and if this is deemed the best solution (i.e., 
causing the children to not inherit fds), and this is the only way to accomplish 
it, then perhaps our temporarily implementing the solution in installer space 
somewhere is the right thing. 

wtc is the man on this, yep.

Comment 25

[Wan-Teh Chang]

I can add a new attribute for PR_CreateProcess that says "the
child process will not inherit any file descriptors from the
parent."  The implementation of this attribute is trivial on
Windows (just pass FALSE as the bInheritHandles argument to
CreateProcess) but is expensive on Unix as it needs to close
all the fd's in the child process before calling exec().

Are you sure you want to do that?  All you really need to do
is to make sure the fd for component.reg is not inherited by
child processes.  I would modify NR_RegOpen() so that it opens
the component.reg file as non-inheritable.  This would require
platform-specific code.

Comment 26

[dbragg]

I just spoke with QA today (Jimmy Lee).  He said the problem is not happening on
Unix.  Perhaps for beta we could just change the Windows call to pass FALSE in
ntmisc.c.  Or if that's to risky could we do what Wan-Teh says just for windows?

Comment 27

[Syd Logan]

Attachment: Patch to try

Comment 28

[dbragg]

Just built and tested Syd'd patch.  Looks fab.  The bug would be fixed with this
patch.

Syd: were you going to factor out the common code or should I just review the
patch as-is?

Comment 29

[dbragg]

Just tested on Windows ME and looks good there too.
r=dbragg

Comment 30

[Syd Logan]

Attachment: patch for review -- tested winnt and 95 based systems

Comment 31

[Scott MacGregor]

sr=mscott

Comment 32

[Syd Logan]

assigning to me

Comment 33

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1

Comment 34

[Syd Logan]

Fix checked in 0.9.1 branch and trunk

Comment 35

[Jimmy Lee]

Build: 2001-06-05-11-0.9.1(WIN), 2001-06-05-04-0.9.1(LINUX), 
2001-06-05-03-0.9.1(MAC)

This looks good on Windows NT, Windows 98, and Linux.  However, I have stumbled 
into Bug 84236 which blocks me from verifying this fix on Macintosh.

Will add vtrunk keyword when Macintosh has been verified.

Comment 36

[Jimmy Lee]

Build: 2001-06-07-11-0.9.1(MAC)

Verified on Mac.  I got around Bug 84236 by installing each xpi package one at a 
time.  Adding vtrunk to keywords.

Comment 37

[Jimmy Lee]

Build:  2001-08-01-08-trunk(LINUX),  2001-08-01-08-trunk(MAC),   
        2001-08-01-06-trunk(WIN)

Works fine on the trunk.  Removing keyword "vtrunk".  Marking Verified.