After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 736774 - Memory leak with Vala 0.25.4
Memory leak with Vala 0.25.4
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Code Generator
0.25.x
Other Linux
: Urgent normal
: ---
Assigned To: Vala maintainers
Vala maintainers
: 786388 (view as bug list)
Depends on:
Blocks: 736016
 
 
Reported: 2014-09-16 23:25 UTC by Jim Nelson
Modified: 2017-08-25 12:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test program (839 bytes, text/x-vala)
2014-09-18 00:58 UTC, Jim Nelson
  Details
Fix regression introduced by ba1fa07 (4.56 KB, patch)
2016-11-12 09:08 UTC, Rico Tzschichholz
none Details | Review
codegen: Don't transfer ownership of local-variable if target-type is unknown (2.77 KB, patch)
2017-08-21 15:41 UTC, Rico Tzschichholz
committed Details | Review

Description Jim Nelson 2014-09-16 23:25:17 UTC
After recompiling Geary with Vala 0.25.4, it will quickly grow to consume system memory while running.  This didn't occur with 0.25.3.
Comment 1 Jim Nelson 2014-09-16 23:43:43 UTC
Another problem I've seen is repeated "unable to open database file" errors in the log.  This takes some time to happen, however.  I suspect it's an out-of-memory issue, not a bona fide SQLite problem.
Comment 2 Jim Nelson 2014-09-16 23:58:05 UTC
Using git bisect I've determined the problem is with the commit at

https://git.gnome.org/browse/vala/commit/?id=ba1fa0759989dcbb9046c7dfc06cce6c4aa23411

Unfortunately, I don't have a minimal test case at the moment.  I know the issues likes there simply by watching Geary's memory usage over a short period of time.

I also don't know if this is a Geary problem or a compiler issue; it's possible Geary is doing something wrong with owned references that was hidden or masked by a compiler bug that's now been corrected.
Comment 3 Jim Nelson 2014-09-17 00:41:35 UTC
The associated bug for that commit is bug #736016.
Comment 4 Luca Bruno 2014-09-17 11:32:06 UTC
I've tried some usage of geary 0.6.3 with both vala versions, but I couldn't find anything evident. What should I do to reproduce the problem?
Comment 5 Jim Nelson 2014-09-17 18:41:59 UTC
I'm doing this on a Gmail account.  It's easiest to see if you have an account with a lot of mail and an All Mail folder.

* Open System Monitor or top, whatever you prefer.
* Start Geary and compose a new message.
* Type a few characters and let the composer auto-save your draft.  Type more, let it save.  Repeat a number of times.
* Unfortunately, now you have to wait a few minutes.  In the background, Geary will auto-synchronize with All Mail.

If you run Geary like this:

$ geary --debug

You will see similar lines in the log while it's synchronizing with All Mail:

 [deb] 11:16:34 0.000931 imap-engine-minimal-folder.vala:123: Gmail:jim@yorba.org:[Gmail]/All Mail: Begin normalizing remote and local folders
** [deb] 11:16:34 0.041060 imap-engine-minimal-folder.vala:252: Gmail:jim@yorba.org:[Gmail]/All Mail: Messages appended/removed (local/remote UIDNEXT=157415/157416 total=109011/109011 diff=1), gathering mail UIDs 84793:157415
 [deb] 11:16:41 7.085519 imap-engine-minimal-folder.vala:275: Gmail:jim@yorba.org:[Gmail]/All Mail: Loaded local (52659) and remote (52667) UIDs, normalizing...
 [deb] 11:16:41 0.092151 imap-engine-minimal-folder.vala:314: Gmail:jim@yorba.org:[Gmail]/All Mail: changes since last seen: removed=0 appended=0 inserted=8
 [deb] 11:16:41 0.305100 imap-engine-minimal-folder.vala:367: Gmail:jim@yorba.org:[Gmail]/All Mail: Finished creating/merging 8 emails
 [deb] 11:16:41 0.031220 imap-engine-minimal-folder.vala:411: Gmail:jim@yorba.org:[Gmail]/All Mail: Notifying of 8 inserted emails since last opened
 [deb] 11:16:41 0.000051 imap-engine-minimal-folder.vala:446: Gmail:jim@yorba.org:[Gmail]/All Mail: Notifying of 2h count change reason (109011 remote messages)
 [deb] 11:16:41 0.000015 imap-engine-minimal-folder.vala:451: Gmail:jim@yorba.org:[Gmail]/All Mail: Completed normalize_folder

