[patch][win32] Fix memleak in terminal

[Ken Takata]

Hi,

I found memory leakage in term_and_job_init() on Win32.
I also added a workaround when AssignProcessToJobObject() failed. This the same
as mch_job_start() in os_win32.c and would be useful on Windows 7 or earlier.
Please check the attached patch.

Regards,
Ken Takata

[Ken Takata]

Hi,

And here is another patch written by mattn, which fixes refcount problem.
Jobs were freed unexpectedly by GC.
Please apply this after my previous patch to avoid conflicts. (That's why I
send this patch, not from mattn.)

Regards,
Ken Takata

[Ozaki Kiichi]

copyID of job of terminal needs to be updated.

https://gist.github.com/ichizok/0f5559a771ae40c15616e20cd3515d2b

(this includes above fix-terminal-refcount.patch)

* Add set_ref_in_term() to update copyID of job of terminal

Ozaki Kiichi

[Bram Moolenaar]

Thanks. Looks like spawn_config also needs to be freed after "failed:".

> @@ -1227,7 +1228,12 @@ term_and_job_init(term_T *term, int rows
> goto failed;
>
> if (!AssignProcessToJobObject(jo, child_process_handle))
> - goto failed;
> + {
> + /* if failing, switch the way to terminate
> + * process with TerminateProcess. */
> + CloseHandle(jo);
> + jo = NULL;
> + }

Shouldn't it goto failed here?

--
This sentence is not sure that it exists, but if it does, it will
certainly consider the possibility that other sentences exist.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Bram Moolenaar]

Thanks!

--
GOD: That is your purpose Arthur ... the Quest for the Holy Grail ...
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[Ken Takata]

Hi Bram,

2017/7/28 Fri 4:47:08 UTC+9 Bram Moolenaar wrote:
> Ken Takata wrote:
>
> > I found memory leakage in term_and_job_init() on Win32.
> > I also added a workaround when AssignProcessToJobObject() failed.
> > This the same
> > as mch_job_start() in os_win32.c and would be useful on Windows 7 or earlier.
> > Please check the attached patch.
>
> Thanks. Looks like spawn_config also needs to be freed after "failed:".
>
> > @@ -1227,7 +1228,12 @@ term_and_job_init(term_T *term, int rows
> > goto failed;
> >
> > if (!AssignProcessToJobObject(jo, child_process_handle))
> > - goto failed;
> > + {
> > + /* if failing, switch the way to terminate
> > + * process with TerminateProcess. */
> > + CloseHandle(jo);
> > + jo = NULL;
> > + }
>
> Shouldn't it goto failed here?

No, it shouldn't. This is the same as mch_job_start() in os_win32.c:
https://github.com/vim/vim/blob/67883b4909d0e9d4c024beb18f02750c6f7e3069/src/os_win32.c#L5098-L5104

If the process has been already assigned to a job object,
AssignProcessToJobObject() fails on Windows 7 or earlier, because a process
can be assigned to only one job object on those OSes.
In that case, it should fall back to a way without using a job object.

Regards,
Ken Takata