1384701 - Stylo: Linux debug parallel reftest / mochitest runs crash on shutdown

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See all 8 crashing reftest runs here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6813000f2a777de4ed8d410522ef9dc4bf8f6de5&filter-searchStr=test-linux64-stylo%2Fdebug%20tc-R(

An example log at the crash:

https://treeherder.mozilla.org/logviewer.html#?job_id=117730537&repo=try&lineNumber=39514

The tests themselves seem to pass, but then we crash on shutdown, perhaps because of the assertion "firefox: ../../../../src/cairo-hash.c:217: _cairo_hash_table_destroy: Assertion `hash_table->live_entries == 0' failed."

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:bholley, this is blocking the work to test on more desktop platforms.  We haven't been testing the configuration of Stylo + e10s off + parallel traversal correctly, which I ran into while working on bug 1380053.

What's the right way to proceed here?  Do we want to fix it even though it's e10s off only?  If so, who is best placed to do so?  Or else should we disable this test configuration?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'll try disabling these runs for now to unblock the other platforms at least.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Mochitest style runs are also affected.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like the crashing thread goes through cairo, which then tries to call "firefox!arena_dalloc [mozjemalloc.cpp:6813000f2a77 : 1115 + 0x5]" which appears to fail.

:bholley and I discussed on IRC that we'll disable this configuration for now to unblock other platforms while we investigate a fix here.

:glandium, any ideas about the cairo / jemalloc issue here?  It only appears to happen for e10s-off + parallel Stylo + debug runs on Linux64, so necessary options include:

STYLO_THREADS=4
STYLO_FORCE_ENABLED=1
--disable-e10s
(debug build)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Just to emphasize it: we aren't testing this correctly on try currently, because we were not setting STYLO_THREADS to 4 when e10s was off.  So we'll need to double-check the try runs when testing any fix to be sure we've covered the right thing here.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Now that bug 1380053 has landed (which disabled the failing configs here), you can re-enable them for testing on try by searching tests.yml[1] for the lines mentioning this bug and removing them so that e10s-off runs as well.

[1]: https://hg.mozilla.org/mozilla-central/diff/30fb5403e45c/taskcluster/ci/test/tests.yml

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The same issue also seems to occur in a parallel _e10s on_ run of the DevTools test devtools/client/responsive.html/test/browser/browser_window_close.js.

Comment 8

[Mike Hommey [:glandium]]

After some painful experience with a one-click loaner (most of which revolves around bug 1386946), I managed to get a gdbserver attached to the crashing firefox, which just showed that our crash reporter can't really handle to get a good stack trace from system libraries, even when the debug packages are installed (incidentally, libc's was, but not libcairo's), and the trace in the logs is just quoting irrelevant things.

Here the stack trace from gdb's perspective:

Thread 1 "firefox" received signal SIGABRT, Aborted.
0x00007ffff6b79428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff6b79428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6b7b02a in __GI_abort () at abort.c:89
#2  0x00007ffff6b71bd7 in __assert_fail_base (fmt=<optimized out>, 
    assertion=assertion@entry=0x7ffff343eaad "hash_table->live_entries == 0", 
    file=file@entry=0x7ffff343ea90 "../../../../src/cairo-hash.c", line=line@entry=217, 
    function=function@entry=0x7ffff343ec70 <__PRETTY_FUNCTION__.10447> "_cairo_hash_table_destroy")
    at assert.c:92
#3  0x00007ffff6b71c82 in __GI___assert_fail (
    assertion=assertion@entry=0x7ffff343eaad "hash_table->live_entries == 0", 
    file=file@entry=0x7ffff343ea90 "../../../../src/cairo-hash.c", line=line@entry=217, 
    function=function@entry=0x7ffff343ec70 <__PRETTY_FUNCTION__.10447> "_cairo_hash_table_destroy")
    at assert.c:101
#4  0x00007ffff33975bc in _cairo_hash_table_destroy (hash_table=<optimized out>)
    at ../../../../src/cairo-hash.c:217
#5  0x00007ffff33cd697 in _cairo_scaled_font_map_destroy ()
    at ../../../../src/cairo-scaled-font.c:444
#6  0x00007ffff338ed69 in cairo_debug_reset_static_data () at ../../../../src/cairo-debug.c:67
#7  0x00007fffe8a563d7 in MOZ_gdk_display_close(_GdkDisplay*) ()
   from target:/home/worker/workspace/build/application/firefox/libxul.so