There's a delay after the one marked with two stars.  During that moment, Geary's memory usage drives up from ~150MB to ~300MB.  If you keep performing the test, memory usage will continue to drive upward.  This doesn't happen under the prior commit of Vala.

I wish I had an easier way to repro the problem, but that's the best I can do for now.  I'm looking into the code path that's followed in between the two log calls and don't see anything obvious.  If I can narrow down the issue, I will let you know.
Comment 6 Luca Bruno 2014-09-17 19:23:09 UTC
Will retry tomorrow. Can you run with valgrind?
Comment 7 Jim Nelson 2014-09-18 00:53:08 UTC
Alright, I pinpointed it.  It is a very particular bug.  It turns out there are several hot code paths in Geary which use this code pattern, leading to an aggregate number of memory leaks and not one source in particular.

I'll attach a test case which I developed.  In essence, with 0.25.4, if you have a factory method that throws an Error and the returned object is passed directly to another method, the interim reference is not unref'd.  So:

Object create() {
  return new Object();
}

Object create_throws() throws Error {
  return new Object();
}

void func(Object o) {
}

void main() {
  func(create());
  func(create_throws());
}

... the Object from create_throws() will be dangling when the app exits.

The interim reference *must* be passed directly to a method.  There are no dangling references in these forms:

create_throws();
Object o = create_throws(); func(o);

If my test program is built with Vala as of the commit just prior to ba1fa075, all references are dropped properly.
Comment 8 Jim Nelson 2014-09-18 00:58:17 UTC
Created attachment 286430 [details]
Test program

This is a verbose test app which demonstrates the bug.  In particular, the program should print "6" and it does not.  Examining the C code, I can see that the interim reference is being NULL'd prior to being unref'd.
Comment 9 Luca Bruno 2014-09-18 21:17:41 UTC
Can you try the bug736774 branch and check whether your software works correctly?
Comment 10 Jim Nelson 2014-09-18 22:34:14 UTC
As soon as I run Geary I get a fasttop assertion:

