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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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]]

Other than ANGLE and Skia, there's also other-licenses/skia-npapi and ... js/src :-/

Comment 2

[Mike Hommey [:glandium]]

Attachment: 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/[e-mail]-b397cfa64854/

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

https://hg.mozilla.org/mozilla-central/rev/ef0180b53eae

Comment 5

[Mike Hommey [:glandium]]

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)]

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]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/3a0cedb8ecec

Comment 8

[Mike Hommey [:glandium]]

Brown paper bag: while operator new/delete were correctly wrapped, they were wrapped to system malloc/free...

Comment 9

[Mike Hommey [:glandium]]

Attachment: 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]]

Attachment: Wrap operator new/delete to jemalloc on Android.

Forgot to remove a (moved) prototype.
<none>

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/306365557d64

Comment 12

[Mike Hommey [:glandium]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/540694f5df42

Comment 13

[Julian Seward [:jseward]]

(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]]

(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?)]

(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