#8  0x00007fffe8a5e90c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) ()
   from target:/home/worker/workspace/build/application/firefox/libxul.so
#9  0x00007fffe8a5ea80 in XRE_main(int, char**, mozilla::BootstrapConfig const&) ()
   from target:/home/worker/workspace/build/application/firefox/libxul.so
#10 0x00000000004064b6 in do_main(int, char**, char**) ()
#11 0x0000000000405c5c in main ()

Which says two things:
- yes, the assertion "firefox: ../../../../src/cairo-hash.c:217: _cairo_hash_table_destroy: Assertion `hash_table->live_entries == 0' failed." is what's causing it
- no, jemalloc has nothing to do with it.

Comment 9

[Bobby Holley (:bholley)]

I see this assertion all the time locally with --disable-e10s on linux64.

Comment 10

[Bobby Holley (:bholley)]

See also bug 1387490.

Comment 12

[Cameron McCormack (:heycam)]

Jonathan, do you know what the cause of this assertion (_cairo_scaled_font_map_destroy is not empty) generally is?  Is it leaking a gfxFontEntry?

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Given this blocks more mochitests, it should pretty much have a higher priority.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #12)
> Jonathan, do you know what the cause of this assertion
> (_cairo_scaled_font_map_destroy is not empty) generally is?  Is it leaking a
> gfxFontEntry?

Yes, that seems likely. It might be interesting to catch this in a debugger (rr?) and see exactly what entry is left there during shutdown.

