debug build crashes on exit with memory corruption

RESOLVED FIXED in mozilla1.9beta4

Status

()

--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: a, Assigned: a)

Tracking

({crash})

Trunk
mozilla1.9beta4
x86
Windows XP
crash
Points:
---
Bug Flags:
blocking1.9 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: 1/22/2008 trunk

Upon exiting any xulrunner app, windows xulrunner crashes with corrupt heap.
The problem is in nsWindowsWMain.cpp. We clone argv, pass argvClone to main(), then delete argvClone. This crashes, because main() rearranges argv behind our back. I'll attach a sample patch.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Assignee)

Comment 1

11 years ago
Created attachment 298666 [details] [diff] [review]
Sample patch for the nsWindowsWMain.cpp
Attachment #298666 - Flags: review?
Assignee: nobody → a
Status: UNCONFIRMED → NEW
Component: XULRunner → XRE Startup
Ever confirmed: true
QA Contact: xulrunner → xre.startup
Version: unspecified → Trunk
Attachment #298666 - Flags: review? → review?(benjamin)
Severity: normal → critical
Status: NEW → ASSIGNED
Keywords: crash

Comment 2

11 years ago
Comment on attachment 298666 [details] [diff] [review]
Sample patch for the nsWindowsWMain.cpp

check deleteUs for OOM |if (!deleteUs) {
  FreeAllocStrings(argc, argvConverted);
  return 127;
}|

or something...
Attachment #298666 - Flags: review?(benjamin) → review-
(Assignee)

Comment 3

11 years ago
Created attachment 298783 [details] [diff] [review]
Revised patch, added OOM check.

Added OOM check per instructions. 

Is checking for OOM on all allocations the offical FF policy? I can't imagine how we would get OOM on startup, but I can understand being extra careful for security reasons.
Attachment #298783 - Flags: review?
Attachment #298783 - Flags: review? → review?(benjamin)

Comment 4

11 years ago
Comment on attachment 298783 [details] [diff] [review]
Revised patch, added OOM check.

You don't ever delete[] deleteUs... was that intentional?
Attachment #298783 - Flags: review?(benjamin) → review-
(Assignee)

Comment 5

11 years ago
deleteUs gets deleted inside FreeAllocString. That's the way old code worked.

(Assignee)

Updated

11 years ago
Attachment #298783 - Attachment description: Revised patch, added OOM check → Revised patch, added OOM check.
Attachment #298783 - Flags: review- → review?
Attachment #298783 - Flags: review? → review?(benjamin)
This patch also fixes the shutdown crash that occurs when running XUL apps with Firefox using the "-app" flag

Updated

11 years ago
Attachment #298783 - Flags: review?(benjamin) → review+
(Assignee)

Updated

11 years ago
Flags: blocking1.9?
Attachment #298783 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Assignee: a → dougt
Status: ASSIGNED → NEW
Aleks, why did you reassign this bug to Doug?
(Assignee)

Comment 8

11 years ago
So he can check it in for me. I do not have write privileges (I think). And doug is sitting right next to me.
(Assignee)

Comment 9

11 years ago
Or do you want to check it in? (reed)
The assignee is always the person who wrote the patch. This can't be checked-in right now because it does not have approval. Is this needed for beta 3, or can it wait until after the beta 3 freeze is over?
Assignee: dougt → a

Comment 11

11 years ago
reed, when/if this is approved, please check in.
(In reply to comment #11)
> reed, when/if this is approved, please check in.

Will do.
Status: NEW → ASSIGNED

Updated

11 years ago
Attachment #298783 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in toolkit/xre/nsWindowsWMain.cpp;
/cvsroot/mozilla/toolkit/xre/nsWindowsWMain.cpp,v  <--  nsWindowsWMain.cpp
new revision: 1.4; previous revision: 1.3
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4

Updated

10 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.