Closed Bug 1104055 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

(blocking-b2g:2.0+, firefox35 wontfix, firefox36 wontfix, firefox37 fixed, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

RESOLVED FIXED
2.2 S2 (19dec)
blocking-b2g 2.0+
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: youngwoo.jo, Assigned: mikeh)

References

Details

(Whiteboard: [2.2 target] [ft:media])

Attachments

(6 files, 28 obsolete files)

71.80 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
61.73 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
58.80 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
2.30 MB, video/mp4
Details
3.17 KB, text/plain
Details
4.10 MB, video/3gpp
Details
Attached file flame_b2g_procrank.txt (obsolete) —
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.
This is reproduced in flame master.
And I confirmed nsGlobalWindow for camera pick activity is not released even if the activity finished.
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.
Flags: needinfo?(khuey)
Attached file cc-edges.1230.1416849964.log.gz (obsolete) —
This is a cc log extracted from flame v2.0 camera app process.
Attached file gc-edges.1230.1416849964.log.gz (obsolete) —
This is a gc log extracted from flame v2.0 camera app process.
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?
Flags: needinfo?(khuey) → needinfo?(mhabicher)
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.
Attached patch Remove listener on CameraControl (obsolete) — Splinter Review
this is WP1 patch to remove listener on nsDOMCameraControl.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #8528387 - Flags: review-
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.
Flags: needinfo?(mhabicher) → needinfo?(jeremy.kim.leo)
Assignee: nobody → jeremy.kim.leo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Camera pick activity makes a memory leakage → [Camera][Gecko] Camera pick activity makes a memory leakage
(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)
Flags: needinfo?(jeremy.kim.leo) → needinfo?(mhabicher)
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
Attachment #8528387 - Attachment is obsolete: true
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.
Flags: needinfo?(mhabicher)
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.
Assignee: jeremy.kim.leo → mhabicher
Attachment #8527725 - Attachment is obsolete: true
Attachment #8527816 - Attachment is obsolete: true
Attachment #8527817 - Attachment is obsolete: true
Attachment #8529119 - Attachment is obsolete: true
Attachment #8529964 - Flags: review?(khuey)
Attachment #8529964 - Flags: review?(aosmond)
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?
Flags: needinfo?(mhabicher)
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?
Attachment #8529964 - Flags: review?(aosmond) → review+
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)
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.
(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.
Flags: needinfo?(mhabicher)
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
Attachment #8529964 - Attachment is obsolete: true
Attachment #8529964 - Flags: review?(khuey)
Attachment #8530456 - Flags: feedback?(jeremy.kim.leo)
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
================================================================================================
I made a patch for the CameraThread(Case 3).
How about this patch?
Flags: needinfo?(mhabicher)
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.
Attachment #8530456 - Flags: feedback?(jeremy.kim.leo) → feedback-
(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.
Flags: needinfo?(mhabicher)
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.
Attachment #8530456 - Attachment is obsolete: true
Attachment #8530488 - Attachment is obsolete: true
Attachment #8530976 - Flags: feedback?(youngsu.rho)
Fix broken builds:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2988a0d288f7
Attachment #8530976 - Attachment is obsolete: true
Attachment #8530976 - Flags: feedback?(youngsu.rho)
Attachment #8530988 - Flags: feedback?(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?
Flags: needinfo?(mhabicher)
Attachment #8530988 - Flags: feedback?(youngsu.rho) → feedback+
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.
Flags: needinfo?(mhabicher)
Attachment #8531102 - Flags: review?(aosmond)
(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.
Summary: [Camera][Gecko] Camera pick activity makes a memory leakage → [Camera][Gecko] Camera pick activity causes a memory leak
Attached file pull-request (master) (obsolete) —
Attachment #8531743 - Flags: review?(mhabicher)
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.
Flags: needinfo?(wilsonpage)
Comment on attachment 8531743 [details] [review]
pull-request (master)

Moved this patch to its own bug 1107284.
Attachment #8531743 - Attachment is obsolete: true
Attachment #8531743 - Flags: review?(mhabicher)
Flags: needinfo?(wilsonpage)
Attachment #8531102 - Attachment is obsolete: true
Attachment #8531102 - Flags: review?(aosmond)
Attachment #8531791 - Flags: review?(aosmond)
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.
Attachment #8531792 - Flags: feedback?(youngsu.rho)
Updated patch for v2.0, fixes an XPCOM error that was preventing the camera from starting.
Attachment #8531792 - Attachment is obsolete: true
Attachment #8531792 - Flags: feedback?(youngsu.rho)
Attachment #8532113 - Flags: feedback?(youngsu.rho)
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.
Attachment #8532113 - Flags: feedback?(youngsu.rho) → feedback+
Attachment #8532113 - Flags: review?(aosmond)
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.
Attachment #8532113 - Flags: review?(aosmond) → review+
Whiteboard: [2.2 target] [ft:media]
Target Milestone: --- → 2.2 S2 (19dec)
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
Rebased on top of attachment 8534184 [details] [diff] [review].
Attachment #8531791 - Attachment is obsolete: true
Attachment #8531791 - Flags: review?(aosmond)
Attachment #8534191 - Flags: review?(aosmond)
Minor clean-up.
Attachment #8534191 - Attachment is obsolete: true
Attachment #8534191 - Flags: review?(aosmond)
Attachment #8534192 - Flags: review?(aosmond)
Fix logic error in DCC::SensorAngle() (thanks, :jeremykimleo!). Carry r+ forward.
Attachment #8532113 - Attachment is obsolete: true
Attachment #8534196 - Flags: review+
Comment on attachment 8534192 [details] [diff] [review]
Fix memory leak in CameraControl, v3.4.1

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

LGTM.
Attachment #8534192 - Flags: review?(aosmond) → review+
QA Whiteboard: [2.2-features-qa+]
Remove stale class declaration.
Attachment #8534196 - Attachment is obsolete: true
Attachment #8534693 - Flags: review+
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
Attachment #8534693 - Flags: approval-mozilla-b2g34?
Attachment #8534693 - Flags: approval-mozilla-b2g32?
[Blocking Requested - why for this release]: memory leak, progressive degradation of device, Camera app eventually LMKed.
blocking-b2g: --- → 2.0?
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.
Attachment #8534192 - Attachment is obsolete: true
Attachment #8535238 - Flags: review+
[Approval Request Comment] see comment 47.

v3.5 patch rebased for v2.0/b2g32.
Attachment #8534693 - Attachment is obsolete: true
Attachment #8534693 - Flags: approval-mozilla-b2g34?
Attachment #8534693 - Flags: approval-mozilla-b2g32?
Attachment #8535249 - Flags: review+
Attachment #8535249 - Flags: approval-mozilla-b2g34?
Attachment #8535249 - Flags: approval-mozilla-b2g32?
https://hg.mozilla.org/mozilla-central/rev/220f9cd8097a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
blocking-b2g: 2.0? → 2.0+
Attachment #8535249 - Flags: approval-mozilla-b2g34?
Attachment #8535249 - Flags: approval-mozilla-b2g34+
Attachment #8535249 - Flags: approval-mozilla-b2g32?
Attachment #8535249 - Flags: approval-mozilla-b2g32+
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.
Fix test failures.

[Approval Request Comment]
See previous a+.

b2g32_v2_1 version coming next.
Attachment #8535249 - Attachment is obsolete: true
Flags: needinfo?(mhabicher)
Attachment #8536971 - Flags: review+
Attachment #8536971 - Flags: approval-mozilla-b2g32?
Attachment #8536971 - Attachment is obsolete: true
Attachment #8536971 - Flags: approval-mozilla-b2g32?
Fix test bustage; carry r+ forward.

[Approval Request Comment]
See previous a+.

b2g32_v2_0 version coming next (for real).
Attachment #8537023 - Flags: review+
Attachment #8537023 - Flags: approval-mozilla-b2g34?
Fix test bustage; carry r+ forward.

[Approval Request Comment]
See previous a+.
Attachment #8537029 - Flags: review+
Attachment #8537029 - Flags: approval-mozilla-b2g32?
See Also: → 1111984
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.
Attachment #8537023 - Attachment is obsolete: true
Attachment #8537023 - Flags: review+
Attachment #8537023 - Flags: approval-mozilla-b2g34?
Comment on attachment 8537029 [details] [diff] [review]
Fix memory leak in CameraControl, v3.7 [v2.0/b2g32] r=aosmond

Ditto.
Attachment #8537029 - Attachment is obsolete: true
Attachment #8537029 - Flags: review+
Attachment #8537029 - Flags: approval-mozilla-b2g32?
Remove declaration of symbols not required in backport; carrying r+ forward.

[Approval Request Comment]
See previous a+.
Attachment #8537042 - Flags: review+
Attachment #8537042 - Flags: approval-mozilla-b2g32?
Remove declaration of symbols not required in backport; carrying r+ forward.

[Approval Request Comment]
See previous a+.
Attachment #8537043 - Flags: review+
Attachment #8537043 - Flags: approval-mozilla-b2g34?
Fix editing mistake; carrying r+ forward. Seems I only notice things in the bugzilla diff viewer. :-/

[Approval Request Comment]
See previous a+.
Attachment #8537042 - Attachment is obsolete: true
Attachment #8537042 - Flags: approval-mozilla-b2g32?
Attachment #8537047 - Flags: review+
Attachment #8537047 - Flags: approval-mozilla-b2g32?
Fix typo; carry r+ forward; GO TO SLEEP... and never again work on parallel backports after midnight again.

[Approval Request Comment]
See previous a+.
Attachment #8537047 - Attachment is obsolete: true
Attachment #8537047 - Flags: approval-mozilla-b2g32?
Attachment #8537051 - Flags: review+
Attachment #8537051 - Flags: approval-mozilla-b2g32?
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 :)
Attachment #8537043 - Flags: approval-mozilla-b2g34?
Attachment #8537051 - Flags: approval-mozilla-b2g32?
Attached patch Fix test crashes in v2.0/v2.1 (obsolete) — Splinter Review
Applies on top of attachment 8537051 [details] [diff] [review]. b2g34 version coming next.
Attachment #8537379 - Attachment description: Fix test crashes in v2.0 → Fix test crashes in v2.0/v2.1
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
Attachment #8537043 - Attachment is obsolete: true
Attachment #8537051 - Attachment is obsolete: true
Attachment #8537379 - Attachment is obsolete: true
Flags: needinfo?(mhabicher)
Attachment #8538167 - Flags: review?(aosmond)
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.
Attachment #8538167 - Flags: review?(aosmond) → review+
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
Attached video 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
You need to log in before you can comment on or make changes to this bug.