1104055 - [Camera][Gecko] Camera pick activity causes a memory leak

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Youngwoo Jo]

Attachment: flame_b2g_procrank.txt

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141113030201

Steps to reproduce:

1. (Important) Launch Camera app and then back to homescreen
2. Launch Messages app
3. New message => attachment => camera
4. Take a picture => select
5. Back from New messages => discard
6. Repeat 3~5 steps


Actual results:

Memory usage of camera app is getting bigger continuously when checking via b2g-procrank.
When repeated 30 times, the memory usage is Rss : 153528K, Pss : 136524K.


Expected results:

Memory usage of camera app should be stable.

Comment 1

[Youngwoo Jo]

This is reproduced in flame master.
And I confirmed nsGlobalWindow for camera pick activity is not released even if the activity finished.

Comment 2

[Youngwoo Jo]

Hi, Kyle.

Long time no see.
I'm Youngwoo Jo. I met you in San Francisco office last year.

Could you take this issue? I think this issue is related to cc/gc.
So this issue seems to need to analyze cc/gc logs.
I think you look like the best engineer for this issue.

I'm preparing cc/gc logs for this issue.

Comment 3

[Youngwoo Jo]

Attachment: cc-edges.1230.1416849964.log.gz

This is a cc log extracted from flame v2.0 camera app process.

Comment 4

[Youngwoo Jo]

Attachment: gc-edges.1230.1416849964.log.gz

This is a gc log extracted from flame v2.0 camera app process.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hi Youngwoo, I'm happy to help.