*** Error in `./geary': double free or corruption (fasttop): 0x00000000012c5da0 ***

Stack trace:

  • #0 __GI_raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 56
  • #0 __GI_raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 56
  • #1 __GI_abort
    at abort.c line 89
  • #2 __libc_message
    at ../sysdeps/posix/libc_fatal.c line 175
  • #3 malloc_printerr
  • #4 _int_free
    at malloc.c line 3840
  • #5 geary_account_information_construct_from_file
    at /home/jim/git/geary/src/engine/api/geary-account-information.vala line 138
  • #6 geary_account_information_new_from_file
    at /home/jim/git/geary/src/engine/api/geary-account-information.vala line 131
  • #7 geary_engine_add_existing_accounts_async_co
    at /home/jim/git/geary/src/engine/api/geary-engine.vala line 180
  • #8 next_async_callback_wrapper
    at /build/buildd/glib2.0-2.40.0/./gio/gfileenumerator.c line 304
  • #9 g_task_return_now
    at /build/buildd/glib2.0-2.40.0/./gio/gtask.c line 1076
  • #10 complete_in_idle_cb
    at /build/buildd/glib2.0-2.40.0/./gio/gtask.c line 1085
  • #11 g_main_dispatch
    at /build/buildd/glib2.0-2.40.0/./glib/gmain.c line 3064
  • #12 g_main_context_dispatch
    at /build/buildd/glib2.0-2.40.0/./glib/gmain.c line 3663
  • #13 g_main_context_iterate
    at /build/buildd/glib2.0-2.40.0/./glib/gmain.c line 3734
  • #14 g_main_context_iteration
    at /build/buildd/glib2.0-2.40.0/./glib/gmain.c line 3795
  • #15 g_application_run
    at /build/buildd/glib2.0-2.40.0/./gio/gapplication.c line 2114
  • #16 _vala_main
    at /home/jim/git/geary/src/client/application/main.vala line 10
  • #17 __libc_start_main
    at libc-start.c line 287
  • #18 _start

Looking at the generated .c:

		_tmp10_ = g_file_get_path (_tmp9_);
		_tmp8_ = _tmp10_;
		_tmp11_ = _tmp8_;
		_tmp12_ = _tmp11_ == NULL;
		_g_free0 (_tmp11_);
		if (_tmp12_) {
			gchar* _tmp13_ = NULL;
			_tmp13_ = g_strdup ("");
			_g_free0 (_tmp8_);
			_tmp8_ = _tmp13_;
		}
		_tmp14_ = key_file;
		_tmp15_ = _tmp8_;
		g_key_file_load_from_file (_tmp14_, _tmp15_, G_KEY_FILE_NONE, &_inner_error_);
		_g_free0 (_tmp15_);

_tmp10 is being triple-freed (aliased as _tmp11_, _tmp8_, and _tmp15_).
Comment 11 Luca Bruno 2014-09-18 22:37:24 UTC
Thanks for testing. Can you show me the relevant vala code? Btw it's double-freed.
Comment 12 Jim Nelson 2014-09-18 22:39:56 UTC
Just caught that, my analysis wasn't quite right, there is no triple-free, but there is a double-free in both code paths.

That line of Vala is this (src/engine/api/geary-account-information.vala:138):

    key_file.load_from_file(file.get_path() ?? "", KeyFileFlags.NONE);

Maybe the ?? operator is causing the issue?
Comment 13 Luca Bruno 2014-09-18 22:51:05 UTC
Yes, I thought I fixed that. Perhaps it's some other case. This thing is trickier than what it seems. Will work on it tomorrow, thanks.
Comment 14 Luca Bruno 2014-09-19 12:44:51 UTC
I've reverted:

commit c08dd2c82dcfe42f2f6fb23333eadf63ed2d7f22
Author: Luca Bruno <luca.bruno@immobiliare.it>
Date:   Fri Sep 19 14:43:45 2014 +0200

    Revert "Fix regression when assigning owned expressions to unowned variables."
    
    This reverts commit ba1fa0759989dcbb9046c7dfc06cce6c4aa23411.
    
    Due to bug #736774

To be sure we don't release a vala stable with this commit in case I'm not able to fix it in time.
Comment 15 Rico Tzschichholz 2016-11-12 09:08:32 UTC
Created attachment 339681 [details] [review]
Fix regression introduced by ba1fa07

Hope this is the last time we touch this code.
In the owned->unowned case, we want the variable to be floating:
 - If we assign to a local variable we get an error.
 - Otherwise the value is destroyed when goes out of scope.
In the owned->owned case, we want to avoid copies and thus
we transfer the ownership and make the variable non-floating.

Fixes bug 736774
Comment 16 Rico Tzschichholz 2017-08-21 15:39:59 UTC
*** Bug 786388 has been marked as a duplicate of this bug. ***
Comment 17 Rico Tzschichholz 2017-08-21 15:41:22 UTC
Created attachment 358083 [details] [review]
codegen: Don't transfer ownership of local-variable if target-type is unknown
Comment 18 Rico Tzschichholz 2017-08-25 12:49:38 UTC
Attachment 358083 [details] pushed as 1d867ed - codegen: Don't transfer ownership of local-variable if target-type is unknown