1166592 - Leak ContentParent through ParentIdleListener register to idle service

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Ting-Yu Chou [:ting] (away)]

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

Comment 1

[Ting-Yu Chou [:ting] (away)]

I'll take this.

Comment 2

[Ting-Yu Chou [:ting] (away)]

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

Comment 3

[Ting-Yu Chou [:ting] (away)]

(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()...

Comment 4

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

Also related, bug 968558.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Comment 6

[Reuben Morais [:reuben]]

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.

Comment 7

[[:fabrice] Fabrice Desré]

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?

Comment 8

[Brian Huang [:brianhuang][:brhuang]]

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.

Comment 9

[Josh Cheng [:josh]]

Comment on attachment 8608596 [details] [diff] [review]
patch v1

Hi Bent,
Could you help to review the patch?
Thanks!

Comment 10

[Josh Cheng [:josh]]

Hi Andrew,
Bent is currently on PTO.
Could you help to find someone else who can help to review the patch?
Thanks!

Comment 11

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

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>>.

Comment 12

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

Addressed review comment and updated the reviewer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aed448ea8a6

Comment 13

[Ting-Yu Chou [:ting] (away)]

Attachment: patch for b2g37

Comment 14

[Ting-Yu Chou [:ting] (away)]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0687b6c670bc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3164d17f49fd

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0687b6c670bc