97334 - Update.html for browser fails for Win 98/ME

Status: VERIFIED FIXED

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

Description

[Jimmy Lee]

Build: 2001-08-28-09-trunk(WIN)

1. Open
ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2001-08-28-09-trunk/update.html
2. Click Launch XPInstall button
3. After update is successful, exit browser and restart machine
4. Launch browser

RESULT:
Browser does not launch.

Some files have been named improperly.  From the chrome directory, Messen~1.jar
and Net2ph~1.jar are incomplete.  From the components directory, Activa~1.dll,
Embedc~1.dll, Gkcont~1.dll, and Qfaser~1.dll are incomplete.

EXPECTED RESULT:
Browser launches.

Comment 1

[Jimmy Lee]

Nominating.  This needs to be addressed for files that get replaced even if they
are not browser files.

Comment 2

[Daniel Veditz [:dveditz]]

This is because 1) post-install replace was not implemented for Win9x/ME, and 2)
ren8dot3 is no longer shipped because we though post-install replace was
implemented.

We should fix 1) since that will remove the reboot requirement, but 2) might be
the faster short-term fix. if the scripts to generate the rename lists still works

Comment 3

[Syd Logan]

*** Bug 65678 has been marked as a duplicate of this bug. ***

Comment 4

[Syd Logan]

Ok, so, what was the ren8dot3 hack...

Comment 5

[Syd Logan]

I found the ren8dot3 stuff in a file called windows.t. In browser.jst, there is
references to it at
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/windows/browser.jst#355
and
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/windows/browser.jst#369,
though I am not sure what those lines end up translating to. Is all this stuff
wired in but we need something in config.it to kick it off?

Comment 6

[Daniel Veditz [:dveditz]]

I know the utility itself was removed from packages-win so that's one thing
you'd have to fix. And double-check to make sure it's still being built.

Don't know the state of the build scripts, but ssu would know whether they're
functional, turned off, bit-rotted, etc.

Comment 7

[Sean Su]

To get the functionality working again, just put ren8dot3.exe back into 
packages-win and packages-static-win.

It uses a file list to know what needs to be renamed.  This list, stored in var 
listLongFilePaths, is automatically generated by makejs.pl.  However, it is only 
generated it the script files a key: $Ren8dot3List$

If you have a list, you will also have to include another key for all this to 
work:
$Ren8dot3Call$

As an example, take a look at browser.jst and then browser.js (either mozilla or 
ns).

Suggested patch coming up...

Comment 8

[Sean Su]

Attachment: possible fix

Comment 9

[Sean Su]

oh, forgot to mention that you only need to apply the patch to the moz tree.  
The ns build script will just pick it up the change afterwards.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 48306 [details] [diff] [review]
possible fix

r=dveditz

Comment 11

[Daniel Veditz [:dveditz]]

This needs to go on the branch and trunk

Comment 12

[Cindy Roberts]

There aren't any comments on this bug since the 5th of Sept.  Can QA regess
against the Netscape commercial builds and determine if this is still a valid
bug?  Please mark as nsbranch+ which will get this on the PDT radar if you think
this is critical and can give us an ETA for the fix/reviews. Else, mark is as
nsbranch-. Also, can someone comment in the bug how serious you think this is? 
PDT is only accepting "stop ship" bugs at this point such as data loss, loss of
major functionality, regressions and bugs to the eMojo "stop ship" features.

Comment 13

[Syd Logan]

r=syd, dan, can I get an sr= from branch?

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 48306 [details] [diff] [review]
possible fix

sr=dveditz

Comment 15

[Daniel Veditz [:dveditz]]

Check this into the trunk as well for now, although the real fix is to do
"post-install replace" correctly on Win9x.

Comment 16

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 17

[Curt Patrick (gone)]

I'll take this now.

Comment 18

[Curt Patrick (gone)]

Checked Sean's patch into mozilla trunk and branch.

Comment 19

[Jimmy Lee]

Build:  2001-10-02-05-0.9.4(WIN)

Looks good on the branch.  This still needs to be verified on the trunk.

Comment 20

[Jimmy Lee]

Build:  2001-10-24-09-trunk(WIN)

Works for trunk.  Marking Verified!

Comment 21

[Daniel Veditz [:dveditz]]

Reopening. The temporary hack is good because this was a "critical" bug, but
we've really got to fix it the right way by making win9x use the "post-install
replace" feature of bug 65678.

This blocks bug 105144 on win9x

Comment 22

[Daniel Veditz [:dveditz]]

*** Bug 115883 has been marked as a duplicate of this bug. ***

Comment 23

[Daniel Veditz [:dveditz]]

I'll take this for now

Comment 24

[Syd Logan]

DPRICEFIX

Comment 25

[dprice (gone)]

Attachment: fix for the underlying problem

