1346680 - Memory leak in VRLayer after VRDisplay calls requestPresent

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[Daosheng Mu[:daoshengmu]]

Reproduce step:
Run test_vrDisplay_requestPresent mochitest at dom/vr/test and remain only request present test like below:

"promise_test((test) => {
            return setupVRDisplay(test).then(() => {
              return WebVRHelpers.RequestPresentOnVRDisplay(vrDisplay, [{ source: canvas }]);
            }).then(() => {
              assert_true(vrDisplay.isPresenting, "vrDisplay.isPresenting must be true if requestPresent is fulfilled.");
              assert_equals(vrDisplay.getLayers().length, 1, "vrDisplay.getLayers() should return one layer.");
                return vrDisplay.exitPresent();
            }).then(() => {
              assert_false(vrDisplay.isPresenting, "vrDisplay.isPresenting must be false if exitPresent is fulfilled.");
              // exitPresent() should reject since we are no longer presenting.
              return promise_rejects(test, null, vrDisplay.exitPresent());
            });
          }, "WebVR requestPresent fulfilled");"

Remove skip-if = true, and run ./mach mochitest dom/vr/test

It would show:

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 71066

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       32       96| 1499072        1|
 632 |VRLayerParent                         |       96       96|       1        1|

nsTraceRefcnt::DumpStatistics: 1332 entries
TEST-INFO | leakcheck | default process: leaked 1 VRLayerParent
14 ERROR TEST-UNEXPECTED-FAIL | leakcheck | default process: 96 bytes leaked (VRLayerParent)

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 71075

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       36       88|  200485        1|
 310 |VRLayerChild                          |       88       88|       2        1|

Comment 1

[Daosheng Mu[:daoshengmu]]

Using NS_INLINE_DECL_THREADSAFE_REFCOUNTING instead of NS_INLINE_DECL_REFCOUNTING in both of VRLayerParent and VRLayerChild can make total objects be 1 in them. I think it should be a good way to change but still have leaks.

Comment 2

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #0)

> It would show:
> 
> == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process
> 71066
> 
>     
> |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
>      |                                      | Per-Inst   Leaked|   Total    
> Rem|
>    0 |TOTAL                                 |       32       96| 1499072    
> 1|
  632 |VRLayerParent                         |      104       104|       2    
 1|
 
VRLayerParent has two leaked if using NS_INLINE_DECL_REFCOUNTING. If usng NS_INLINE_DECL_THREADSAFE_REFCOUNTING would be 1 like comment 0

Comment 3

[Daosheng Mu[:daoshengmu]]

Interesting... I have confirmed VRManagerParent::DeallocPVRLayerParent and VRManagerChild::DeallocPVRLayerChild are called, and VRLayerParent / VRLayerChild's destructor are called as well. I didn't find any possibility that could happen memory leak.

Kip, could you take a look this?

Comment 4

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #3)
> Interesting... I have confirmed VRManagerParent::DeallocPVRLayerParent and
> VRManagerChild::DeallocPVRLayerChild are called, and VRLayerParent /
> VRLayerChild's destructor are called as well. I didn't find any possibility
> that could happen memory leak.
> 
> Kip, could you take a look this?

I think probably the problem is aPtr/aType at NS_NS_LogCtor() is a VRLayerParent, but aPtr at NS_LogDtor() is a PVRLayerParent, and the size of this pointer does not match (96 vs 40 bytes). I will digest it deeper tomorrow.

