1187649 - [gtk3] various valgrind leaks

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mike Hommey [:glandium]]

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?

Comment 1

[Andrew Comminos [:acomminos]]

Sure, I'll take a look at this.

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Mike Hommey [:glandium]]

Filed bug 1187664

Comment 4

[Mike Hommey [:glandium]]

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

Comment 5

[Mike Hommey [:glandium]]

(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

[Andrew Comminos [:acomminos]]

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

[Andrew Comminos [:acomminos]]

Attachment: Free constructed widget path in nsLookAndFeel on GTK.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac04ab483fba

Comment 9

[Carsten Book [:Tomcat]]

backed out for causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12190147&repo=mozilla-inbound

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0cb66ac149f

Comment 11

[Andrew Comminos [:acomminos]]

Ah, sorry about that- forgot to put it in a MOZ_WIDGET_GTK == 3 block. Fixing.

Comment 12

[Andrew Comminos [:acomminos]]

Attachment: Free constructed widget path in nsLookAndFeel on GTK. Carry r=karlt

Comment 13

[Andrew Comminos [:acomminos]]

Should be fine now. Try push for safety's sake: https://treeherder.mozilla.org/#/jobs?repo=try&revision=052b0744dbe

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb4268b9651

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3bb4268b9651

Comment 16

[Mike Hommey [:glandium]]

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.

Comment 17

[Mike Hommey [:glandium]]

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

[Nicholas Nethercote [inactive]]

Thank you for investigating these leaks so carefully, glandium.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Add valgrind suppressions for leaks in Gtk+3, cairo and fontconfig that are not due to Gecko

Comment 20

[Nicholas Nethercote [inactive]]

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"?

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03afc91034dc

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03afc91034dc

Comment 23

[Karl Tomlinson (:karlt)]

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.