Last Comment Bug 758010 - C++ new/delete aren't wrapped with jemalloc on Android
: C++ new/delete aren't wrapped with jemalloc on Android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: ARM Android
: -- blocker (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 695068 758773
Blocks: 758858 740194 746794
  Show dependency treegraph
 
Reported: 2012-05-23 14:22 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-27 17:44 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
betaN+


Attachments
Wrap operator new/delete on Android (3.71 KB, patch)
2012-05-26 01:23 PDT, Mike Hommey [:glandium]
khuey: review+
joe: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Wrap operator new/delete to jemalloc on Android (1.90 KB, patch)
2012-05-31 13:52 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Wrap operator new/delete to jemalloc on Android. (2.19 KB, patch)
2012-05-31 14:32 PDT, Mike Hommey [:glandium]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-05-23 14:22:43 PDT
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.
Comment 1 Mike Hommey [:glandium] 2012-05-24 00:36:35 PDT
Other than ANGLE and Skia, there's also other-licenses/skia-npapi and ... js/src :-/
Comment 2 Mike Hommey [:glandium] 2012-05-26 01:23:48 PDT
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/
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-27 23:01:49 PDT
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.
Comment 4 Mike Hommey [:glandium] 2012-05-27 23:19:55 PDT
https://hg.mozilla.org/mozilla-central/rev/ef0180b53eae
Comment 5 Mike Hommey [:glandium] 2012-05-28 11:18:33 PDT
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.
Comment 6 Joe Drew (not getting mail) 2012-05-28 11:40:23 PDT
Comment on attachment 627451 [details] [diff] [review]
Wrap operator new/delete on Android

Please land ASAP for the Fennec beta build.
Comment 7 Mike Hommey [:glandium] 2012-05-28 11:56:51 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/3a0cedb8ecec
Comment 8 Mike Hommey [:glandium] 2012-05-31 13:51:13 PDT
Brown paper bag: while operator new/delete were correctly wrapped, they were wrapped to system malloc/free...
Comment 9 Mike Hommey [:glandium] 2012-05-31 13:52:28 PDT
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>
Comment 10 Mike Hommey [:glandium] 2012-05-31 14:32:15 PDT
Created attachment 628904 [details] [diff] [review]
Wrap operator new/delete to jemalloc on Android.

Forgot to remove a (moved) prototype.
<none>
Comment 11 Mike Hommey [:glandium] 2012-05-31 14:50:29 PDT
https://hg.mozilla.org/mozilla-central/rev/306365557d64
Comment 12 Mike Hommey [:glandium] 2012-05-31 14:58:40 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/540694f5df42
Comment 13 Julian Seward [:jseward] 2012-06-01 00:23:21 PDT
(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.
Comment 14 Alex Keybl [:akeybl] 2012-06-01 11:02:56 PDT
(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?
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-06-02 14:41:50 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.