(I tried to reproduce this and bug 1387490 in a local linux64 build with stylo, but so far I'm finding them elusive here...)

Comment 15

[Bobby Holley (:bholley)]

Are you passing --disable-e10s? That makes it happen consistently for me.

Comment 16

[Jonathan Kew [:jfkthame]]

Yes, I'm using --disable-e10s.

I did get the assertion once after loading the testcase from bug 1387490 and then quitting the browser, but then when I tried to repeat that in order to record/debug it, I couldn't get it to happen again. I'm guessing something racy may be going on, and my particular system has a hard time getting the timing just right to catch it.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

FWIW, this seems to affect non-stylo as well. See bug 1369815 where the test change leads to a shutdown assertion similar to bug 1387490.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Related try push for comment 18: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06dc0a37528eddc566bf22c4db30090d2e24922c&selectedJob=123311402

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Yes, I'm using --disable-e10s.
> 
> I did get the assertion once after loading the testcase from bug 1387490 and
> then quitting the browser, but then when I tried to repeat that in order to
> record/debug it, I couldn't get it to happen again. I'm guessing something
> racy may be going on, and my particular system has a hard time getting the
> timing just right to catch it.

You may want to clear all cache before trying again probably?

Also you can probably try applying the patch in bug 1369815 then executing "mach mochitest layout/style/test/chrome". This is a reasonably small test set, which, according to that bug, can reproduce this issue without stylo.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This also appears to affect Linux 32-bit Stylo runs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4225df6913ab00cb97bcdb17b8c29a24ee52673b

(Ignore the browser chrome part, that seems unrelated...) Mochitest chrome, a11y, and clipboard seem to trigger this issue on Linux 32-bit with Stylo enabled.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

So I did several try push yesterday with the assertion removed... but those patch doesn't seem to work.

It seems on Linux, we use the system cairo rather than our in-tree one, and it seems we don't switch to the in-tree one even if I make MOZ_TREE_CAIRO set unconditionally in old-configure.in. I have no idea why...

Comment 23

[Mike Hommey [:glandium]]

We *are* using the in-tree cairo. However, *gtk* is using system cairo, and there's no way around that.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

So, not all tests trigger this assertion.

I've figured out that accessible/tests/mochitest/test_aria_token_attrs.html is one test which can trigger this shutdown assertion in a11y.

Given the testcase in bug 1387490, I suspect this is related to the <input>s in this test as well.

Comment 25

[Jonathan Kew [:jfkthame]]

I'm still finding this very hard to reproduce locally with any consistency.

I did catch it in rr once (after loading the testcase from bug 1387490, then quitting the browser), and poked around enough to confirm that we had one stray gfxFont instance still alive with refcnt=1. This was an instance of the DejaVu Serif font, which was created early in the run (it was the second gfxFont instance to be created).

Exploring this led me to notice bug 1393185, which was causing a lot of refcounting noise, though I don't believe it is directly connected to the eventual assertion here. But having touched my build there, I can no longer replay that recording in order to investigate further; and now I can't seem to trigger the assertion again to get a fresh recording. :(

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO, back Aug 28) from comment #24)
> I've figured out that accessible/tests/mochitest/test_aria_token_attrs.html
> is one test which can trigger this shutdown assertion in a11y.

I got this test to trigger the cairo hashtable assertion during shutdown, though it's worth noting that it is preceded by a couple of other assertions as well, starting with a LOUD WARNING ABOUT JSRuntime LEAKAGE:

GECKO(9593) | WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
GECKO(9593) | [9593] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /home/jkew/mozdev/mozilla-central/xpcom/build/XPCOMInit.cpp, line 1040
GECKO(9593) | #01: mozilla::ShutdownXPCOM(nsIServiceManager*) (/home/jkew/mozdev/mozilla-central/xpcom/build/XPCOMInit.cpp:1040)
GECKO(9593) | #02: NS_ShutdownXPCOM (/home/jkew/mozdev/mozilla-central/xpcom/build/XPCOMInit.cpp:825)
GECKO(9593) | #03: ScopedXPCOMStartup::~ScopedXPCOMStartup() (/home/jkew/mozdev/mozilla-central/toolkit/xre/nsAppRunner.cpp:1478)
GECKO(9593) | #04: operator delete(void*) (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:224)
GECKO(9593) | #05: XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/jkew/mozdev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4905)
GECKO(9593) | #06: mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/jkew/mozdev/mozilla-central/toolkit/xre/Bootstrap.cpp:46)
GECKO(9593) | #07: do_main(int, char**, char**) (/home/jkew/mozdev/mozilla-central/browser/app/nsBrowserApp.cpp:237 (discriminator 2))
GECKO(9593) | #08: main (/home/jkew/mozdev/mozilla-central/browser/app/nsBrowserApp.cpp:309)
GECKO(9593) | #09: __libc_start_main (/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:325)
GECKO(9593) | #10: _start (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox)
GECKO(9593) | #11: ??? (???:???)
GECKO(9593) | [9593] ###!!! ASSERTION: 2543 dynamic atom(s) with non-zero refcount: New Folder…,inactive,menu_openFile,panel-wide-item,onDOMMenuInactive,context_unpinTab,context_reloadTab,notification-anchor-icon,context-sep-ctp,addon-webext-perm-notification-content,BMB_bookmarksPopup,menu_sendLink,cmd_cut,View:ReaderView,pointerlock-warning,cmd_quitApplication,Email Image…,X,onsizemodechange,messageCloseButton,...: 'nonZeroRefcountAtomsCount == 0', file /home/jkew/mozdev/mozilla-central/xpcom/ds/nsAtom
GECKO(9593) | #01: nsACString::~nsACString() (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTSubstring.h:347)
GECKO(9593) | #02: mozilla::BaseAutoLock<mozilla::Mutex>::~BaseAutoLock() (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Mutex.h:171)
GECKO(9593) | #03: mozilla::ShutdownXPCOM(nsIServiceManager*) (/home/jkew/mozdev/mozilla-central/xpcom/build/XPCOMInit.cpp:1050)
GECKO(9593) | #04: NS_ShutdownXPCOM (/home/jkew/mozdev/mozilla-central/xpcom/build/XPCOMInit.cpp:825)
GECKO(9593) | #05: ScopedXPCOMStartup::~ScopedXPCOMStartup() (/home/jkew/mozdev/mozilla-central/toolkit/xre/nsAppRunner.cpp:1478)
GECKO(9593) | #06: operator delete(void*) (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:224)
GECKO(9593) | #07: XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/jkew/mozdev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4905)
GECKO(9593) | #08: mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/jkew/mozdev/mozilla-central/toolkit/xre/Bootstrap.cpp:46)
GECKO(9593) | #09: do_main(int, char**, char**) (/home/jkew/mozdev/mozilla-central/browser/app/nsBrowserApp.cpp:237 (discriminator 2))
GECKO(9593) | #10: main (/home/jkew/mozdev/mozilla-central/browser/app/nsBrowserApp.cpp:309)
GECKO(9593) | #11: __libc_start_main (/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:325)
GECKO(9593) | #12: _start (/home/jkew/mozdev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox)
GECKO(9593) | #13: ??? (???:???)
GECKO(9593) | firefox: ../../../../src/cairo-hash.c:217: _cairo_hash_table_destroy: Assertion `hash_table->live_entries == 0' failed.
TEST-INFO | Main app process: killed by out-of-range signal, number 134
Buffered messages finished
238 ERROR TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/test_aria_token_attrs.html | application terminated with exit code -134

I don't know whether those earlier leak messages have any connection with the cairo-hash issue (due to holding something alive beyond the expected shutdown time) or not.

However, if I run this under rr, the assertions magically go away; despite repeated attempts, it consistently exits cleanly. Which makes investigation harder... and suggests that this may be some kind of timing-dependent shutdown race, and the overhead of rr recording (on my machine, anyhow) is enough to perturb the behavior and prevent it happening.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

It's... mysterious.

So if we run only the specific test on try, we leak the world as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d148caee5ca701aa4a7a983caa1d2218cf66b15d&filter-tier=3&selectedJob=125062322

However, if we run several tests, we end up having this assertion without leaking the world: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aebe4461764721d163c8b59870d06f869cf57159&filter-tier=3&selectedJob=125029871

That says, there may actually be two different issues on this...


It is also weird that why this happens only for debug build. If this is an assertion in system cairo (which gtk uses), then it should always assert, or never assert, if the leak happens consistently. If this is timing-dependent, then probably we are just lucky on opt builds that this situation never happens...

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO, back Aug 28) from comment #27)

> It is also weird that why this happens only for debug build. If this is an
> assertion in system cairo (which gtk uses), then it should always assert, or
> never assert, if the leak happens consistently. If this is timing-dependent,
> then probably we are just lucky on opt builds that this situation never
> happens...

IIRC, this assertion comes from (somewhere underneath) cairo_debug_reset_static_data, which I don't believe we call at all in release builds.

Comment 29

[Jonathan Kew [:jfkthame]]

Bug 1393585 is another report of what I'd guess is probably the same issue. :jesup reports (on irc) similar inability to reproduce under rr.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

I have repro'd it under rr -- add the -h flag when recording (chaos mode).  Unfortunately, my kernel has a bug and when I tried to replay the trace, rr failed -- I'll update my kernel/etc and try again.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

So, if I skip the whole MOZ_gdk_display_close (via undefining CLEANUP_MEMORY in toolkit/xre/nsAppRunner.cpp), the task would finish successfully without any leak detected. [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=18c0d85723fbb3289843068e1409817626da898d&filter-tier=3

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Actually... it doesn't seem to fail anymore! Let me see other tasks.

Comment 33

[Bobby Holley (:bholley)]

Karl is going to try to reproduce.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

FWIW, as I mentioned in bug 1387993 comment 4, this crash goes away for most of the tasks, probably because of bug 1383332 and/or bug 1393632 which make us do fewer parallelism. tc-M(c1) on Linux x64 seems to be the only affected task now. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=183bca8aff06dc698683c94cdea36489e21c40a5&selectedJob=126332600

Comment 35

[Bobby Holley (:bholley)]

And in particular, using a checking from before that bug landed (last week) might improve reproducibility.

Comment 37

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1387490 discovered this same issue via fuzzing.  It has a small test case that might aid in reproducing, however it was found before the traversal changes that Xidorn mentions in comment 34, so you may still need to go back in time before those traversal changes for the best change at reproduction.

Comment 39

[Bobby Holley (:bholley)]

Bug 1393585 may also have useful STR here.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So if commenting out the assertion shows that we don't get any of our own leak reports when the assertion would have happened, that suggests the problem might be related to the memory management of GTK, GDK, or cairo objects.

Have we done anything to verify that any of those objects whose reference-counting is single-threaded are being reference-counted only on the correct threads?  It seems like reference-count races could lead to something like this.

Or might there be similar issues related to management of statics, either in our code or in those libraries?

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #40)
> So if commenting out the assertion shows that we don't get any of our own
> leak reports when the assertion would have happened, that suggests the
> problem might be related to the memory management of GTK, GDK, or cairo
> objects.

Not so fast... I probably should do a try push again with an earlier commit, because the push in comment 31 doesn't show anything simply because it doesn't fail anymore as I mentioned in comment 32, and I realized what happened after that (in comment 34).

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #41)
> Not so fast... I probably should do a try push again with an earlier commit,
> because the push in comment 31 doesn't show anything simply because it
> doesn't fail anymore as I mentioned in comment 32, and I realized what
> happened after that (in comment 34).

Alright, I just did a try push [1] with a m-c commit on Aug 21 which is supposed to still have this problem, and without triggering the assertion, we don't have any other leak, so it probably means the problem is related to other libraries we link to.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9564909c81aa6abdacdda26d17a7122bfa9f585&filter-tier=3

Comment 43

[Karl Tomlinson (:karlt)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #18)
> FWIW, this seems to affect non-stylo as well. See bug 1369815 where the test
> change leads to a shutdown assertion similar to bug 1387490.

https://bugzilla.mozilla.org/show_bug.cgi?id=1369815#c35 seems to indicate
that was a PresContext leak, and so is unrelated.

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Karl Tomlinson (:karlt) from comment #43)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #18)
> > FWIW, this seems to affect non-stylo as well. See bug 1369815 where the test
> > change leads to a shutdown assertion similar to bug 1387490.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1369815#c35 seems to indicate
> that was a PresContext leak, and so is unrelated.

Yep, that one is unrelated.

Comment 45

[Karl Tomlinson (:karlt)]

I'm yet to reproduce this bug locally.

I've run the build from comment 42, with STYLO_THREADS=4 STYLO_FORCE_ENABLED=1
in the environment, against gtk, pango, cairo, and glibc libraries from Ubuntu
16.04, with Ubuntu's Ambiance theme, and failed to reproduce with steps of bug
1387490 and bug 1393585.  I've tried this with and without rr record -h.

I've similarly run caps/tests/mochitest/test_bug995943.xul against a recent
local build and against a local build based on e16dba457260 (i.e. before
changes for bug 1383332 and bug 1393632).

I did catch one world leak, but this did not trigger this assertion.
This was with rr record -h from a local build of e16dba457260, closing the
browser after loading https://bugzilla.mozilla.org/attachment.cgi?id=8893860.
e10s was disabled but there was still a child (perhaps for tab previews?):

There were many messages such as:

ERROR: GC found live Cell 0x4c8b51c9a040 of kind FUNCTION at shutdown

ERROR: GC found 2706 live Cells at shutdown
ERROR: GC found 2293 live Cells at shutdown

ERROR: GC found live SharedScriptData 0x38f37e4824a0 with ref count 2 at shutdown

and then

[Child 26082] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /home/karl/moz/dev/xpcom/build/XPCOMInit.cpp, line 1040

[Parent 25579] WARNING: Fonts still alive while shutting down gfxFontCache: 'mFonts.Count() == 0', file /home/karl/moz/dev/gfx/thebes/gfxFont.cpp, line 217
Assertion failed at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-hash.c:196: hash_table->live_entries == 0
WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
[Parent 25579] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /home/karl/moz/dev/xpcom/build/XPCOMInit.cpp, line 1040
firefox: /var/tmp/portage/media-libs/fontconfig-2.11.1-r2/work/fontconfig-2.11.1/src/fccache.c:525: FcCacheFini: Assertion `fcCacheChains[i] == NULL' failed.

As I have a recording, I could debug that, but it seems unrelated to this bug.

Comment 46

[Bobby Holley (:bholley)]

(In reply to Karl Tomlinson (:karlt) from comment #45)
> I'm yet to reproduce this bug locally.
> 
> I've run the build from comment 42, with STYLO_THREADS=4
> STYLO_FORCE_ENABLED=1
> in the environment, against gtk, pango, cairo, and glibc libraries from
> Ubuntu
> 16.04, with Ubuntu's Ambiance theme, and failed to reproduce with steps of
> bug
> 1387490 and bug 1393585.  I've tried this with and without rr record -h.

To be clear, the build in comment 42 has the cairo leak checking turned off (with the hopes that it would allow us to reach some gecko leak checking, which didn't happen). So you wouldn't reproduce anything with that build.

> 
> I've similarly run caps/tests/mochitest/test_bug995943.xul against a recent
> local build and against a local build based on e16dba457260 (i.e. before
> changes for bug 1383332 and bug 1393632).
> 
> I did catch one world leak, but this did not trigger this assertion.
> This was with rr record -h from a local build of e16dba457260, closing the
> browser after loading https://bugzilla.mozilla.org/attachment.cgi?id=8893860.
> e10s was disabled but there was still a child (perhaps for tab previews?):

Yes, I see this sometimes as well. Would be interesting to investigate, but ideally we'd focus on the specific cairo crash.

I just grabbed a build (--enable-debug --disable-optimize --enable-stylo) from a few weeks ago that I had lying around, based on [1]. I was able to reproduce it reliably with:

./mach run --disable-e10s https://en.wikipedia.org/wiki/Barack_Obama --debugger=rr --debugger-args="record -h"

The --disable-e10s is necessary to reproduce. The backtrace isn't that useful because my system cairo doesn't have any debug symbols. Not sure if there's an easy way to install symbols after the fact?

So anyway, I have a recording, and can play human gdbserver over IRC if need be. But hopefully you can reproduce it yourself using the above. Let me know.

[1] https://hg.mozilla.org/mozilla-central/rev/d25db0546c92

Comment 47

[Karl Tomlinson (:karlt)]

Thank you for that.

It seems the key to reproducing reliably is to load an input before the
Nightly Start Page.

STYLO_THREADS=4 STYLO_FORCE_ENABLED=1 ./mach run --disable-e10s 'data:text/html,<input>'

will reproduce reliably with the parent of
https://hg.mozilla.org/mozilla-central/rev/a9f5863c9d48
but not with that change for bug 1393632.

I have a recording and will investigate the cause.

(FWIW the world leak happens even with stylo off.)

Comment 48

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee42fbb740f6f965d24d759f20896ebb1dd08f34

Comment 49

[Karl Tomlinson (:karlt)]

Attachment: bug 1384701 remove MOZ_WIDGET_GTK == 2 code from nsLookAndFeel

This old code no longer builds and is superseded by the GTK3 port.
See also bug 1278282.

Review commit: https://reviewboard.mozilla.org/r/176244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176244/

Comment 50

[Karl Tomlinson (:karlt)]

Attachment: bug 1384701 get system font name and size from widget style context instead of GtkSettings

gtk_widget_get_settings() returns the same settings for all widgets [1] but
GTK actually uses specific fonts for each widget [2].

[1] https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c?h=3.22.19#n11637
[2] https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c?h=3.22.19#n10334

Changing to GtkStyleContext* will also make this easier to use from code where
the GtkStyleContext exists but not the GtkWidget, such as EnsureInit() in a
future patch.

Review commit: https://reviewboard.mozilla.org/r/176246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176246/

Comment 51

[Karl Tomlinson (:karlt)]

Attachment: bug 1384701 get system fonts in EnsureInit() which is on main thread even with servo

GTK should be used only from the main thread, but the specific symptom before
this change was that creating a GtkEntry causes
pango_cairo_font_map_get_default() to be called.  This function returns a
different font map on each thread.  The font map is leaked when StyleThreads
are leaked at exit.

Font caches are usually expensive and so using the existing font map on the
main thread is preferable.

A GtkEntry already exists on the main thread, as well as style contexts for
most other system fonts, and so it is more efficient to create these on the
main thread while the style contexts exist.

Doing this also avoids the need for Gecko_nsFont_InitSystem() to hold a global
lock to avoid concurrently calling into GTK through LookAndFeel::GetColor().

Review commit: https://reviewboard.mozilla.org/r/176248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176248/

Comment 52

[Manish Goregaokar [:manishearth]]

Comment on attachment 8904420 [details]
bug 1384701 remove MOZ_WIDGET_GTK == 2 code from nsLookAndFeel

https://reviewboard.mozilla.org/r/176244/#review181370

(might not be the appropriate person to review this, but it looks ok to me)

Comment 53

[Bobby Holley (:bholley)]

Awesome - thanks for figuring this our Karl!

Comment 54

[Manish Goregaokar [:manishearth]]

Comment on attachment 8904421 [details]
bug 1384701 get system font name and size from widget style context instead of GtkSettings

https://reviewboard.mozilla.org/r/176246/#review181384

Comment 55

[Manish Goregaokar [:manishearth]]

Comment on attachment 8904422 [details]
bug 1384701 get system fonts in EnsureInit() which is on main thread even with servo

https://reviewboard.mozilla.org/r/176248/#review181392

looks ok to me, but I might not be the best person to review this

::: commit-message-9ae5f:16
(Diff revision 1)
> +
> +A GtkEntry already exists on the main thread, as well as style contexts for
> +most other system fonts, and so it is more efficient to create these on the
> +main thread while the style contexts exist.
> +
> +Doing this also avoids the need for Gecko_nsFont_InitSystem() to hold a global

should the lock be removed?

Comment 56

[Manish Goregaokar [:manishearth]]

Thanks for this! I'm not overly familiar with all this code so you may want to get a font peer to review it, but it looks good from my side!

Comment 57

[Karl Tomlinson (:karlt)]

(In reply to Manish Goregaokar [:manishearth] from comment #56)
> Thanks for this! I'm not overly familiar with all this code so you may want
> to get a font peer to review it, but it looks good from my side!

Thank you for looking these over.

This is ancient code.  It's been moved around but hasn't really changed
significantly since changes for bug 176842.  The people involved are no longer
around.

I've tested that scaling works as expected with GDK_SCALE=2 in the
environment, and that the style resolves with this in
~/.config/gtk-3.0/gtk.css:

entry {
    font-family: serif;
}

dbaron more or less suggested this change in
https://bugzilla.mozilla.org/show_bug.cgi?id=96971#c0
"It would make it much easier (well, I would be able to avoid duplicating a
lot more code) to implement system fonts correctly on GTK, since for GTK we
have to create native widgets and poke at their style information, and I'd
rather not duplicate all the creation/destruction code that already exists in
nsLookAndFeelGTK for the system colors implementation."

I'm happy if someone else would like to review, but I don't think we need to
wait for dbaron to return and review this.  GTK3 is different from GTK2
anyway.

Comment 58

[Karl Tomlinson (:karlt)]

Comment on attachment 8904422 [details]
bug 1384701 get system fonts in EnsureInit() which is on main thread even with servo

https://reviewboard.mozilla.org/r/176248/#review181392

> should the lock be removed?

My point here was meant to be that, if GTK calls were to continue to be risked
off main thread from nsLookAndFeel::GetFontImpl(), then
Gecko_nsFont_InitSystem() would need to compete with
Gecko_GetLookAndFeelSystemColor() for the same lock.

Each of those functions does hold a lock, but they hold different locks, and
so are not mutually exclusive.

From the perspective of the GTK port, there is no longer a need for any lock
around calls to LookAndFeel::GetFont(), and nsRuleNode::ComputeSystemFont()
also looks thread-safe to me.

However, the WINNT port at least does not look thread-safe [3], and so the
lock can't just be removed.  From a quick look at the Mac port, it might be
thread-safe or possible to make it thread-safe, but that requires more
investigation.

[3] http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/widget/windows/nsLookAndFeel.cpp#713

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4166c5ecc5d7
remove MOZ_WIDGET_GTK == 2 code from nsLookAndFeel r=manishearth
https://hg.mozilla.org/integration/autoland/rev/143ef903d8f6
get system font name and size from widget style context instead of GtkSettings r=manishearth
https://hg.mozilla.org/integration/autoland/rev/e711937ff6fa
get system fonts in EnsureInit() which is on main thread even with servo r=manishearth

Comment 60

[Mike Hommey [:glandium]]

(In reply to Pulsebot from comment #59)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/4166c5ecc5d7
> remove MOZ_WIDGET_GTK == 2 code from nsLookAndFeel r=manishearth

What does this have to do with this bug? Wasn't that still kept for redhat?

Comment 61

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #60)
> What does this have to do with this bug?

The file modified here is hard to understand with all the GTK2 #ifs.
It is much easier to understand the associated code portions and changes here without the many #ifs.

Maintaining support for GTK2 in the system fonts code modified to address this
bug would require more #ifs.

> Wasn't that still kept for redhat?

https://bugzilla.mozilla.org/show_bug.cgi?id=1278282#c7 indicated that RedHat
no longer need this.

Comment 62

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

https://hg.mozilla.org/mozilla-central/rev/4166c5ecc5d7
https://hg.mozilla.org/mozilla-central/rev/143ef903d8f6
https://hg.mozilla.org/mozilla-central/rev/e711937ff6fa