Unbalanced refcount of remote browser's ContentParent

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

unspecified
mozilla32
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
The ContentParent instance PreallocatedProcessManager::Take() returns could be from:

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

which all have been initialized already.
(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.
(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.
Component: General → IPC
Product: Firefox OS → Core
See Also: → 999351
Attachment #8414285 - Flags: review?(bent.mozilla)
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.
Summary: ContentParent::GetNewOrUsed() initializes ContentParent twice → Unbalanced refcount of remote browser's ContentParent
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?
blocking-b2g: --- → 1.3T?
Flags: needinfo?(fabrice)
We'll likely take that, yes.
Flags: needinfo?(fabrice)
Comment on attachment 8414285 [details] [diff] [review]
WIP -  Fix not to initialize ContentParent twice

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

Thanks!
Attachment #8414285 - Flags: review?(bent.mozilla) → review+
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.
(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.
(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().
Commit message updated.
Attachment #8414285 - Attachment is obsolete: true
Attachment #8415092 - Attachment description: Fix not to initialize remote browser's ContentParent twice → Fix not to initialize remote browser's ContentParent twice. r=bent
triage: 1.3T+ to avoid mem leaks
blocking-b2g: 1.3T? → 1.3T+
to Ting as Ting provide a patch. thanks
Assignee: nobody → tchou
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2b6ce6648675
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.