Comment 5

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #4)
> (In reply to Daosheng Mu[:daoshengmu] from comment #3)
> > Interesting... I have confirmed VRManagerParent::DeallocPVRLayerParent and
> > VRManagerChild::DeallocPVRLayerChild are called, and VRLayerParent /
> > VRLayerChild's destructor are called as well. I didn't find any possibility
> > that could happen memory leak.
> > 
> > Kip, could you take a look this?

  I think probably the problem is aPtr/aType at NS_NS_LogCtor() is a
  VRLayerParent, but aPtr at NS_LogDtor() is a PVRLayerParent, and the size of
  this pointer does not match (96 vs 40 bytes). I will diagnose it deeper
  tomorrow.

Sorry for typo...

Comment 6

[Daosheng Mu[:daoshengmu]]

If we use both of NS_INLINE_DECL_REFCOUNTING and MOZ_COUNT_DTOR that will make us duplicate reference like https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/xpcom/base/nsISupportsImpl.h#96 mentions. So, MOZ_COUNT_DTOR is redundant at the constructor of VRLayerParent/Child.

Then, we need to think about whether we should adopt NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING. NS_INLINE_DECL_THREADSAFE_REFCOUNTING makes more sense to me because we need use IPC to communicate between them and keep thread-safe is important to care.

At last, due to PVRLayerParent calls NS_LogDtor (40 Bytes) at its destructor and it is wrong for VRLayerParent (96 Bytes). We should let VRLayerParent to override this. Therefore, remaining MOZ_COUNT_DTOR() at ~VRLayerParent (96 Bytes) is a good way to go.

Comment 7

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1346680 - Duplicate refcount on VRLayerParent/Child when the construction;

Review commit: https://reviewboard.mozilla.org/r/120034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120034/

Comment 8

[:kip (Kearwood Gilbert)]

(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> If we use both of NS_INLINE_DECL_REFCOUNTING and MOZ_COUNT_DTOR that will
> make us duplicate reference like
> https://dxr.mozilla.org/mozilla-central/rev/
> f9362554866b327700c7f9b18050d7b7eb3d2b23/xpcom/base/nsISupportsImpl.h#96
> mentions. So, MOZ_COUNT_DTOR is redundant at the constructor of
> VRLayerParent/Child.
> 
> Then, we need to think about whether we should adopt
> NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING.
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING makes more sense to me because we need
> use IPC to communicate between them and keep thread-safe is important to
> care.
> 
> At last, due to PVRLayerParent calls NS_LogDtor (40 Bytes) at its destructor
> and it is wrong for VRLayerParent (96 Bytes). We should let VRLayerParent to
> override this. Therefore, remaining MOZ_COUNT_DTOR() at ~VRLayerParent (96
> Bytes) is a good way to go.

Good find, and thanks for taking this @daosheng

Please ask if I can provide any help on it.

Comment 9

[:kip (Kearwood Gilbert)]

Comment on attachment 8846991 [details]
Bug 1346680 - Duplicate refcount on VRLayerParent/Child when the construction;

https://reviewboard.mozilla.org/r/120034/#review122218

Excellent find, thanks!

Comment 10

[Daosheng Mu[:daoshengmu]]

(In reply to :kip (Kearwood Gilbert) from comment #8)
> (In reply to Daosheng Mu[:daoshengmu] from comment #6)
> > If we use both of NS_INLINE_DECL_REFCOUNTING and MOZ_COUNT_DTOR that will
> > make us duplicate reference like
> > https://dxr.mozilla.org/mozilla-central/rev/
> > f9362554866b327700c7f9b18050d7b7eb3d2b23/xpcom/base/nsISupportsImpl.h#96
> > mentions. So, MOZ_COUNT_DTOR is redundant at the constructor of
> > VRLayerParent/Child.
> > 
> > Then, we need to think about whether we should adopt
> > NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING.
> > NS_INLINE_DECL_THREADSAFE_REFCOUNTING makes more sense to me because we need
> > use IPC to communicate between them and keep thread-safe is important to
> > care.
> > 
> > At last, due to PVRLayerParent calls NS_LogDtor (40 Bytes) at its destructor
> > and it is wrong for VRLayerParent (96 Bytes). We should let VRLayerParent to
> > override this. Therefore, remaining MOZ_COUNT_DTOR() at ~VRLayerParent (96
> > Bytes) is a good way to go.
> 
> Good find, and thanks for taking this @daosheng
> 
> Please ask if I can provide any help on it.

Thanks for review. I will make you know if I need any help.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5fd5117ce4e
Duplicate refcount on VRLayerParent/Child when the construction; r=kip

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5fd5117ce4e