Closed Bug 413599 Opened 13 years ago Closed 13 years ago

debug build crashes on exit with memory corruption

Categories

(Toolkit :: Startup and Profile System, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: a, Assigned: a)

Details

(Keywords: crash)

Attachments

(2 files)

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.
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 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-
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 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-
deleteUs gets deleted inside FreeAllocString. That's the way old code worked.

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
Attachment #298783 - Flags: review?(benjamin) → review+
Flags: blocking1.9?
Attachment #298783 - Flags: approval1.9?
Assignee: a → dougt
Status: ASSIGNED → NEW
Aleks, why did you reassign this bug to Doug?
So he can check it in for me. I do not have write privileges (I think). And doug is sitting right next to me.
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
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
Attachment #298783 - Flags: approval1.9? → approval1.9+
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
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
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.