Leak ContentParent through ParentIdleListener register to idle service

RESOLVED FIXED in Firefox 41, Firefox OS v2.2

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

Trunk
mozilla41
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
ContentParent is leaked through the observers registered to idle service [1], only the local hash table is cleared when ActoryDestroy() [2].

[1] https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/81a304f176ea/dom/ipc/ContentParent.cpp#l4260
[2] https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/81a304f176ea/dom/ipc/ContentParent.cpp#l1853
(Assignee)

Updated

4 years ago
status-b2g-v2.2: --- → affected
(Assignee)

Comment 1

4 years ago
I'll take this.
Assignee: nobody → janus926
Status: NEW → ASSIGNED
Component: IPC → DOM: Content Processes
Whiteboard: [MemShrink]
(Assignee)

Comment 2

4 years ago
From nsIIdleService spec [1], the same observer can be added multiple times. So RecvAddIdleObserver() could put different ParentIdleListener to the hashtable |mIdleListeners| with the same key, this makes problem. I'll change |mIdleListeners| to an array and remove the found checking at RecvRemoteIdleObserver().

Besides, idle time is required to RemoveIdleObserver(), so I'll also keep it in ParentIdleListener.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIdleService
(Assignee)

Comment 3

4 years ago
(In reply to Ting-Yu Chou [:ting] from comment #2)
> hashtable |mIdleListeners| with the same key, this makes problem. I'll
> change |mIdleListeners| to an array and remove the found checking at
> RecvRemoteIdleObserver().

A drawback of this is need to iterate |mIdleListeners| to search the ParentIdleListener when RecvRemoveIdleObserver()...

Updated

4 years ago
Blocks: 1121812
(Assignee)

Comment 5

4 years ago
Created attachment 8608596 [details] [diff] [review]
patch v1
Attachment #8608596 - Flags: review?(fabrice)
Attachment #8608596 - Flags: feedback?(reuben.bmo)
Comment on attachment 8608596 [details] [diff] [review]
patch v1

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

LGTM. Would be nice if ParentIdleListener managed adding and removing itself from the idle service so we didn't have to duplicate that code.
Attachment #8608596 - Flags: feedback?(reuben.bmo) → feedback+
Comment on attachment 8608596 [details] [diff] [review]
patch v1

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

I'm not the right reviewer for this code. Maybe khuey or bent?
Attachment #8608596 - Flags: review?(fabrice)
(Assignee)

Updated

4 years ago
Attachment #8608596 - Flags: review?(khuey)
Hi Josh,

Could you help to find someone to review the patch? QA already ran this patch and waiting for it to uplift to 2.2.
Flags: needinfo?(jocheng)

Comment 9

4 years ago
Comment on attachment 8608596 [details] [diff] [review]
patch v1

Hi Bent,
Could you help to review the patch?
Thanks!
Flags: needinfo?(jocheng) → needinfo?(bent.mozilla)
Attachment #8608596 - Flags: review?(khuey)
Attachment #8608596 - Flags: review?(bent.mozilla)

Updated

4 years ago
Attachment #8608596 - Flags: review?(bent.mozilla) → review?(khuey)

Updated

4 years ago
status-b2g-master: --- → affected

Updated

4 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]

Updated

4 years ago
Flags: needinfo?(jocheng)

Comment 10

4 years ago
Hi Andrew,
Bent is currently on PTO.
Could you help to find someone else who can help to review the patch?
Thanks!
Flags: needinfo?(overholt)
Comment on attachment 8608596 [details] [diff] [review]
patch v1

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

::: dom/ipc/ContentParent.h
@@ +927,5 @@
>  
>      nsRefPtr<nsConsoleService>  mConsoleService;
>      nsConsoleService* GetConsoleService();
>  
> +    nsCOMArray<nsIObserver> mIdleListeners;

Don't add new nsCOMArrays.  Use nsTArray<nsCOMPtr<T>>.
Attachment #8608596 - Flags: review?(khuey) → review+
Attachment #8608596 - Flags: review?(bent.mozilla)
Flags: needinfo?(overholt)
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 12

4 years ago
Created attachment 8611073 [details] [diff] [review]
patch v2

Addressed review comment and updated the reviewer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aed448ea8a6
Attachment #8608596 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8611148 [details] [diff] [review]
patch for b2g37
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

3 years ago
Comment on attachment 8611148 [details] [diff] [review]
patch for b2g37

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 #): 938889
User impact if declined: memory leak
Testing completed: linux32 and b2g emulator
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a
Attachment #8611148 - Flags: approval-mozilla-b2g37?

Updated

3 years ago
Flags: needinfo?(jocheng)
Attachment #8611148 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
status-b2g-master: affected → fixed
status-firefox39: --- → wontfix
status-firefox40: --- → wontfix
https://hg.mozilla.org/mozilla-central/rev/0687b6c670bc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Duplicate of this bug: 968558
You need to log in before you can comment on or make changes to this bug.