Closed Bug 995893 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: m_kato, Assigned: masayuki)

References

Details

Attachments

(3 files)

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)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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)
Attachment #8454442 - Flags: review?(jmathies)
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.
Attachment #8454445 - Flags: review?(jmathies)
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
Attachment #8454442 - Flags: review?(jmathies) → review+
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.
Attachment #8454445 - Flags: review?(jmathies) → review+
(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!
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Thank you. I'll use the message.

# m-i was closed :-(
https://hg.mozilla.org/mozilla-central/rev/05ae727d8db2
https://hg.mozilla.org/mozilla-central/rev/5d68184741ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.