194298 - OS/2 xpicleanup is TOTALLY horked

Status: VERIFIED FIXED

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

Description

[Max Alekseyev]

OS/2 build 2003021913

To reproduce:
1. Open URL
2. Click on "install 0.8.22" and proceed with installing ChatZilla 0.8.22
3. Exit the browser when installation asked you to restart it
4. Try to start Mozilla again, and get a message 
"The program must close to allow a previous installation attempt to complete.
Please restart."
5. Click Ok and go to 4.

Comment 1

[Mike Kaply [:mkaply]]

To get the browser starting again, you can delete xpicleanup.dat where 
mozilla.exe

This is happening on Windows as well. Anyone know why?

rginda? Anything unique about the chatzilla XPI?

Comment 2

[Robert Ginda]

Nope, not AFAIK.  If there were, I'd expect to hear about this much more often.

Comment 3

[Mike Kaply [:mkaply]]

Weirdness. Something does happen on Windows, but it fixes itself on the next start.

Os/2 is really broke. We never delete xpicleanup.dat.

Taking for Os/2

Comment 4

[Max Alekseyev]

Michael, there is another strange thing. When downloading/installation is done,
there are _two_ windows claiming that it's done, and the browser need to be
restarted. One is Alert window, another is Software Installation. I think, one
would be enough.

Comment 5

[Max Alekseyev]

Hmmm. I've just installed ChatZilla 0.8.20, exited Mozilla, deleted
xpicleanup.dat as suggested, and started Mozilla again.
But it seems that ChatZilla is not installed after all: /about says

Chatzilla 0.8.13c [Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.3b) Gecko/20030220]

What's wrong?

Comment 6

[Mike Kaply [:mkaply]]

The answer is chatzilla.jar is never getting replaced because xpicleanup on OS/2
is TOTALLY wrong.

I'm working on it now.

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Complete rewrite of xpicleanup

OK, this stuff never worked on OS/2. Wow.

This makes xpicleanup work properly. We have one difference between Windows and
OS/2 that I can't solve.

On Os/2, with some XPIs, you will have to restart the browser a second time
because I can't replace JAR files that are in use. On OS/2, child processes
inherit parent filehandles, so we can't replace the files.

With this patch, XPI actually works.

The problem with the message box appearing behind the splash is a separate
issue - I opened bug 194317 for that.

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Better patch

better API for checking if files exist

Comment 9

[Mike Kaply [:mkaply]]

Attachment: NSPR change

We need a change to NSPR so we can delete files on exit.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 115148 [details] [diff] [review]
NSPR change

We were inheriting from the parent which is wrong. What this meant is that
detached processes kept file handles open when the main app was closed.

For this problem, it meant that xpicleanup couldn't replace files.

We have seen other problems where helper apps were launched and the user closed
Mozilla but couldn't restart it.

Fix is to inherit from the shell.

Comment 11

[Wan-Teh Chang]

Comment on attachment 115148 [details] [diff] [review]
NSPR change

r=wtc. I am not really qualified to review this patch.
If would be a good idea to have one of your colleagues
to review it as well.

Is there a flag that says "do not inherit any handles"?
Maybe that's what SSF_INHERTOPT_SHELL means?

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 115147 [details] [diff] [review]
Better patch

sr=blizzard (OS/2 only code)

The is OS/2 only and no risk.

We discovered the xpicleanup function never worked on OS/2. This bug implements
it.

The NSPR change in here is nice to have but not required.

Thanks

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 115147 [details] [diff] [review]
Better patch

a=dveditz

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 115148 [details] [diff] [review]
NSPR change

This tiny OS/2 only NSPR patch combined with the previous OS/2 only patch would
make xpicleanup work perfectly.

Without this, OS/2 will require two browser restarts to overlay files.

Thanks

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 115148 [details] [diff] [review]
NSPR change

a=asa (on behalf of drivers) for checkin to 1.3

Comment 16

[Mike Kaply [:mkaply]]

Fix checked in to branch and trunk

Comment 17

[Wan-Teh Chang]

Comment on attachment 115148 [details] [diff] [review]
NSPR change

I checked in this NSPR patch on the NSPR TIP (NSPR 4.3)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.4alpha).

Comment 18

[Grace Bush]

verified code fixes
OS/2 users please reopen if you disagree