1002527 - Unbalanced refcount of remote browser's ContentParent

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

ContentParent::GetNewOrUsed() calls Init() always, even when PreallocatedProcessManager::Take() returns a non-null instance which has been initialized. This creates problems:

- The ContentParent instance is registered for the same event twice from Init(),
  which will handle the same event twice.
- Even after ContentParent::ActoryDestroy() is called, there still remain one
  observer registration for each event.
  * The ContentParent will still receive the events in future.
  * The ContentParent will always be referenced by observer list.
- The observer list will keep growing.

I have tried to enlarge /sys/module/lowmemorykiller/parameters/notify_trigger to monitor memory-pressure event on ContentParent, and confirmed the symptoms above.

Comment 1

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

The ContentParent instance PreallocatedProcessManager::Take() returns could be from:

  - ContentParent::PreallocateAppProcess()
  - ContentParent::RunNuwaProcess()
  - ContentParent::RecvAddNewProcess()

which all have been initialized already.

Comment 2

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

Attachment: WIP - Fix not to initialize ContentParent twice

Comment 3

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

(In reply to Ting-Yu Chou [:ting] from comment #2)
> Created attachment 8414285 [details] [diff] [review]
> WIP -  Fix not to initialize ContentParent twice

Browser shows "Server not found" page when this patch is included, am checking now.

Comment 4

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

(In reply to Ting-Yu Chou [:ting] from comment #3)
> Browser shows "Server not found" page when this patch is included, am
> checking now.

Never mind, false alarm.

Comment 5

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

Discussed this with Cervantes, it'd be better to make sure there's no other unbalanced reference count for the remote browser's ContentParent. I will check it.

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Considering that LMK happens often, I think we should consider this for 1.3t

Fabrice, what do you think?  Would it be too risky now?

Comment 7

[[:fabrice] Fabrice Desré]

We'll likely take that, yes.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8414285 [details] [diff] [review]
WIP -  Fix not to initialize ContentParent twice

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

Thanks!

Comment 9

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

Attachment: ~ContentParent() backtrace

With the patch, ContentParent will be destroyed minutes after DelayedDeleteContentParentTask::Run(), note the reference count is bigger than 1 in the function.

Tried to capture a backtrace of ~ContentParent(), it is destroyed by cycle collector.

Comment 10

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

(In reply to Ting-Yu Chou [:ting] from comment #9)
> DelayedDeleteContentParentTask::Run(), note the reference count is bigger
> than 1 in the function.

Most of time it is 2, should be the mContentParent from nsFrameLoader. Sometimes it is 3, not sure yet where's the extra reference. But in both cases, CC will destroy it.

Comment 11

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

(In reply to Ting-Yu Chou [:ting] from comment #9)
> With the patch, ContentParent will be destroyed minutes after DelayedDeleteContentParentTask::Run()

From what I experienced, it can be 1min ~ 10mins after DelayedDeleteContentParentTask::Run().

Comment 12

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

Attachment: Fix not to initialize remote browser's ContentParent twice. r=bent

Commit message updated.

Comment 13

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

Try results: https://tbpl.mozilla.org/?tree=Try&rev=afa7ce3c5469

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ to avoid mem leaks

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

to Ting as Ting provide a patch. thanks

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6ce6648675

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b6ce6648675

Comment 18

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/71790bf476cb