1173683 - Unfold mozalloc for firefox 39

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Backout changesets beed1c584a22, 9b2413915a5e (bug 868814), 1f34ebc9eb00, 80555e0558a3 (bug 1141660), 22aea188dac5 (bug 1141731) to give us more time to figure the situation with the SDK.

Approval Request Comment
[Feature/regressing bug #]:
Bug 868814 folded mozalloc into mozglue. While it works fine, there are some complications for third party components on Windows. One is crashes, but that might end up just being a linking problem, and another is bug 1168291. While that bug has a patch, there are redistribution issues that we need to sort out first. So until we have those issues sorted out, I'd rather backout to unblock those people building third party components.

[User impact if declined]:
Difficulty to build third party components on Windows.

[Describe test coverage new/current, TreeHerder]:
I did a try build (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c90ceb86feb) which:
- fails e10s tests but beta doesn't run them (and doesn't have e10s)
- failed to build on mac because of bug 1154377
- failed to build on windows because of a conflict with bug 1147447 that will require an additional patch
- failed mochitest-push, but beta doesn't run them
- failed b2g emulator mochitests for some reason (looks like try just doesn't like beta)
- failed gaia tests for whatever reason, but beta doesn't run them

[Risks and why]: 
On non-Windows, this gets us back to where we were before bug 868814 wrt mozalloc, so there's no risk at all. On Windows, this would get us back to the same, if the fix for bug 1147447 wasn't getting in the way. The fix is hopefully straightforward: it should only require adding mozalloc as a dependency to libEGL and libGLES. Hopefully, whether /that/ causes problems should be visible on try.

[String/UUID change made/needed]: None

Comment 2

[Mike Hommey [:glandium]]

Forgot to mention: there are things that piled up on top of those changes from bug 868814 that make it much harder to possibly backout from aurora and central. IOW, those issues will have to be figured out in the next 9 weeks.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Link libEGL and libGLESv2 against mozalloc

Here's the rationale:
- before bug 1147447, libEGL and libGLESv2 were using jemalloc for malloc/free via mozglue (which I don't think they were using directly) and the system allocator operator new/delete. The latter made OOM dealt with C++ exceptions instead of our "standard" OOM mechanism.
- after bug 1147447, libEGL and libGLESv2 use jemalloc for malloc/free/operator new/delete via mozglue. Obviously, we want to keep this property.
- backing out bug 868814 means the operator new->moz_xmalloc->malloc path changes from being all in mozglue to being split between mozalloc and mozglue, with the entry point in mozalloc.
So, explicitly link those libraries against mozalloc. The added runtime dependency shouldn't be a problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df39dac6801

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

That's the additional patch that makes the backout build on windows.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike, how does this issue manifest? Can we reproduce it? (To verify that this works)

Comment 6

[Mike Hommey [:glandium]]

The issue is bug 1168291. People can't use the SDK because bug 868814 removed mozalloc.lib but failed to add mozcrt.lib which is needed instead of it, but whether we can actually redistribute mozcrt.lib is not sure yet (while the problem doesn't exist with mozalloc.lib). Verifying this works would be checking that mozalloc.lib is in the sdk.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

Approved for uplift to beta

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Well, approved once the 2nd patch gets reviewed.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike can you follow up? This can still go into beta 7.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Liz Henry (:lizzard) from comment #8)
> Well, approved once the 2nd patch gets reviewed.

What about the first patch?

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

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

rs=me

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

I must have missed this too - I thought the first patch already had a review. Usually people don't request uplift until there's an r+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dde4eeaaa7ad
https://hg.mozilla.org/releases/mozilla-beta/rev/37c99d72de9a

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8620850 [details] [diff] [review]
Backout changesets beed1c584a22, 9b2413915a5e (bug 868814), 1f34ebc9eb00, 80555e0558a3 (bug 1141660), 22aea188dac5 (bug 1141731) to give us more time to figure the situation with the SDK.

Approved for uplift to beta