Closed Bug 80383 Opened 20 years ago Closed 19 years ago

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

Categories

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

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: rossi, Assigned: slogan)

References

()

Details

(Keywords: regression, Whiteboard: fix in handing. waiting for drivers to approve for branch and trunk checkin)

Attachments

(7 files)

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
Browser, not engine. Reassigning to Installer component - 
Assignee: rogerl → ssu
Component: Javascript Engine → Installer
QA Contact: pschwartau → gemal
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?
Assignee: ssu → syd
Component: Installer → Installer: XPInstall Engine
QA Contact: gemal → jimmylee
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.
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*)...
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows ME → All
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
Keywords: nsbeta1nsbeta1+, regression
Summary: InstallTrigger is not defined → Install must delete component.reg and xpti.dat on upgrade
Target Milestone: --- → mozilla0.9.1
Whiteboard: [smartupdate]
taking this bug
Assignee: syd → ssu
got r=sgehani for all patches.
Status: NEW → ASSIGNED
sr=mscott
patches checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
reassigning to Don.  He seems to have found the problem.
Assignee: ssu → dbragg
Status: REOPENED → NEW
Blocks: 65678
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.
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.

 
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
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...
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?
Whiteboard: [smartupdate] → [smartupdate] No ETA yet, investigating.
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.
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.
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?
Attached patch Patch to trySplinter Review
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?
Just tested on Windows ME and looks good there too.
r=dbragg
sr=mscott
assigning to me
Assignee: dbragg → syd
Status: NEW → ASSIGNED
Whiteboard: [smartupdate] No ETA yet, investigating. → fix in handing. waiting for drivers to approve for branch checkin
Whiteboard: fix in handing. waiting for drivers to approve for branch checkin → fix in handing. waiting for drivers to approve for branch and trunk checkin
a= asa@mozilla.org for checkin to 0.9.1
Fix checked in 0.9.1 branch and trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago19 years ago
Resolution: --- → FIXED
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.
Depends on: 84236
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.
Keywords: vtrunk
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.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.