Closed Bug 486974 Opened 15 years ago Closed 15 years ago

Reftest: intermittent leak of 'gfxFontEntry, nsStreamLoader, ...'

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: karlt)

References

(
URL
)

Details

(Keywords: fixed1.9.1, intermittent-failure, memory-leak, Whiteboard: [fixed1.9.1b99] )

Attachments

(2 files, 1 obsolete file)

test
1.13 KB, patch
Details | Diff | Splinter Review
cherry pick patch from cairo
9.96 KB, patch
jtd
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
Reftest leak error report was enabled a few builds ago, and:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238954020.1238962539.5163.gz&fulltext=1
Linux mozilla-central unit test on 2009/04/05 10:53:40

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 552 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 6 instances of gfxFontEntry with size 36 bytes each (216 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 6 instances of nsStreamLoader with size 32 bytes each (192 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 12 instances of nsStringBuffer with size 8 bytes each (96 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 12 instances of nsTArray_base with size 4 bytes each (48 bytes total)

TinderboxPrint: reftest<br/>2875/0/138&nbsp;<em class="testfail">LEAK</em>
}

Previous build with same revision did not leak.
Flags: wanted-firefox3.6?
Whiteboard: [orange]
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239137051.1239145640.6464.gz
Linux mozilla-central unit test on 2009/04/07 13:44:11
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239165978.1239174060.726.gz
Linux mozilla-central unit test on 2009/04/07 21:46:18

A worse case:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239193923.1239204404.1340.gz
Linux mozilla-central unit test on 2009/04/08 05:32:03
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 736 bytes during test execution
Component: General → GFX: Thebes
Flags: wanted-firefox3.6?
Product: Firefox → Core
QA Contact: general → thebes
Flags: wanted1.9.1?
Summary: Reftest: intermittent leak of 552 bytes (gfxFontEntry, nsStreamLoader, ...) → Reftest: intermittent leak of 'gfxFontEntry, nsStreamLoader, ...' (552 bytes or more)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239882735.1239888765.16624.gz&fulltext=1
Linux mozilla-1.9.1 unit test on 2009/04/16 04:52:15
TEST-PASS | runtests-leaks | WARNING leaked 460 bytes during test execution
Summary: Reftest: intermittent leak of 'gfxFontEntry, nsStreamLoader, ...' (552 bytes or more) → Reftest: intermittent leak of 'gfxFontEntry, nsStreamLoader, ...'
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241180887.1241184895.11661.gz
Linux mozilla-central unit test on 2009/05/01 05:28:07
I can reproduce this locally with font-face/reftest.list
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
Leaked objects include cairo_font_face_t but not cairo_unscaled_font_t.
cairo_ft_unscaled_font_t may have a list of faces, but it is only destroying the first:

http://hg.mozilla.org/mozilla-central/file/f662f710fead/gfx/cairo/cairo/src/cairo-ft-font.c#l534
Filed cairo bug with a patch
https://bugs.freedesktop.org/show_bug.cgi?id=21706
This should block 1.9.1.  As well as the havoc it is causing on the tinderboxen, leaking the nsStreamLoader means that the entire face of font data is leaked.
Flags: blocking1.9.1?
Attached patch test — — Splinter Review 
The leak only happens when two different cairo_font_faces use the same font face.  This happens when artificial oblique from fontconfig is applied to one face, which turns off embeddedbitmap (which is true by default).

The existing tests are intermittant with the current tests because they are such that the second face is only created when reflow occurs at a time when only some of the src:url() faces have been downloaded.

This test leaks reliably (unless fontconfig settings change the default embeddedbitmap setting).
Attached patch patch for mozilla (obsolete) — — Splinter Review 
Anyone want to check the logic?
Attachment #377012 - Flags: review?
Attachment #377012 - Attachment is patch: true
Comment on attachment 377012 [details] [diff] [review]
patch for mozilla

I don't know who (if anyone) knows this code well.  It was written in 2005
http://cgit.freedesktop.org/cairo/commit/?id=c803908d95d0022463d138f0caee949b14d0cadb
and even the author denies recollection.

John, are you able to look over this, please?

For background, the unscaled_font hash table and the font_face lists exist so that these objects are reused.  (i.e. There are never two copies of the same object.)  The objects are released when their ref count reduces to zero (except for the mutual referencing involved), so the hash table doesn't cache the objects longer than their use.  However, these objects are kept alive, for a period after their last external reference is released, by the scaled font cache.
Attachment #377012 - Flags: review? → review?(jdaggett)
Whiteboard: [orange] → [orange][no more tinderbox logs needed thanks]
The patch that Chris Wilson committed to cairo had some differences.
We might as well use the same patch in Mozilla.
Attachment #377012 - Attachment is obsolete: true
Attachment #377965 - Flags: review?
Attachment #377012 - Flags: review?(jdaggett)
Attachment #377965 - Flags: review? → review?(jdaggett)
Comment on attachment 377965 [details] [diff] [review]
cherry pick patch from cairo

Seems fine, although I wish there were more comments explaining the invariants related to the faces list, I'm still kinda hazy on how on the exact set of states possible for this list.
Attachment #377965 - Flags: review?(jdaggett) → review+
http://hg.mozilla.org/mozilla-central/rev/a6611891e0ff
http://hg.mozilla.org/mozilla-central/rev/d88e6b8fab83
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [orange][no more tinderbox logs needed thanks] → [orange]
Attachment #377965 - Flags: approval1.9.1?
Flags: wanted1.9.1?
Target Milestone: --- → mozilla1.9.2a1
Had to backout the test changes due to some parser issues (ask ted for more details; I just did the dirty work).
http://hg.mozilla.org/mozilla-central/rev/e3b0b664e5dd
http://hg.mozilla.org/mozilla-central/rev/fc8e5ce6bddf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, backed out for immediate bustage. Allow me to explain. We now package all our unit tests up so that we can run them on other machines (http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox-Unittest for example.) For reftest, I have a script that parses the reftest manifests and packages:
1) The directories containing manifests
2) The directories containing test files listed in the manifests
3) The directories referenced by HTTP(...)
The problem here is that your test specifies HTTP(../../..), which works out to be topsrcdir. This broke the packaging script, as it wasn't expecting that. I can fix that, but I'm not going to make the script handle it, since packaging the entire srcdir for reftest would be silly. I can just ignore it, and your test will work, since the file you reference will get packaged since it's in a dir that would otherwise get packaged anyway. My only problem with that is that it's a little weird, since you won't necessarily have access to every file in the srcdir if your test is being run from a pre-packaged build.
Thanks, Shawn and Ted for diagnosing and sorting out the HTTP(../../..) issue.
I'll mark this bug as fixed, but I still intend to check in the reliable test in one form or another.

