en-US.jar and toolkit.jar not replaced after update.html

VERIFIED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
P3
major
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: Jimmy Lee, Assigned: dveditz)

Tracking

({pp})

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++])

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
Build: 2000-10-04-09-MN6(WIN) Win NT only

1. Open 
ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2000-10-04-09-MN6/upd
ate.html and click Launch XPInstall button
2. After Successful update, exit and relaunch browser
3. If Profile Manager appears, choose one and start browser

RESULT:
en-US.jar and toolkit.jar not replaced from Chrome directory.  Browser does 
startup.  Titlebar still shows the old Build ID.  Checking Help menu and 
selecting About Netcape does show the correct updated Build ID.

EXPECTED RESULT:
All jar files replaced.  Correct new Build ID appears on browser title bar.
(Reporter)

Comment 1

17 years ago
Nominating for rtm.  We need to investigate why certain files are not getting 
replaced after an update.  There may be some lost functionality if some files 
are not replaced.  It is expected that a browser can fail if the files replaced 
are very different.
Keywords: pp, rtm
(Assignee)

Comment 2

17 years ago
This will kill the SmartUpdate site. Possibly affects Unix as well since we have
a similar replace mechanism there. Win9x has a different problem. This could be
a tricky startup timing issue and may require something of a redesign. Or it
might be very easy :-)
Whiteboard: [rtm+ need info]

Updated

17 years ago
Assignee: dveditz → ssu

Comment 3

17 years ago
okay, I finally traced it down to what's going wrong.
so, here is the order of the files it tries to replace the files at startup.  
It's not a race condition at all:

  comm.jar
  modern.jar
  net2phone.jar
  classic.jar
  toolkit.jar
  en-US.jar

The buffer that the filenames get stored at does not get zero'ed out nor does it 
append a terminating NULL when copying a new filename into it.  This means that:

  comm.jar      - works
  modern.jar    - works
  net2phone.jar - works
  classic.jar   - returns DOES_NOT_EXIST.  buffer used to be "net2phone.jar"
                                                 but now has "classic.jarar"
  toolkit.jar   - returns DOES_NOT_EXIST.  buffer used to be "classic.jarar"
                                                 but now has "toolkit.jarar"
  en-US.jar     - returns DOES_NOT_EXIST.  buffer used to be "toolkit.jarar"
                                                 but now has "en-US.jararar"

The fix is zero out the memory buffers before the new filenames get copied into 
them.  The attached patch to mozilla/xpinstall/src/ScheduleTasks.cpp will fix 
this bug.

I think this might also fix mac as well.

Reassigning this bug to myself since I have a patch.

Comment 4

17 years ago
Created attachment 16599 [details] [diff] [review]
patch

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

17 years ago
Created attachment 16617 [details] [diff] [review]
Alternate fix
(Assignee)

Comment 6

17 years ago
Created attachment 16618 [details] [diff] [review]
Same, slightly more self-documenting
(Assignee)

Comment 7

17 years ago
Let me suggest an alternate patch. The real problem is that the code is not 
explicitly storing the null when using REGTYPE_ENTRY_BYTES (I have no one to 
cvs blame but myself). Adding one to the stored lengths makes the problem go 
away (I used "sizeof('\0')" to document this a little better).

The rest of the patch is adding nsXPIDLCString to fix leaks and make the code 
more robust in the face of future changes. I also removed the two memsets 
entirely as they just masked the real problem and adding overhead to the 
processing time. Since these replaces happen at startup every little bit helps.

Comment 8

17 years ago
reassigning back to Dan.
Assignee: ssu → dveditz
Status: ASSIGNED → NEW
(Assignee)

Comment 9

17 years ago
Created attachment 16902 [details] [diff] [review]
Minimal patch so as not to scare the PDT
(Assignee)

Comment 10

17 years ago
Ok, so I propose the minimal 10/12 patch for the branch, and the second 10/10
patch (with memory leak fixes) for the trunk. I'll need super reviews of both.

Both patches are r=dbragg/ssu

Comment 11

17 years ago
sr=mscott for both patches.
(Assignee)

Updated

17 years ago
Whiteboard: [rtm+ need info] → [rtm+]

Comment 12

17 years ago
Marking minimal patch shown as rtm++ for landing on the branch.
Whiteboard: [rtm+] → [rtm++]
(Assignee)

Comment 13

17 years ago
Checked 2-line patch into branch, larger patch with leak fix into trunk
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

17 years ago
Build: 2000-10-16-09-MN6(WIN)

Looks good on the branch.  Files are getting replaced as expected.  Verified for 
branch.
Keywords: vtrunk

Comment 15

17 years ago
dveditz, has this made it's way into the trunk.  if not, perhaps it shouldn't be
marked fixed.
(Assignee)

Comment 16

17 years ago
See 10/13/2000 comment - yes, checked into both trunk and branch. Appears not 
to be verified on the trunk.
(Reporter)

Comment 17

17 years ago
Build 2000-11-07-08-Mtrunk(WIN)

Looks good on the TRUNK.  Marking Verified.  Removing vtrunk from Keywords.
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.