C++ new/delete aren't wrapped with jemalloc on Android

RESOLVED FIXED in Firefox 14

Status

--
blocker
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: bjacob, Assigned: glandium)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
ARM
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 betaN+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

That has caused countless crashes in C++ libraries that we import into libxul/libgkmedias: see bug 746794 for ANGLE and bug 740194 for Skia. We need a good definitive solution so we don't keep running into allocator mismatch crashes over and over again.

Updated

7 years ago
Blocks: 746794, 740194
(Assignee)

Comment 1

7 years ago
Other than ANGLE and Skia, there's also other-licenses/skia-npapi and ... js/src :-/
Severity: normal → blocker
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → betaN+
(Assignee)

Updated

7 years ago
Depends on: 758773
(Assignee)

Updated

7 years ago
Blocks: 758858
(Assignee)

Comment 2

7 years ago
Created attachment 627451 [details] [diff] [review]
Wrap operator new/delete on Android

Doing this properly is going to be tough in the given timeframe, so for now, only address the immediate Android problem. Bug 758858 filed for the long-term solution.

Try builds with this patch applied will be there once built:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-b397cfa64854/
Attachment #627451 - Flags: review?(blassey.bugs)
(Assignee)

Updated

7 years ago
Attachment #627451 - Flags: review?(khuey)
Comment on attachment 627451 [details] [diff] [review]
Wrap operator new/delete on Android

Review of attachment 627451 [details] [diff] [review]:
-----------------------------------------------------------------

I'm just going to assume that your mangled names are correct.
Attachment #627451 - Flags: review?(khuey) → review+
(Assignee)

Comment 4

7 years ago
https://hg.mozilla.org/mozilla-central/rev/ef0180b53eae
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Updated

7 years ago
Attachment #627451 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

7 years ago
Comment on attachment 627451 [details] [diff] [review]
Wrap operator new/delete on Android

[Approval Request Comment]
User impact if declined: random crashes due to memory allocator mismatch
Testing completed (on m-c, etc.): Landed on m-c today.
Risk to taking this patch (and alternatives if risky): It shouldn't break anything. In the worst case scenario it might unveil other allocator mismatches, but certainly much less severe than the ones being fixed (and it's a very hypothetical scenario)
String or UUID changes made by this patch: No string change.
Attachment #627451 - Flags: approval-mozilla-aurora?
Comment on attachment 627451 [details] [diff] [review]
Wrap operator new/delete on Android

Please land ASAP for the Fennec beta build.
Attachment #627451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

7 years ago
Depends on: 695068
(Assignee)

Comment 8

7 years ago
Brown paper bag: while operator new/delete were correctly wrapped, they were wrapped to system malloc/free...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

7 years ago
Created attachment 628888 [details] [diff] [review]
Wrap operator new/delete to jemalloc on Android

Also make operator new distinguishible from operator new[] for valgrind. (likewise for delete/delete[]).
<none>
Attachment #628888 - Flags: review?(justin.lebar+bug)

Updated

7 years ago
status-firefox14: fixed → affected
(Assignee)

Comment 10

7 years ago
Created attachment 628904 [details] [diff] [review]
Wrap operator new/delete to jemalloc on Android.

Forgot to remove a (moved) prototype.
<none>
Attachment #628904 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

7 years ago
Attachment #628888 - Attachment is obsolete: true
Attachment #628888 - Flags: review?(justin.lebar+bug)
Attachment #628904 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 11

7 years ago
https://hg.mozilla.org/mozilla-central/rev/306365557d64
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

7 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/540694f5df42
status-firefox14: affected → fixed
(In reply to Mike Hommey [:glandium] from comment #8)
> Brown paper bag: while operator new/delete were correctly wrapped, they were

Ah .. V's cross-DSO checking failed to pick this up because it intercepts
new/new[]/delete/delete[] as well as malloc/free, and so it would not see
that new etc were routing to the wrong place.  Ah well.
(In reply to Mike Hommey [:glandium] from comment #12)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/540694f5df42

Can we also land on MOBILE140_2012053010_RELBRANCH on mozilla-aurora?
(In reply to Alex Keybl [:akeybl] from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > http://hg.mozilla.org/releases/mozilla-aurora/rev/540694f5df42
> 
> Can we also land on MOBILE140_2012053010_RELBRANCH on mozilla-aurora?

done https://hg.mozilla.org/releases/mozilla-aurora/rev/3dbfe52b2649

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.