One option is to move the test from gfx/thebes/crashtests to layout/reftests/font-face.  That's probably good enough for now.  (Maybe we should have a leaktests folder too...)

I would like to add more tests using font resources in the future, and the foreseeable ones could go in layout/reftests/something.

But I wonder whether requiring close proximity of test resources and tests is desirable, or whether proximity of tests and source code is more desirable.

Maybe manifests could specify test resources explicitly and we could remove the HTTP(...) logic, or is that going to cause too many inconsistencies between in tree tests and packaged tests?
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Flags: in-testsuite+ → in-testsuite?
Resolution: --- → FIXED
If you can figure out a sane way to refer to resources in the manifest, we can get the packager script to package them. If you want to file a bug on reftest, do so in Testing:Reftest and CC dbaron and myself.
Put test in layout/reftests/font-face
http://hg.mozilla.org/mozilla-central/rev/f67592bacc20

(In reply to comment #32)
> If you can figure out a sane way to refer to resources in the manifest

I'll file a bug if I come up with some ideas, thanks.
Flags: in-testsuite? → in-testsuite+
Attachment #377965 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 377965 [details] [diff] [review]
cherry pick patch from cairo

a191=beltzner
Flags: blocking1.9.1? → blocking1.9.1-
Whiteboard: [orange] → [needs 1.9.1 landing] [orange]
Depends on: 469518
Whiteboard: [orange] → [fixed1.9.1rc1] [orange]
Whiteboard: [fixed1.9.1rc1] [orange] → [fixed1.9.1b99] [orange]
Whiteboard: [fixed1.9.1b99] [orange] → [fixed1.9.1b99]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: