Closed
Bug 1187649
Opened 9 years ago
Closed 9 years ago
[gtk3] various valgrind leaks
Categories
(Core :: Widget: Gtk, defect)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
2.00 KB,
patch
|
acomminos
:
review+
|
Details | Diff | Splinter Review |
3.89 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
See https://treeherder.mozilla.org/logviewer.html#?job_id=9775519&repo=try
Some I don't know what they are, but most are rooted in widget code. Some are from code shared with gtk2, so that could indicate leaks in gtk itself. Or, since we have suppressions for gtk2, that could indicate leaks in our code that we're ignoring (see suppressions in build/valgrind/x86_64-redhat-linux-gnu.sup).
It seems to me we've been too prompt to add broad suppressions for gtk/fontconfig/pango/gobject "leaks" that may well root in our code.
One of the leaks in that try run roots in code specific to gtk3 on our end and *does* look like we are leaking something, but I don't know the gtk APIs well enough to tell whether that's true.
If we added similar broad suppressions for gtk3 libs, we'd probably hide real leaks.
Andrew, would you mind looking into this?
Assignee | ||
Comment 2•9 years ago
|
||
Let's separate those issues in two groups:
- leaks
- other errors
There are very many leaks, and fwiw, adding a broad suppression like we have for gtk2 gets rid of them all.
I'll file a separate bug for the latter, and those don't involve our code afaict.
Summary: [gtk3] various valgrind errors → [gtk3] various valgrind leaks
Assignee | ||
Comment 3•9 years ago
|
||
Filed bug 1187664
Assignee | ||
Comment 4•9 years ago
|
||
FWIW, here's a list of the suppressed leaks on Gtk+2 builds (grepping for used_suppression in a valgrind job log):
--23919-- used_suppression: 30 Bug 984196 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:80 suppressed: 47,791 bytes in 36 blocks
--23919-- used_suppression: 213 Bug 794373 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:46 suppressed: 21,980 bytes in 213 blocks
--23919-- used_suppression: 5 dl-hack3-cond-1 /usr/lib64/valgrind/default.supp:1206
--24032-- used_suppression: 5727 Bug 794373 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:46 suppressed: 692,901 bytes in 8,144 blocks
--24032-- used_suppression: 221 dl-hack3-cond-1 /usr/lib64/valgrind/default.supp:1206
--23915-- used_suppression: 1951 Bug 794366 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:32 suppressed: 378,290 bytes in 4,804 blocks
--23915-- used_suppression: 3605 Bug 794373 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:46 suppressed: 504,848 bytes in 5,292 blocks
--23915-- used_suppression: 1 Bug 979242 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:64 suppressed: 20,992 bytes in 1 blocks
--23915-- used_suppression: 3 Bug 794372 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:90 suppressed: 20,704 bytes in 647 blocks
--23915-- used_suppression: 2 Bug 794374 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:99 suppressed: 5,728 bytes in 179 blocks
--23915-- used_suppression: 74 Bug 793598 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:15 suppressed: 11,357 bytes in 90 blocks
--23915-- used_suppression: 30 Bug 794369 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:72 suppressed: 7,208 bytes in 53 blocks
--23915-- used_suppression: 3 Bug 794370 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:81 suppressed: 288 bytes in 6 blocks
--23915-- used_suppression: 1 PR_SetEnv requires its argument to be leaked, but does not appear on stacks. (See bug 793549.) /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:15 suppressed: 170 bytes in 1 blocks
--23915-- used_suppression: 3 Bug 794368 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:39 suppressed: 192 bytes in 3 blocks
--23915-- used_suppression: 1 PR_SetEnv requires its argument to be leaked, but does not appear on stacks. (See bug 793548.) /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:29 suppressed: 72 bytes in 1 blocks
--23915-- used_suppression: 73 Bug 793537 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/x86_64-redhat-linux-gnu.sup:8 suppressed: 2,880 bytes in 90 blocks
--23915-- used_suppression: 1 See bug 793535 /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:43 suppressed: 32 bytes in 1 blocks
--23915-- used_suppression: 9 PR_SetEnv requires its argument to be leaked, but does not appear on stacks. (See bug 793534.) /builds/slave/m-in-l64-valgrind-000000000000/src/build/valgrind/cross-architecture.sup:8 suppressed: 160 bytes in 9 blocks
--23915-- used_suppression: 29 dl-hack3-cond-1 /usr/lib64/valgrind/default.supp:1206
"Bug 794366" is suppressions of everything involving libgtk-x11
"Bug 794373" is suppressions of everything involving libgobject
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4)
> FWIW, here's a list of the suppressed leaks on Gtk+2 builds (grepping for
> used_suppression in a valgrind job log)
... after the landing of bug 1187662
Comment 6•9 years ago
|
||
Most of these leaks appear to be internal to GTK/GDK/GLib/Pango/Cairo after some initial inspection. We are, however, definitely leaking a widget path- I'll post a patch to fix that.
Comment 7•9 years ago
|
||
Attachment #8639412 -
Flags: review?(karlt)
Updated•9 years ago
|
Attachment #8639412 -
Flags: review?(karlt) → review+
Updated•9 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 9•9 years ago
|
||
backed out for causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12190147&repo=mozilla-inbound
Flags: needinfo?(acomminos)
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Ah, sorry about that- forgot to put it in a MOZ_WIDGET_GTK == 3 block. Fixing.
Flags: needinfo?(acomminos)
Comment 12•9 years ago
|
||
Attachment #8639412 -
Attachment is obsolete: true
Attachment #8639834 -
Flags: review+
Comment 13•9 years ago
|
||
Should be fine now. Try push for safety's sake: https://treeherder.mozilla.org/#/jobs?repo=try&revision=052b0744dbe
Keywords: checkin-needed
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 16•9 years ago
|
||
This is what the leaks look like with debug symbols, and with the first patch from this bug applied.
https://treeherder.mozilla.org/logviewer.html#?job_id=9893633&repo=try
Looks like there are 3 big families:
- Fontconfig leaks. They all involve FcPatternObjectInsertElt and realloc. Here is an interesting comment related to these: https://code.google.com/p/skia/issues/detail?id=2879#c1 which leads to https://bugs.freedesktop.org/show_bug.cgi?id=8215 and https://bugs.freedesktop.org/show_bug.cgi?id=8428. I guess the problem is that valgrind is seeing them as lost when they are still referenced, only not by pointer. If pointers were used, valgrind would put them as still reachable, which is still a leak in some sense... anyways, clearly not our problem, and valid for a suppression.
- Gtk leaks. They all look like we are doing the right thing.
- Cairo leaks. They all are completely out of our reach. That is, we're not involved in dealing with cairo for those code paths, it's all in gtk.
Assignee | ||
Comment 17•9 years ago
|
||
I'm testing suppressions for Fontconfig and Cairo, but I'm not sure how to come up with suppressions for the Gtk leaks that wouldn't have been suppressed the real leak that was fixed by https://hg.mozilla.org/mozilla-central/rev/3bb4268b9651
Comment 18•9 years ago
|
||
Thank you for investigating these leaks so carefully, glandium.
Assignee | ||
Comment 19•9 years ago
|
||
Assignee: acomminos → mh+mozilla
Attachment #8640841 -
Flags: review?(n.nethercote)
Comment 20•9 years ago
|
||
Comment on attachment 8640841 [details] [diff] [review]
Add valgrind suppressions for leaks in Gtk+3, cairo and fontconfig that are not due to Gecko
Review of attachment 8640841 [details] [diff] [review]:
-----------------------------------------------------------------
::: build/valgrind/x86_64-redhat-linux-gnu.sup
@@ +27,5 @@
> fun:FcDefaultSubstitute
> fun:_ZN17gfxPangoFontGroup11MakeFontSetEP14_PangoLanguagedP9nsAutoRefI10_FcPatternE
> ...
> }
> +# Fontconfig is going fancy is its cache structure and that confuses valgrind.
Nit: "with its"?
Attachment #8640841 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Updated•9 years ago
|
Blocks: valgrind-tbpl-bugs
Comment 21•9 years ago
|
||
Comment 23•9 years ago
|
||
The cairo leaks at least may be due to not closing the display, which we don't do with early GTK3 versions because of a GTK bug.
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•