Running the cycle collector scripts on the provided logs:

    $ python /c/dev/heapgraph/find_roots.py ~/Desktop/cc-edges.1230.1416849964.log
    0xb3a29fb0
    Parsing c:/Users/mozilla/Desktop/cc-edges.1230.1416849964.log. Done loading grap
    h. Reversing graph. Done.
     
    0xb14e23d0 [DOMMediaStream]
        --[Preserved wrapper]--> 0xb0972840 [JS Object (CameraControl)]
        --[type_proto]--> 0xb1eb07f0 [JS Object (CameraControlPrototype)]
        --[getter]--> 0xb11b9800 [JS Object (Function - onAutoFocusMoving)]
        --[parent]--> 0xb1eaf0d0 [JS Object (Window)]
        --[define]--> 0xb09c85e0 [JS Object (Function - define)]
        --[fun_callscope]--> 0xb0998550 [JS Object (Call)]
        --[contexts]--> 0xb09b5790 [JS Object (Object)]
        --[_]--> 0xb09b1e70 [JS Object (Object)]
        --[deferreds]--> 0xb09b5940 [JS Object (Object)]
        --[BlobView]--> 0xb09f5580 [JS Object (Object)]
        --[promise]--> 0xb09f3f00 [JS Object (Object)]
        --[then]--> 0xb09f3f20 [JS Object (Function - prim/promise.then)]
        --[fun_callscope]--> 0xb09f4ba0 [JS Object (Call)]
        --[fail]--> 0xb09f4c40 [JS Object (Array)]
        --[objectElements[2]]--> 0xb1185e60 [JS Object (Function - finish)]
        --[parent]--> 0xb1185a00 [JS Object (Function - finish)]
        --[fun_callscope]--> 0xb09ccdc0 [JS Object (Call)]
        --[**UNKNOWN SLOT 0**]--> 0xb09ccd30 [JS Object (Call)]
        --[n]--> 0xb116b460 [JS Object (Function - newContext/makeErrback/<)]
        --[fun_callscope]--> 0xb09ccd00 [JS Object (Call)]
        --[d]--> 0xb09cc670 [JS Object (Object)]
        --[promise]--> 0xb113bf80 [JS Object (Object)]
        --[then]--> 0xb1140280 [JS Object (Function - prim/promise.then)]
        --[fun_callscope]--> 0xb09c9c40 [JS Object (Call)]
        --[fail]--> 0xb09c9ce0 [JS Object (Array)]
        --[objectElements[2]]--> 0xb0821860 [JS Object (Function - finish)]
        --[**UNKNOWN SLOT 2**]--> 0xb08216a0 [JS Object (Function - newContext/makeE
    rrback/<)]
        --[fun_callscope]--> 0xb09571f0 [JS Object (Call)]
        --[d]--> 0xb11daca0 [JS Object (Object)]
        --[factory]--> 0xb0821380 [JS Object (Function)]
        --[script]--> 0xb11a8f80 [JS Script]
        --[sourceObject]--> 0xb1ef0520 [JS Object (ScriptSource)]
        --[**UNKNOWN SLOT 1**]--> 0xb0972c60 [JS Object (HTMLScriptElement)]
        --[UnwrapDOMObject(obj)]--> 0xb2d8dac0 [FragmentOrElement (xhtml) script app
    ://camera.gaiamobile.org/index.html#pick]
        --[[via hash] mListenerManager]--> 0xb2b02940 [EventListenerManager]
        --[mListeners event=onload listenerType=3 [i]]--> 0xb134a7c0 [CallbackObject
    ]
        --[mIncumbentGlobal]--> 0xb3a29fb0 [nsGlobalWindow #13 inner app://camera.ga
    iamobile.org/index.html#pick]
     
        Root 0xb14e23d0 is a ref counted object with 1 unknown edge(s).
        known edges:
           0xb0972840 [JS Object (CameraControl)] --[UnwrapDOMObject(obj)]--> 0xb14e23d0 

So there's an unknown edge to the nsDOMCameraControl.  From looking at the code it appears that nsDOMCameraControl calls mCameraControl->AddListener, but never calls RemoveListener.  That would appear to set up a cycle that would explain this leak.

Mike, can you take a look at this?

Comment 6

[Vincent Liu[:vliu]]

It seems that nsDOMCameraManager always new CameraControls by getting a new mWindowId every time runs Step 2 to launch the camera app. 

http://lxr.mozilla.org/mozilla-b2g32_v2_0/source/dom/camera/DOMCameraManager.cpp#379

Is it possible we check the hash table before |CameraControls* controls = sActiveWindows->Get(mWindowId)|? If nsDOMCameraManager contains camera control in hash table, we may reuse it to solve thie problem.

Comment 7

[jeremy.kim.leo [:jeremykimleo]]

Attachment: Remove listener on CameraControl

this is WP1 patch to remove listener on nsDOMCameraControl.

Comment 8

[jeremy.kim.leo [:jeremykimleo]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)

> So there's an unknown edge to the nsDOMCameraControl.  From looking at the
> code it appears that nsDOMCameraControl calls mCameraControl->AddListener,
> but never calls RemoveListener.  That would appear to set up a cycle that
> would explain this leak.

we verified kyle's advice. and we are uploading WP1 patch (attachment 8528387 [details] [diff] [review])
(Attachment 8528387 [details] [diff] is just only focused the purpose to remove listener on nsDOMCameraControl.)

Due to never calls RemoveListener, 
it continuously increase memory because of stacking nsGlobalWindow 
which is created by camera's pick activity. 

and we don't know how to work nsDOMCameraControl, 
when DOMCameraControlListener removes, there are some edge case. 
and to fix edge case, we modified the calling of mCameraControl->Shutdown().

in finally, to fix this problem, we should remove DOMCameraControlListener when nsDOMCameraControl released.
(http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#300)

Mike, can you take a look at this?

Comment 9

[jeremy.kim.leo [:jeremykimleo]]

Comment on attachment 8528387 [details] [diff] [review]
Remove listener on CameraControl

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

this is an attempt to verify affect when it calls RemoveListener.

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

jeremy.kim.leo, why not just call mCameraControl->RemoveListener() at the end of the kHardwareClosed[1] case in OnHardwareStateChange()?

1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#1267

Also, when posting a patch, please make sure you include 8 lines of diff context: for |hg diff| and |git diff| this means specifying the -U8 option.

Comment 11

[jeremy.kim.leo [:jeremykimleo]]

(In reply to Mike Habicher [:mikeh] from comment #10)
> jeremy.kim.leo, why not just call mCameraControl->RemoveListener() at the
> end of the kHardwareClosed[1] case in OnHardwareStateChange()?
> 
> 1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.
> cpp#1267
> 

this trial is failed. 
when it starts camera app, it always received CameraControlListener::kHardwareClosed at first of all.
so, if we are just remove listener at here, it always don't have any listener after this.

and can i ask you why it has only one mCameraThread for all DOMCameraControl?
(http://dxr.mozilla.org/mozilla-central/source/dom/camera/CameraControlImpl.cpp#18)

Comment 12

[jeremy.kim.leo [:jeremykimleo]]

Attachment: RemoveListener-On-DOMCameraControl-WP2.patch

I investigated more and more.

because of comment 11, we need another way to remove listener.
so, how about you like this way? let's clear listener on CameraControlImpl::Shutdown()

and after doing this, When I try test, i found that the ~nsGonkCameraControl{StopImpl();} always called recursively and finally go to crash. 
So I changed the location to call stop() into CameraControlImpl::Shutdown().

Could you advice to me?
Thank you

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

I've tried your patch with the following steps:

1. open Camera app
2. press and hold Home button to bring up the task switch (and with logcat open, wait for camera to stop)
3. tap on the Camera app window to reopen it
4. repeat steps 2 and 3.

I don't see any destructors called on any of the iterations, so there's definitely a leak here. I will investigate.

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v1

Attachment 8529119 [details] [diff] didn't completely address this bug, since two other classes were also listening to ICameraControl, along with the per-window registry. This patch addresses those issues as well. tl;dr:

1. strong registry pointers replaced with weak references;
2. replaces all MOZ_ASSERT(!mCameraControl) instances with error reports;
3. makes CameraCapabilities and CameraRecorderProfiles listen for hardware-closed events, to clean up;
4. adds mAudioChannelAgent to nsDOMCameraControl's CYCLE_COLLECTION list;
5. removes some stable code from CameraControlImpl.

With the above changes, I have confirmed that the cycle collector picks up stale instances of nsDOMCameraCOntrol. Usually 2 to 3 instances hang around, but they are eventually cleaned up as well.

Will push to try once it reopens.

Comment 15

[jeremy.kim.leo [:jeremykimleo]]

thanks mhabicher.

it looks like the patch only based on master branch.

we are focusing on v2.0 branch now. 
because "Dom/Camera" has huge difference between master and v2.0, it hard to apply on v2.0.
Can you make the patch based on v2.0 also?

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8529964 [details] [diff] [review]
Fix memory leak in CameraControl, v1

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

LGTM aside from that below. My head hurts though :).

::: dom/camera/DOMCameraControl.cpp
@@ +820,5 @@
>                                     const Optional<OwningNonNull<CameraStartRecordingCallback> >& aOnSuccess,
>                                     const Optional<OwningNonNull<CameraErrorCallback> >& aOnError,
>                                     ErrorResult& aRv)
>  {
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);

Doesn't OnCreatedFileDescriptor need to check mCameraControl too? And free the descriptor if it doesn't exist?

Comment 17

[jeremy.kim.leo [:jeremykimleo]]

to test your patch(attachment 8529964 [details] [diff] [review]), 
i used latest code(master) on today, but there are some compile error. 
(gecko revision : 59052ffb3b6ef42a1e23f3c0a489e977956263f6)

Comment 18

[jeremy.kim.leo [:jeremykimleo]]

i thinks there are 3 problems that make memory leak 

#1. DOMCameraContorls never removes listener. (STR : Comment #0)
#2. CameraControls always created (STR : Comment #13)
#3. Just doing change visibility (STR : Comment #13), CameraThread is also created every times.
(http://dxr.mozilla.org/mozilla-central/source/dom/camera/CameraControlImpl.cpp#34)

$ adb shell b2g-ps -t | grep "Camera"
APPLICATION    SEC USER      PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
Camera           2 u0_a1968  1968  402   97724  31588 ffffffff b6ee0894 S /system/b2g/b2g
CameraThread     2 u0_a1968  2308  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
Camera           2 u0_a1968  2367  1968  97724  31588 c01c378c b6ee0a60 S Camera
CameraThread     2 u0_a1968  5963  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
CameraThread     2 u0_a1968  6260  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
CameraThread     2 u0_a1968  6321  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
CameraThread     2 u0_a1968  6385  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
CameraThread     2 u0_a1968  6447  1968  97724  31588 c01c378c b6ee0a60 S CameraThread
CameraThread     2 u0_a1968  6509  1968  97724  31588 c01c378c b6ee0a60 S CameraThread

when i reviews your patch(attachment 8529964 [details] [diff] [review]), i think it is only considers #1 and #2.
#3 is also has a chance making troubles or memory leaks.

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to jeremy.kim.leo [:jeremykimleo] from comment #18)
> i thinks there are 3 problems that make memory leak 
> 
> #1. DOMCameraContorls never removes listener. (STR : Comment #0)
> #2. CameraControls always created (STR : Comment #13)
> #3. Just doing change visibility (STR : Comment #13), CameraThread is also
> created every times.
  ...
> #3 is also has a chance making troubles or memory leaks.

Thank you for identifying that issue. I've confirmed that old CameraThreads seem to live indefinitely (mRefCnt=4). Investigating.

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v2

It turns out that nsThreads are not weak-referenceable, so I've replaced it with a strong reference that persists until the end of the process. I've also verified that this doesn't leak threads.

jeremy.kim.leo, please test this and let me know.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=10b46f7bdfbd

Comment 21

[Youngsu Rho]

I applied patch v1 and v2 in master branch.
But there are still some problems appear.

Case 1. Not destroyed GlobalWindow object.
  - Reproduce : Camera Launch -> Home -> SMS Launch -> New Mesage ->
                Open Camera Activity(Attach) -> activity close (3 times)
  - Problem : GlobalWindow and CameraControler still is not destroyed.
================================================================================================
11-29 13:37:51.640  4179  4179 I CAMERA__: nsGlobalWindow : |b3af8e20|      <-- Camera Launch
11-29 13:37:51.960  4179  4179 I CAMERA__: nsGlobalWindow : |b3af9560|
11-29 13:37:52.000  4179  4179 I CAMERA__: nsGlobalWindow : |b3af9ad0|
11-29 13:37:57.570  4179  4179 I CAMERA__: nsGlobalWindow : |b3af9ca0|
11-29 13:37:58.190  4179  4179 I CAMERA__: nsDOMCameraManager : |b2301b50|
11-29 13:37:58.200  4179  4179 I CAMERA__: nsDOMCameraControl : |b6a944c0|
11-29 13:38:05.010  4179  4179 I CAMERA__: ~nsGlobalWindow : |b3af9560|
11-29 13:38:05.010  4179  4179 I CAMERA__: ~nsGlobalWindow : |b3af9ad0|
11-29 13:38:20.780  4179  4179 I CAMERA__: nsGlobalWindow : |b3af9560|      <-- Camera Activity Open
11-29 13:38:21.020  4179  4179 I CAMERA__: nsGlobalWindow : |b3af9ad0|
11-29 13:38:21.060  4179  4179 I CAMERA__: nsGlobalWindow : |b3afc9f0|
11-29 13:38:21.640  4179  4179 I CAMERA__: nsDOMCameraManager : |afd74f70|
11-29 13:38:21.650  4179  4179 I CAMERA__: nsDOMCameraControl : |b0680780|
11-29 13:38:27.730  4179  4179 I CAMERA__: ~nsGlobalWindow : |b3af9ad0|
11-29 13:38:30.680  4179  4179 I CAMERA__: ~nsDOMCameraManager : |afd74f70| <-- Activity Close
11-29 13:39:01.350  4179  4179 I CAMERA__: nsGlobalWindow : |afdc8170|      <-- Camera Activity Open
11-29 13:39:01.470  4179  4179 I CAMERA__: nsGlobalWindow : |afdc8340|
11-29 13:39:01.520  4179  4179 I CAMERA__: nsGlobalWindow : |afdc8510|
11-29 13:39:02.180  4179  4179 I CAMERA__: nsDOMCameraManager : |aefe5100|
11-29 13:39:02.190  4179  4179 I CAMERA__: nsDOMCameraControl : |b0681080|
11-29 13:39:08.560  4179  4179 I CAMERA__: ~nsGlobalWindow : |afdc8340|
11-29 13:39:15.300  4179  4179 I CAMERA__: ~nsDOMCameraManager : |aefe5100| <-- Activity Close
11-29 13:39:34.300  4179  4179 I CAMERA__: nsGlobalWindow : |afdc8340|      <-- Camera Activity Open
11-29 13:39:34.430  4179  4179 I CAMERA__: nsGlobalWindow : |afdc86e0|
11-29 13:39:34.460  4179  4179 I CAMERA__: nsGlobalWindow : |afdc88b0|
11-29 13:39:35.050  4179  4179 I CAMERA__: nsDOMCameraManager : |b342cb50|
11-29 13:39:35.060  4179  4179 I CAMERA__: nsDOMCameraControl : |b06808a0|
11-29 13:39:42.200  4179  4179 I CAMERA__: ~nsGlobalWindow : |afdc86e0|
11-29 13:39:48.630  4179  4179 I CAMERA__: ~nsDOMCameraManager : |b342cb50| <-- Activity Close
================================================================================================

Case 2. Not destroyed CameraControlers
  - Reproduce : Camera Launch -> Visible change : [Hide(Home) -> Show(Camera)] (5 Times)
  - Problem : Your patch has a effect to remove the DOMCameraControl object.
              In this test, Five CameraControlers were created. But only 3 CameraControlers are destroyed.
              The remained one is referenced by camera app. but another one has to be destroyed.
================================================================================================
11-29 13:36:14.830  2929  2929 I CAMERA__: nsGlobalWindow : |b3af8e20|      <-- Camera Launch
11-29 13:36:15.150  2929  2929 I CAMERA__: nsGlobalWindow : |b3af9560|
11-29 13:36:15.200  2929  2929 I CAMERA__: nsGlobalWindow : |b3af9ad0|
11-29 13:36:22.480  2929  2929 I CAMERA__: nsGlobalWindow : |b3af9ca0|
11-29 13:36:23.160  2929  2929 I CAMERA__: nsDOMCameraManager : |b240cb80|
11-29 13:36:23.170  2929  2929 I CAMERA__: nsDOMCameraControl : |b6a944c0|
11-29 13:36:27.370  2929  2929 I CAMERA__: ~nsGlobalWindow : |b3af9560|
11-29 13:36:27.370  2929  2929 I CAMERA__: ~nsGlobalWindow : |b3af9ad0|
11-29 13:36:52.280  2929  2929 I CAMERA__: nsDOMCameraControl : |b039ee80|  <-- Visible : Hide -> Show
11-29 13:36:57.500  2929  2929 I CAMERA__: nsDOMCameraControl : |b039f0c0|  <-- Visible : Hide -> Show
11-29 13:37:00.210  2929  2929 I CAMERA__: ~nsDOMCameraControl : |b6a944c0| <-- Visible : Hide
11-29 13:37:02.470  2929  2929 I CAMERA__: nsDOMCameraControl : |b039f1e0|  <-- Visible : Show
11-29 13:37:05.640  2929  2929 I CAMERA__: ~nsDOMCameraControl : |b039ee80| <-- Visible : Hide
11-29 13:37:08.130  2929  2929 I CAMERA__: nsDOMCameraControl : |b039ee80|  <-- Visible : Show
11-29 13:37:11.870  2929  2929 I CAMERA__: ~nsDOMCameraControl : |b039f0c0| <-- Visible : Hide
================================================================================================


Case 3. CameraThread infinite creation.
  - Reproduce : Camera Launch -> Visible change : [Hide(Home) -> Show(Camera)] (5 Times)
  - Problem : I applied patch v2. But CameraThread is still created.
================================================================================================
u0_a10088 10088 507   131572 32812 ffffffff b6f838dc S /system/b2g/b2g
u0_a10088 10089 10088 131572 32812 c028a0b8 b6f838dc S Chrome_ChildThr
u0_a10088 10090 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10091 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10092 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10093 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10094 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10095 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10096 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10097 10088 131572 32812 c01d5098 b6f83aa8 S Analysis Helper
u0_a10088 10098 10088 131572 32812 c025f4c8 b6f83964 S Socket Thread
u0_a10088 10099 10088 131572 32812 c01d5098 b6f83aa8 S BgHangManager
u0_a10088 10100 10088 131572 32812 c025f4c8 b6f83964 S MemoryPressure
u0_a10088 10101 10088 131572 32812 c01d5098 b6f83aa8 S Timer
u0_a10088 10102 10088 131572 32812 c01d5098 b6f83aa8 S ImageBridgeChil
u0_a10088 10103 10088 131572 32812 c01d5098 b6f83aa8 S BufferMgrChild
u0_a10088 10104 10088 131572 32812 c0666edc b6f8272c S Binder_1
u0_a10088 10105 10088 131572 32812 c0666edc b6f8272c S Binder_2
u0_a10088 10517 10088 131572 32812 c01d5098 b6f83aa8 S CameraThread
u0_a10088 10540 10088 131572 32812 c01d5098 b6f83aa8 S HTML5 Parser
u0_a10088 10599 10088 131572 32812 c0666edc b6f8272c S Binder_3
u0_a10088 10615 10088 131572 32812 c01d5098 b6f83aa8 S CameraThread
u0_a10088 10720 10088 131572 32812 c01d5098 b6f83aa8 S CameraThread
u0_a10088 10822 10088 131572 32812 c01d5098 b6f83aa8 S CameraThread
u0_a10088 10921 10088 131572 32812 c01d5098 b6f83aa8 S CameraThread
================================================================================================

Comment 22

[Youngsu Rho]

Attachment: Case 3 - Fix memory leak about CameraThread, v1

I made a patch for the CameraThread(Case 3).
How about this patch?

Comment 23

[jeremy.kim.leo [:jeremykimleo]]

Comment on attachment 8530456 [details] [diff] [review]
Fix memory leak in CameraControl, v2

please check comment #21, comment #22 remained by my co-worker.
there are no effect between before and after with patches.

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Youngsu Rho from comment #21)
> I applied patch v1 and v2 in master branch.
> But there are still some problems appear.
  ...
> Case 2. Not destroyed CameraControlers
>   - Reproduce : Camera Launch -> Visible change : [Hide(Home) ->
> Show(Camera)] (5 Times)
>   - Problem : Your patch has a effect to remove the DOMCameraControl object.
>               In this test, Five CameraControlers were created. But only 3
> CameraControlers are destroyed.
>               The remained one is referenced by camera app. but another one
> has to be destroyed.

This is normal. The cycle-collector is non-deterministic, so it's likely that a few DOMCameraControl objects may exist at the end of your test. I've observed that all but the last instance eventually get destroyed, and the count of outstanding instances doesn't exceed 3.

> Case 3. CameraThread infinite creation.
>   - Reproduce : Camera Launch -> Visible change : [Hide(Home) ->
> Show(Camera)] (5 Times)
>   - Problem : I applied patch v2. But CameraThread is still created.

This is actually the test case I've been using to find the leaks, and following these steps, I only ever get a single "CameraThread".

I will investigate case 1.

Comment 25

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3

youngsu.rho, my apologies--the v2 patch was missing some additional changes. Please try this patch, which includes all fixes to address cases 1 through 3.

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8334f6b01a43

Comment 27

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.1

Fix broken builds:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2988a0d288f7

Comment 28

[Youngsu Rho]

Thank you Mike.
Works well your patch v3.1.
All cases was resolved.

Can you make the patch for gecko v2.0 branch?

Comment 29

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.2

Fix broken tests.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=edfd8f376477

Comment 30

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8531102 [details] [diff] [review]
Fix memory leak in CameraControl, v3.2

We should nudge the app guys towards dropping callbacks, so we can too. The addition of promises has broken callbacks for methods that can also throw exceptions.

Comment 31

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Youngsu Rho from comment #28)

> Can you make the patch for gecko v2.0 branch?

Yes, but it will take a few days.

Comment 32

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 33

[Mike Habicher [:mikeh] (high bugzilla latency)]

Wilson, would you mind opening a new bug for your patch and blocking this one on it? I have two versions of patch to prepare for this bug, and I think everything will land cleaner if the steps are separate.

Comment 34

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8531743 [details] [review]
pull-request (master)

Moved this patch to its own bug 1107284.

Comment 35

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.3

Comment 36

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.3 [v2.0/b2g32]

youngsu.rho, please try this version of the patch on v2.0. v2.2/master is quite different, so it's possible the port has introduced new issues.

Comment 37

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.3.1 [v2.0/b2g32]

Updated patch for v2.0, fixes an XPCOM error that was preventing the camera from starting.

Comment 38

[jeremy.kim.leo [:jeremykimleo]]

Comment on attachment 8532113 [details] [diff] [review]
Fix memory leak in CameraControl, v3.3.1 [v2.0/b2g32]

LGTM on b2g32 (flame, v2.0) 

Thanks Mike.

Comment 39

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8532113 [details] [diff] [review]
Fix memory leak in CameraControl, v3.3.1 [v2.0/b2g32]

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

r+ with nits addressed.

::: dom/camera/DOMCameraControl.cpp
@@ +725,5 @@
>    }
>  
> +  if (!mCameraControl) {
> +    rv = NS_ERROR_NOT_AVAILABLE;
> +  }

What happens if mCameraControl == null at line 721? I think this check is in the wrong place.

Comment 40

[jeremy.kim.leo [:jeremykimleo]]

Dear. Mike  

i found strange code.
look at this 

int32_t
 nsDOMCameraControl::SensorAngle()
 {
-  MOZ_ASSERT(mCameraControl);
-
   int32_t angle = 0;
-  mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, angle);
+  if (!mCameraControl) {
+    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, angle);
+  }
   return angle;
 }

i think it should be fixed like this 
+  if (mCameraControl) {
+    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, angle);
+  }

thanks

Comment 41

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.4

Rebased on top of attachment 8534184 [details] [diff] [review].

Comment 42

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.4.1

Minor clean-up.

Comment 43

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.4 [v2.0/b2g32] r=aosmond

Fix logic error in DCC::SensorAngle() (thanks, :jeremykimleo!). Carry r+ forward.

Comment 44

[Mike Habicher [:mikeh] (high bugzilla latency)]

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7c208853d56

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8534192 [details] [diff] [review]
Fix memory leak in CameraControl, v3.4.1

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

LGTM.

Comment 46

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.4.1 [v2.0/b2g32] r=aosmond

Remove stale class declaration.

Comment 47

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8534693 [details] [diff] [review]
Fix memory leak in CameraControl, v3.4.1 [v2.0/b2g32] r=aosmond

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): memory leak
User impact if declined: with certain steps, the Camera app memory use can grow without bounds, leading to performance issues, and the app eventually getting LMKed
Testing completed: this bug's STR on Flame; analysis with b2g-ps and get_gc_cc_log.py; normal app usage
Risk to taking this patch (and alternatives if risky): there may be regressions to some corner use-cases, but general app operation is correct.
String or UUID changes made by this patch: none

Comment 48

[Mike Habicher [:mikeh] (high bugzilla latency)]

Piggybacking a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c0b245b244d8

Comment 49

[Mike Habicher [:mikeh] (high bugzilla latency)]

[Blocking Requested - why for this release]: memory leak, progressive degradation of device, Camera app eventually LMKed.

Comment 50

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.5 r=aosmond

This should fix an assertion-count failure in the DEBUG automated tests. (We'll know as soon as the try tree reopens.)

Carrying r+ forward.

Comment 51

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.5 [v2.0/b2g32] r=aosmond

[Approval Request Comment] see comment 47.

v3.5 patch rebased for v2.0/b2g32.

Comment 52

[Mike Habicher [:mikeh] (high bugzilla latency)]

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b1f2b4f3f74

Comment 53

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/integration/b2g-inbound/rev/220f9cd8097a

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/220f9cd8097a

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c327ed6873ff

This needs rebasing for b2g34. Also, take note of a follow-up push Ehsan did on trunk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0a42855a50

AFAICT, it wasn't needed for the b2g32 patch, but do include it with the b2g34 patch if applicable there.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Backed out from b2g32 for B2G mochitest crashes.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/12aea1649f5a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=70793&repo=mozilla-b2g32_v2_0

Comment 57

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.6 [v2.0/b2g32] r=aosmond

Fix test failures.

[Approval Request Comment]
See previous a+.

b2g32_v2_1 version coming next.

Comment 58

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7 [v2.1/b2g34] r=aosmond

Fix test bustage; carry r+ forward.

[Approval Request Comment]
See previous a+.

b2g32_v2_0 version coming next (for real).

Comment 59

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7 [v2.0/b2g32] r=aosmond

Fix test bustage; carry r+ forward.

[Approval Request Comment]
See previous a+.

Comment 60

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8537023 [details] [diff] [review]
Fix memory leak in CameraControl, v3.7 [v2.1/b2g34] r=aosmond

Actually, I just noticed that at some point in the backport, there was a bad merge, and both this and the v2.1 patch are missing some critical code. Dropping for now.

Comment 61

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8537029 [details] [diff] [review]
Fix memory leak in CameraControl, v3.7 [v2.0/b2g32] r=aosmond

Ditto.

Comment 62

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7.1 [v2.0/b2g32] r=aosmond

Remove declaration of symbols not required in backport; carrying r+ forward.

[Approval Request Comment]
See previous a+.

Comment 63

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7.1 [v2.1/b2g34] r=aosmond

Remove declaration of symbols not required in backport; carrying r+ forward.

[Approval Request Comment]
See previous a+.

Comment 64

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7.2 [v2.0/b2g32] r=aosmond

Fix editing mistake; carrying r+ forward. Seems I only notice things in the bugzilla diff viewer. :-/

[Approval Request Comment]
See previous a+.

Comment 65

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.7.3 [v2.0/b2g32] r=aosmond

Fix typo; carry r+ forward; GO TO SLEEP... and never again work on parallel backports after midnight again.

[Approval Request Comment]
See previous a+.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8537043 [details] [diff] [review]
Fix memory leak in CameraControl, v3.7.1 [v2.1/b2g34] r=aosmond

No need to re-request approval unless you've made changes to the patch that substantially change your answers to the approval questionnaire. Given that you're just referring back to those original answers, it doesn't sound like that's the case :)

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2548deff272b
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/01e5265fde4a

Comment 68

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/c327ed6873ff

Comment 69

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/01e5265fde4a

Comment 70

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix test crashes in v2.0/v2.1

Applies on top of attachment 8537051 [details] [diff] [review]. b2g34 version coming next.

Comment 71

[Mike Habicher [:mikeh] (high bugzilla latency)]

v2.0: https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/777195b0a209
v2.1: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/7b161e9fe25e

Comment 72

[Ryan VanderMeulen [:RyanVM]]

Hitting asserts now :(
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=55105&repo=mozilla-b2g34_v2_1
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=72343&repo=mozilla-b2g32_v2_0

Backed out.

Comment 73

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.8 [v2.0/b2g32]

Andrew, would you mind reviewing this backport? It's a pretty significant change. The upside is, it works!

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=df7e5df53e85

Comment 74

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8538167 [details] [diff] [review]
Fix memory leak in CameraControl, v3.8 [v2.0/b2g32]

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

LGTM.

Comment 75

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix memory leak in CameraControl, v3.8 [v2.1/b2g34] r=aosmond

Carrying r+ from related patches.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=df9ca6edb1dc

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d41f6bd64343
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f57067657bfa

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/f57067657bfa

Comment 78

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame v2.0&2.1&2.2 and Woodduck 2.0.
See attachment: verified_v2.2.MP4 and memoryUsage_After30times_v2.1.txt.
Reproduce rate: 0/35.

STR:
1. Launch Camera app and then tap Home button to go back to homescreen.
2. Launch Messages app.
3. New message => attachment => camera.
4. Take a picture => select.
5. Back from New messages => discard.
6. Repeat step 3&4&5.
7. Repeat step 3&4&5...

Actual results:
Memory usage of camera app is stable.
After repeated 30 times, the memory usage of Camera is "RSS: 23.8", "PSS: 15.1".
Please see attachment "memoryUsage_After30times_v2.1.txt" about the memory usage.

Woodduck 2.0 build:
Gaia-Rev        afa87cffbd3cd9e2070b26d45dd556a9324bd4d5
Gecko-Rev       911e6cd6aecf8d37d42c203e162847b78a68a8d8
Build-ID        20141224050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1419368730
FW-Date         Wed Dec 24 05:05:52 CST 2014

Flame 2.0 build:
Gaia-Rev        ce83ea7b8e3fa2d1c3fd771fc22b654c18b3c381
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4dd6d9b58f42
Build-ID        20141223000202
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141223.034247
FW-Date         Tue Dec 23 03:42:58 EST 2014
Bootloader      L1TC00011880

Flame 2.1 build:
Gaia-Rev        17c7ad2e4919a994f0844239b483116090412dee
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/39dfb662c82a
Build-ID        20141223001203
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141223.035107
FW-Date         Tue Dec 23 03:51:18 EST 2014
Bootloader      L1TC00011880

Flame 2.2 build:
Gaia-Rev        c2da2bafd4e809317e2ca70c9bf5c11136a32818
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0532f2509f3f
Build-ID        20141223010202
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141223.043429
FW-Date         Tue Dec 23 04:34:39 EST 2014
Bootloader      L1TC00011880

Comment 79

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.MP4

Comment 80

[Shally Li（leave from Mozilla）]

Attachment: memoryUsage_After30times_v2.1.txt

Comment 81

[Paladin]

Attachment: Verify_video.3gp

This issue is verified not happen on latest build of Woodduck 2.0,Flame 2.0/2.1/2.2
See attachment: Verify_video.3gp
Reproducing rate: 0/2

When repeated 30 times, the memory usage is Rss: 29.7K, Pss: 19.9K.(Woodduck 2.0)
                                            Rss: 34.6K, Pss: 20.3K.(Flame 2.0)
                                            Rss: 21.2K, Pss: 13.9K.(Flame 2.1)
                                            Rss: 28.7K, Pss: 20.8K.(Flame 2.2)
When repeated first time,the memory usage is Rss: 30.5K, Pss: 18.2K.(Woodduck 2.0)
                                             Rss: 33.3K, Pss: 19.0K.(Flame 2.0)
                                             Rss: 23.0K, Pss: 15.4K.(Flame 2.1)
                                             Rss: 26.0K, Pss: 16.5K.(Flame 2.2)

Flame 2.0 Build:
Gaia-Rev        ce83ea7b8e3fa2d1c3fd771fc22b654c18b3c381
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4dd6d9b58f42
Build-ID        20141223000202
Version         32.0
----------------------
Flame 2.1 Build:
Gaia-Rev        17c7ad2e4919a994f0844239b483116090412dee
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/39dfb662c82a
Build-ID        20141223001203
Version         34.0
----------------------
Flame 2.2 Build:
Gaia-Rev        c2da2bafd4e809317e2ca70c9bf5c11136a32818
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0532f2509f3f
Build-ID        20141223010202
Version         37.0a1
----------------------
Woodduck 2.0 Build:
Gaia-Rev        afa87cffbd3cd9e2070b26d45dd556a9324bd4d5
Gecko-Rev       911e6cd6aecf8d37d42c203e162847b78a68a8d8
Build-ID        20141224050313
Version         32.0