995893 - TEST-UNEXPECTED-FAIL | leakcheck | 8 bytes leaked (nsTArray_base) with intl.tsf.enable=true

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P5)

Description

[Makoto Kato [:m_kato]]

Attachment: try_win7-ix-debug_test-mochitest-1-bm109-tests1-windows-build700.txt.gz

This is memo for TSF.   When turn on TSF, all tests cause memory leak.


https://tbpl.mozilla.org/?tree=Try&rev=3f1d77b5bfb2

...
00:18:53     INFO -   => Process ID: 2128, Thread ID: 2096
00:18:53     INFO -  TEST-INFO | Main app process: exit status 0
00:18:53     INFO -  INFO | runtests.py | Application ran for: 0:49:14.181000
00:18:53     INFO -  INFO | zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpbtrlygpidlog
00:18:53     INFO -  ==> process 2128 launched child process 2336 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=2128.272622a8.1731342215 "c:\users\cltbld\appdata\local\temp\tmpl7uwux\plugins\nptest.dll" -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 2128 "\\.\pipe\gecko-crash-server-pipe.2128" plugin)
00:18:53     INFO -  ==> process 2128 launched child process 3572 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=2128.27f46e88.1299679912 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 2128 "\\.\pipe\gecko-crash-server-pipe.2128" tab)
00:18:53     INFO -  ==> process 2128 launched child process 3256 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=2128.281f8f98.1504325541 "c:\users\cltbld\appdata\local\temp\tmpl7uwux\plugins\nptest.dll" -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 2128 "\\.\pipe\gecko-crash-server-pipe.2128" plugin)
00:18:53     INFO -  ==> process 2128 launched child process 472 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=2128.27fc1590.1277776988 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 2128 "\\.\pipe\gecko-crash-server-pipe.2128" tab)
00:18:53     INFO -  Stopping web server
00:18:53     INFO -  Stopping web socket server
00:18:53     INFO -  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 2128
00:18:53     INFO -       |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
00:18:53     INFO -                                                Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
00:18:53     INFO -     0 TOTAL                                          15        8 368095399        2 (39279.45 +/- 74249.92) 337666764        0 ( 5634.38 +/- 13196.61)
00:18:53     INFO -  1549 nsTArray_base                                   4        8 84076135        2 (157543.89 +/- 77043.36)        0        0 (    0.00 +/-     0.00)
00:18:53     INFO -  nsTraceRefcnt::DumpStatistics: 1711 entries
00:18:53     INFO -  TEST-INFO | leakcheck | leaked 2 nsTArray_base (8 bytes)
00:18:53  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 8 bytes leaked (nsTArray_base)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Use nsAutoRefCnt in IUnknown inherited classes and log the refcounting

The reason of this memory leak is that the nsTextStore instance isn't released at shutting down.

There are two causes.

One is fixed by this patch. nsTextStore sets its refcount to 1 in the constructor. Therefore, storing the instance to nsRefPtr causes that the refcounter becomes 2. So, the instance always has wrong refcount.

This patch defines macro in WinUtils.h. It defines and implements AddRef() and Release() for IUnknown inherited classes. The logic is same as NS_INLINE_DECL_REFCOUNTING().

nsAutoRefCnt initializes the refcount to 0 when it's created.

Therefore, this patch fixes the refcount problem and logging the refcount for detecting the leak. (With this patch, we can see nsTextStore's leak in the mochitest's log)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Unregister all references to nsTextStore before releasing its instance

This patch fixes the other bug.

Now, the destructor of nsTextStore calls UnadviseSink(). This is necessary to decrease refcount of nsTextStore. However, the destructor has never been performed because TSF framework grabs the instance!

This patch makes nsTextStore::Terminate() call OnTerminate() for freeing the instance from TSF framework first. Then, nsTextStore must be released by a call of NS_IF_RELEASE() macro.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8454442 [details] [diff] [review]
part.1 Use nsAutoRefCnt in IUnknown inherited classes and log the refcounting

Review of attachment 8454442 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.h
@@ +44,5 @@
> +    return static_cast<ULONG>(mRefCnt.get());                                 \
> +  }                                                                           \
> +  STDMETHODIMP_(ULONG) Release()                                              \
> +  {                                                                           \
> +    MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");                          \

nit - "dup release"? what does 'dup' mean? :)

::: widget/windows/nsTextStore.cpp
@@ +2224,5 @@
>      if (mInputScopeRequested) {
>        paAttrVals->varValue.vt = VT_UNKNOWN;
> +      nsRefPtr<IUnknown> inputScope = new InputScopeImpl(mInputScopes);
> +      paAttrVals->varValue.punkVal = inputScope.forget().take();
> +    } 

nit - a bit of whitespace here

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8454445 [details] [diff] [review]
part.2 Unregister all references to nsTextStore before releasing its instance

Review of attachment 8454445 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +3869,5 @@
>  {
>    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS, ("TSF: nsTextStore::Terminate()"));
>  
> +  if (sTsfTextStore) {
> +    sTsfTextStore->PrepareToTerminate();

hmm, this method naming seems confusing. how about changing PrepareToTerminate() to something like Shutdown(), which meshes well with your existing Init method.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] from comment #3)
> > +  STDMETHODIMP_(ULONG) Release()                                              \
> > +  {                                                                           \
> > +    MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");                          \
> 
> nit - "dup release"? what does 'dup' mean? :)

I guess that it's "double" or something. I just copied it from nsISupportsImpl.h.

Should it be "already released"?

> ::: widget/windows/nsTextStore.cpp
> @@ +2224,5 @@
> >      if (mInputScopeRequested) {
> >        paAttrVals->varValue.vt = VT_UNKNOWN;
> > +      nsRefPtr<IUnknown> inputScope = new InputScopeImpl(mInputScopes);
> > +      paAttrVals->varValue.punkVal = inputScope.forget().take();
> > +    } 
> 
> nit - a bit of whitespace here

Oh, thanks!

Comment 6

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #3)
> > > +  STDMETHODIMP_(ULONG) Release()                                              \
> > > +  {                                                                           \
> > > +    MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");                          \
> > 
> > nit - "dup release"? what does 'dup' mean? :)
> 
> I guess that it's "double" or something. I just copied it from
> nsISupportsImpl.h.
> 
> Should it be "already released"?
> 
 
I'd go with something better that 'dup release' seeing as how we control the assert here. "Release called on object that has already been released!"  or something similar. Up to you.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you. I'll use the message.

# m-i was closed :-(

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d68184741ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ae727d8db2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05ae727d8db2
https://hg.mozilla.org/mozilla-central/rev/5d68184741ea