Closed Bug 1166592 Opened 6 years ago Closed 6 years ago

Leak ContentParent through ParentIdleListener register to idle service

Categories

(Core :: DOM: Content Processes, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ting, Assigned: ting)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 1 obsolete file)

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
I'll take this.
Assignee: nobody → janus926
Status: NEW → ASSIGNED
Component: IPC → DOM: Content Processes
Whiteboard: [MemShrink]
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
(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()...
Attached patch patch v1 (obsolete) — Splinter Review
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)
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 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)
Attachment #8608596 - Flags: review?(bent.mozilla) → review?(khuey)
Whiteboard: [MemShrink] → [MemShrink:P2]
Flags: needinfo?(jocheng)
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)
Attached patch patch v2Splinter Review
Addressed review comment and updated the reviewer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aed448ea8a6
Attachment #8608596 - Attachment is obsolete: true
Attached patch patch for b2g37Splinter Review
Keywords: checkin-needed
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?
Flags: needinfo?(jocheng)
Attachment #8611148 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/mozilla-central/rev/0687b6c670bc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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.