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.

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

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

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

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

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?

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.

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...

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 on attachment 48306 [details] [diff] [review] possible fix r=dveditz

This needs to go on the branch and trunk

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.

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

Comment on attachment 48306 [details] [diff] [review] possible fix sr=dveditz

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

check it in - PDT+

I'll take this now.

Checked Sean's patch into mozilla trunk and branch.

Build: 2001-10-02-05-0.9.4(WIN) Looks good on the branch. This still needs to be verified on the trunk.

Build: 2001-10-24-09-trunk(WIN) Works for trunk. Marking Verified!

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

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

I'll take this for now

DPRICEFIX

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

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 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)

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.

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.

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

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 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.

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

>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.

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 on attachment 73180 [details] [diff] [review] the whole patch this time assuming it works and everything. r=dougt

> 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.

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 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 on attachment 73180 [details] [diff] [review] the whole patch this time a=asa (on behalf of drivers) for checkin to the 1.0 trunk

ADT2 per ADT/XPInstall triage. TOPEMBED+ per saari in ADT triage.

Checked in

the checkin was horked.

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

cleared

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!

I think this should be marked fixed1.0

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

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