Re: [PATCH] fix unable to load *.pyd files with Python 3.8.x on Windows

On 2020/05/18 2:51, Johan Corveleyn wrote:
> On Sun, May 17, 2020 at 3:43 PM Jun Omae <jun66j5_at_gmail.com> wrote:

>> Assertion for negative ref count is raised from test_conflict
>> (client.SubversionClientTestCase).
>> 1.10.x through trunk have the issue.
 
> Thank you for the thorough research!
> I'm not sure what can be done further about this. Perhaps others can
> chime in here.

Jun and I found it is caused by long existing bug on SWIG Python bindings,
since before swig-py3 branch was merged (thus this bug exists on 1.13.x
and 1.10.3 branches). On SWIG Python bindings, APIs which have multiple
apr_pool_t * argments cannot be wrapped correctly.

If we call such a wrapper function with specifying multiple pool argments
explicitly, only last one is used for *all* apr_pool_t * arguments when
it calls C API. And if we call it without specifying pools, wrapper
function create same number of new temporary pool wrapper Python objects
as pool arguments, however except last one are all disposed without
dereferencing, and last one is dereferenced instead. Thus, it causes
negative reference count assertion in debug mode, and causes memory leak
both in release and debug mode.

In SubversionClientTestCase.test_conflict, the client.conflict_get() call
is the former and the client.conflict_tree_get_resolution_options() is the
latter case.

Actually, the patch below can make test_conflict() pass in debug mode,
however test_inherited_props() still causes negative ref assertion
(on trunk, FreeBSD, Python 2.7, debug build). In case of 1.10.x
and 1.13.x in Jun's report, it was also caused in test_inherited_props().
[[[
Index: subversion/bindings/swig/python/tests/client.py
===================================================================
--- subversion/bindings/swig/python/tests/client.py (revision 1877742)
+++ subversion/bindings/swig/python/tests/client.py (working copy)
@@ -597,11 +597,13 @@
 
     self.assertTrue(isinstance(conflict, client.svn_client_conflict_t))
 
- conflict_opts = client.conflict_tree_get_resolution_options(conflict, self.client_ctx)
+ conflict_opts = client.conflict_tree_get_resolution_options(conflict, self.client_ctx, pool)
 
     self.assertTrue(isinstance(conflict_opts, list))
     self.assert_all_instances_of(conflict_opts, client.svn_client_conflict_option_t)
 
+ del conflict
+ del conflict_opts
     pool.clear()
 
   @unittest.skip("experimental API, not currently exposed")
]]]

I'll try to fix, later.

Cheers,

-- 
Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>/<futatuki_at_yf.bsdclub.org>