147672 - Crash installing langpacks

Status: VERIFIED FIXED

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

Description

[Mike Kaply [:mkaply]]

We are getting crashes installing langpacks on RC3 on OS/2.

We traced it down to an XP issue:

http://lxr.mozilla.org/mozilla1.0/source/xpinstall/src/nsInstall.cpp#2837

This is freeing an nsString buffer. It should not be there.

Comment 1

[Daniel Veditz [:dveditz]]

Over to Darin, this is a result of the nsIFile landing. Other suspicious spots
(diff shows a char* -> nsCAutoString change but no getting rid of free) might be
 nsLoggingProgressNotifier.cpp
 nsSoftwareUpdate.cpp
 nsInstallPatch.cpp
 nsInstallFileOpItem.cpp
 nsDownloadManager.cpp
 nsDownloadProxy.cpp
 nsFilePicker.cpp (beos)
 xcDll.cpp (need to double check m_registrylocation and m_dllName frees)
 nsDiskCacheDevice.cpp
 
There were other places that fit this pattern but actually fixed leaks leftover
from a botched nsFileSpec ->nsIFile conversion; it's possible some of the above
are OK too, I only looked at what little context was available in the diffs.

I didn't look at the mailnews diffs.

Comment 2

[Daniel Veditz [:dveditz]]

OK, the mozilla folks are trying to push this release out and I can't reach
Darin. Taking back

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I went through the files in comment 1 and I didn't see any double-frees,
although I didn't go through xcDll.cpp carefully enough.  From looking at the
nsFileSpec code, I think the comment in nsLoggingProgressNotifier.cpp is wrong,
although I'm not sure.

Comment 4

[Daniel Veditz [:dveditz]]

All the other spots were OK -- most turned out to be leaks that Darin fixed
(sometimes leaks leftover from an nsFileSpec->nsIFile conversion, but many times
in newer files due to lameness).

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: fix for the nsInstall.cpp crash

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 85311 [details] [diff] [review]
fix for the nsInstall.cpp crash

r=dbaron

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 85311 [details] [diff] [review]
fix for the nsInstall.cpp crash

sr=shaver.  Thanks for the detective work.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 85311 [details] [diff] [review]
fix for the nsInstall.cpp crash

a=valeski,rjesup,scc

Comment 9

[Daniel Veditz [:dveditz]]

This is branch-only, the trunk was fine. Fix checked in.

Comment 10

[Daniel Veditz [:dveditz]]

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

Comment 11

[scottputterman]

adding adt1.0.0+ for checkin to the 1.0 branch.  Please get drivers approval and
then check it in and mark it fixed1.0.0

Comment 12

[scottputterman]

looks like this was checked in on the branch so adding fixed1.0.0

Comment 13

[Jimmy Lee]

Michael Kaply, can you confirm that this fix allows you to install langpacks on
OS/2 without crashing?  Thanks.

Comment 14

[Mike Kaply [:mkaply]]

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

Comment 15

[Jimmy Lee]

Adding keyword verified1.0.1 for Verified on BRANCH.  Marking status as VERIFIED
since this was not a problem on TRUNK from comment #9.