This patch adds a new flag to addFile() that will allow us to label files as
windows system files.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 48306 [details] [diff] [review]
possible fix

Obsoleting the hack. This will have to be removed, alone with the replacement
variables in the .jst files.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 71826 [details] [diff] [review]
fix for the underlying problem

>-    /* Get build numbers for Windows NT or Win32s */
>+    char* final;
>+    char* current;
>+
>+    finalSpec->GetPath(&final);
>+    currentSpec->GetPath(&current);

As we're updating this old code we should update old string usage.
Use nsXPIDLCString for final and current and drop the leak-prone
explicit frees.

The mixture of // and /**/ comments is annoying. Since // is used
overwhelmingly in the file please switch the /**/ ones in this routine
to use //. Don't switch the license header though, which is standardized
for potential script processing.

>+    else
>+    {
>+        // Windows NT
>+        MoveFileEx(final, current, MOVEFILE_DELAY_UNTIL_REBOOT);
>+    }
>+
>+    free(final);
>+    free(current);
>+
>     return err;

How did you test this? Looks to me like a system file will get replaced
twice on NT, once here, and once in the main routine because you don't
return 0 for success (nor do you check MoveFileEx for failure or success).

By "How did you test this?" I'm looking for a list of specific test
scenarios that you tried (you've got at least 4, right?). XPInstall,
like widgets, is one of those sucky non-XP areas of the product that has
to deal with real OS differences. We've tried to minimize that as much
as possible (and in Mozilla--unlike 4.x--a lot is hidden inside nsLocalFile)
but it's still something we have to test to validate our assumptions.

> #ifdef _WINDOWS
>-        if ( ReplaceExistingWindowsFile(replacementFile, doomedFile) == 0 )
>-            return nsInstall::REBOOT_NEEDED;
>+        if ( aMode == WIN_SYSTEM_FILE )
>+            if ( ReplaceWindowsSystemFile(replacementFile, doomedFile) == 0 )
>+                return nsInstall::REBOOT_NEEDED;

mode is a bitfield ("flags" or "style" might have been clearer names
originally -- but good that your arg name matches "mode" used elsewhere).
This will do the wrong thing if another style is set in addition to
WIN_SYSTEM_FILE.

Please make this if(A && B) rather than nested if(A) if(B)

Comment 28

[dprice (gone)]

For testing I ran the f_addfile_notepad_inuse test.  I changed it to mark the
notepad.exe as a system file.  But looking over the code, I see that we call
ReplaceFileNow before the check of the WIN_SYSTEM_FILE flag.  So the replace was
succeeding before we got to the changed code.  doh!  Working with Jimmy to get a
proper test case.

Comment 29

[dprice (gone)]

unfortunately AnsiToOem is expecting char*'s  I talked to sfraser about how to
pass in 'the right thing'  He's concerned that putting the cstring's buffer into
AnsiToOem may change the length of the buffer and thoroughly confuse everything.
 His advice is to leave this alone.

Comment 30

[dprice (gone)]

Attachment: patch with changes for review

Comment 31

[dprice (gone)]

Attachment: test case

here's the test case i created.  It replaces the msvcrtd.dll in the windows
system folder.	So in a debug build, run the test.  The .dll will be held open
and the update will have to be postponed.  Testing shows it works on Win98. 
Win2000 still avoids the new code, but it properly returns code 999 reboot
required.  Testing NT 4.0 tomorrow

Comment 32

[dprice (gone)]

on nt 4.0 and 2000 I'm running into something of a problem.  The initial call to
ReplaceFileNow was succeeding when replacing a system file.  So I stepped
through this with a debugger and reset the return value of RepaceFileNow and
forced it to exercise the code in ReplaceWindowsSystemFile()  And it worked.  yay! 

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 72691 [details] [diff] [review]
patch with changes for review

Why are you changing comment style around?  When in rome....

Change this comment to Windows 95/98 or Win32 Win3.1:

// Windows 95 or Win16


why did you remove the scope from the following:

+// not using 0x1 in this bitfield because it causes problems with legacy code
+#define DO_NOT_UNINSTALL  0x2
+#define WIN_SHARED_FILE   0x4
+#define WIN_SYSTEM_FILE   0x8
+

If that is the direction you want to go in, at least be consistant and change
the others too.

Maybe I missed something, but where is ReplaceFileNowOrSchedule defined in this
patch??

Have you tested this patch on at least win98 and win2000?

post a new patch with my suggests.  Please use cvs diff -u10 so that there is
more context.  dt.

Comment 34

[dprice (gone)]

The majority of the comments in the file are // style.  I'm changing these to
match.  

dveditz suggested that I pull those definitions from the enum and make them
defines because he felt that they were in the wrong place.  These flags aren't
really error codes.  I could give them their own enum so they'd maintain their
scope.  What do you think is best? 

ReplaceFileNowOrSchedule is defined in ScheduledTasts.h... 

 PRInt32 DeleteFileNowOrSchedule(nsIFile* filename);
-PRInt32 ReplaceFileNowOrSchedule(nsIFile* tmpfile, nsIFile* target );
+PRInt32 ReplaceFileNowOrSchedule(nsIFile* tmpfile, nsIFile* target, PRInt32 aMode);
 PRInt32 ScheduleFileForDeletion(nsIFile* filename);

It has been tested under win98, 2000 and NT 4.0

Comment 35

[Doug Turner (:dougt)]

>The majority of the comments in the file are // style.  I'm changing these to
match.  

If you are going to touch that file, fix all of them then. Line 453, 493, 510,
560.  Lets be consistent.

>dveditz suggested that I pull those definitions from the enum and make them
defines because he felt that they were in the wrong place.  These flags aren't
really error codes.  I could give them their own enum so they'd maintain their
scope.  What do you think is best? 

You made them #defines which are not enum's.  debuggers will have no access to
them, ect.  I don't care.  What you got is fine, i guess.

>ReplaceFileNowOrSchedule is defined in ScheduledTasts.h... 

Where is it implemented in the patch?  Y

>It has been tested under win98, 2000 and NT 4.0

great.

Comment 36

[dprice (gone)]

Attachment: the whole patch this time

I cleaned out unrelated changes from the diff, and clobbered the implementation
of ReplaceFileNowOrSchedule Here's the complete diff with the changes you
suggested.

Comment 37

[Doug Turner (:dougt)]

Comment on attachment 73180 [details] [diff] [review]
the whole patch this time 

assuming it works and everything. r=dougt

Comment 38

[Daniel Veditz [:dveditz]]

> I cleaned out unrelated changes from the diff, and clobbered [part
> of the patch]

This worries me... are we reviewing what you're going to be checking in? Are 
those "unrelated changes" you had to remove going to accidentally piggy-back on 
this check-in if you forget to remove them later?

If you can do a clean cvs diff then reviewers can trust a simple cvs commit 
will do the job. Anything else is relying too much on Human Perfection, and if 
we thought we could rely on perfection we wouldn't have a double and triple 
review policy.

Comment 39

[dprice (gone)]

The changes I removed were from 105087  I didn't realize that both bugs had
touched the file until I started doing diffs for the second review.  My plan was
to get approval for both bugs and check them in at the same time.  If it will
make you feel more comfortable, I can put each patch in a separate tree.

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 73180 [details] [diff] [review]
the whole patch this time 

>-PRInt32 ReplaceFileNowOrSchedule(nsIFile* replacementFile, nsIFile* doomedFile )
>+PRInt32 ReplaceFileNowOrSchedule(nsIFile* replacementFile, nsIFile* doomedFile, PRInt32 aMode)
> {
>     PRInt32 result = ReplaceFileNow( replacementFile, doomedFile );
> 
>     if ( result == nsInstall::ACCESS_DENIED )
>     {
>         // if we couldn't replace the file schedule it for later
> #ifdef _WINDOWS
>-        if ( ReplaceExistingWindowsFile(replacementFile, doomedFile) == 0 )
>-            return nsInstall::REBOOT_NEEDED;
>+        if ( (aMode & WIN_SYSTEM_FILE) && 
>+             (ReplaceWindowsSystemFile(replacementFile, doomedFile) == 0) )
>+                return nsInstall::REBOOT_NEEDED;
> #endif

I wonder if we shouldn't put this before the ReplaceFileNow() call? If
the script author said WIN_SYSTEM_FILE on WinNT he might be expecting
we always use the OS-approved mechanism. On the other hand I can't see
why the above won't work on NT in all cases so I don't think we need
another patch.

sr=dveditz

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 73180 [details] [diff] [review]
the whole patch this time 

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 42

[Jaime Rodriguez, Jr.]

ADT2 per ADT/XPInstall triage.

TOPEMBED+ per saari in ADT triage.

Comment 43

[dprice (gone)]

Checked in

Comment 44

[dprice (gone)]

the checkin was horked. 

Comment 45

[dprice (gone)]

Attachment: diff to test on all platforms

Comment 46

[dprice (gone)]

patch builds on mac windows and linux (Thanks simon and akanna)

Checking in

Comment 47

[dprice (gone)]

cleared

Comment 48

[Jimmy Lee]

Build: 2002-03-28-06-trunk(WIN), 2002-03-28-08-trunk(MAC),
2002-03-28-08-trunk(LINUX)

Looks good on all platforms.  And no restart necessary for Windows 98!  Marking 
Verified!

Comment 49

[Stephan Hohe]

I think this should be marked fixed1.0

Comment 50

[dprice (gone)]

It was checked in before we branched.  So there's no need to mark it fixed1.0.0

Comment 51

[Syd Logan]

adding fixed1.0.0 so we can tell which bugs are in the branch