Closed
Bug 1134565
Opened 9 years ago
Closed 9 years ago
Angle uses system operator new
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: [MemShrink])
Attachments
(2 files)
1.01 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
gps
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
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•9 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.
Comment 2•9 years ago
|
||
glandium++
Comment 3•9 years ago
|
||
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•9 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•9 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•9 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)
Updated•9 years ago
|
Attachment #8566875 -
Flags: review?(gps) → review+
Attachment #8566874 -
Flags: review?(vladimir) → review+
Updated•9 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a279774a1f3b
Assignee | ||
Comment 8•9 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)
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a279774a1f3b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•9 years ago
|
Attachment #8566874 -
Flags: checkin?(vladimir)
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
Dan, can you please help upstream attachment 8566875 [details] [diff] [review]?
Flags: needinfo?(dglastonbury)
Assignee | ||
Comment 12•9 years ago
|
||
It's not even about upstreaming, it's about pushing to http://github.com/mozilla/angle.
Comment 14•9 years ago
|
||
https://github.com/mozilla/angle/commit/e656ff457f832879a935919f7a7fdf874f84c5a2
Updated•9 years ago
|
Attachment #8566875 -
Flags: checkin?(vladimir) → checkin+
You need to log in
before you can comment on or make changes to this bug.
Description
•