Angle uses system operator new

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

Assignee

Description

4 years ago
A snarky (and misled) comment added in bug 774755 in libEGL/libGLESv2 Makefiles made its way through to angle's moz.build in bug 1010371, with the effective result of making angle not use mozalloc.h's operator new, and use the system one instead.

In practice, it means OOM will not abort(), but emit an exception, which fortunately should crash, but more importantly, on Windows (and fortunately Windows only), it means Angle likely doesn't allocate memory with jemalloc!

I'll note that neither of those patches were reviewed by a build system peer... I'll also note it's the third time bug 1010371 hits my radar, which, to me means something should be changed with the way angle is managed in mozilla-central.
Assignee

Comment 1

4 years ago
(In reply to Mike Hommey [:glandium] from comment #0)
> A snarky (and misled) comment added in bug 774755 in libEGL/libGLESv2
> Makefiles made its way through to angle's moz.build in bug 1010371, with the
> effective result of making angle not use mozalloc.h's operator new, and use
> the system one instead.

Heh, missing a part here: The snarky comment was accompanying a DISABLE_STL_WRAPPING = True.
glandium++
FWIW, I intend to use the reviewer metadata being added in bug 1132771 to enable not only automatic reviewer selection, but also to power auditing for changes that don't get proper review. If all goes according to plan, the machines should be able to automatically flag patches not receiving adequate review before they land.
Assignee

Comment 4

4 years ago
FWIW, I want to kill the use of generate_mozbuild.py for both skia and angle. It is possible to refer to gyp files from moz.build directly, and while the gyp processor doing that is currently laden with webrtc specific context, it should be possible to use it for angle and skia.

I however don't want to block fixing this bug on that, so I'll just fix generate_mozbuild.py and the in-tree moz.build for now.
Assignee

Comment 5

4 years ago
This looks awkward, but the generated moz.builds for libEGL and libGLESv2 are byte-equivalent to the ones in-tree with this patch.

I'll leave it to you to land that in the angle repository.
Attachment #8566874 - Flags: review?(vladimir)
Assignee

Comment 6

4 years ago
This is the result of running generate_mozbuild.py as modified by attachment 8566874 [details] [diff] [review], and gets us back to where we were wrt stl wrapping in angle before bug 1010371.
Attachment #8566875 - Flags: review?(gps)
Attachment #8566875 - Flags: review?(gps) → review+
Whiteboard: [MemShrink]
Assignee

Comment 8

4 years ago
Comment on attachment 8566874 [details] [diff] [review]
Fix generate_mozbuild.py to not set DISABLE_STL_WRAPPING in gfx/angle

Please push to http://github.com/mozilla/angle.
Attachment #8566874 - Flags: checkin?(vladimir)
https://hg.mozilla.org/mozilla-central/rev/a279774a1f3b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8566874 - Flags: checkin?(vladimir)
Comment on attachment 8566875 [details] [diff] [review]
Remove DISABLE_STL_WRAPPING in gfx/angle

Sorry, got a bit mixed up there with the Gecko patch. Setting the checkin? flag annoying makes this show up on our bug queries :(
Attachment #8566875 - Flags: checkin?(vladimir)
Dan, can you please help upstream attachment 8566875 [details] [diff] [review]?
Flags: needinfo?(dglastonbury)
Assignee

Comment 12

4 years ago
It's not even about upstreaming, it's about pushing to http://github.com/mozilla/angle.
Sure thing. I can push to gh/moz/angle.
Flags: needinfo?(dglastonbury)
Attachment #8566875 - Flags: checkin?(vladimir) → checkin+
You need to log in before you can comment on or make changes to this bug.