Closed Bug 895204 Opened 8 years ago Closed 7 years ago

Make sure that ContentParent::ShutDownProcess() successfully closes its channel

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jeffhwang, Assigned: justin.lebar+bug)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212



Actual results:

Channel is in "Error state" but we just report "MsgDropped" to ContentParent.
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/AsyncChannel.cpp#l670

So, ContentParent just ignore this if the error is MsgDropped.
http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#l835



Expected results:

Need to notify channel error to listeners.
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Don't set mIsDestroyed to true until closing channel done
Attachment #778404 - Flags: feedback?(justin.lebar+bug)
This (Comment1) is not a big deal but we need to make sure that we don't set mIsDestroyed var to set until closing channel has been done successfully.
So, we can have a chance to close unclosed channel later on.

When ShutDownProcess called once by others(NotifyTabDestoryed or observe) and it fails to close() channel, we cannot have any other chance to close channel because of mIsDestoryed.
But I don't think it can happen easily just in case of...

Justin, could you please check this patch.
If you think we need this, I will attach new patch file well-formed for HG.
Thank you.
Summary: Notify "ChannelError" to ContentParent when the channel is in "Error" state → Make sure that closing channel is done successfully by ShutDownProcess()
We moved the mIsDestroying call up to the top of the function because it looked like ShutDownProcess was re-entering itself.  I'm not sure if that is still happening, but we should be cautious...
(In reply to Justin Lebar [:jlebar] from comment #3)
> We moved the mIsDestroying call up to the top of the function because it
> looked like ShutDownProcess was re-entering itself.  I'm not sure if that is
> still happening, but we should be cautious...
I agree with you. it should be very cautious.

But I want to share our internal test result.
we tested the version with this patch for 22 hours and the result seems very good to have no memory leakage in IPC messages.
Although the patch is not applied right now, could you review or comment about this patch?
Your comment maybe very helpful for leo to decide whether we should apply this patch to leo's internal version or not.
DMD, memory-report, cc and gc logs for this result are not yet shared with me.
Because of that, I am sorry for having only thing, summary of result.
Thanks for your patch, Jinho.

I think this patch is functionally equivalent to the patch you attached, but I prefer this approach because it doesn't rely on ContentParent::Close() doing nothing other than mChannel->Close(), and because it still guards against reentrancy of Close() and CloseWithError().

Would you mind testing this and letting me know if this helps?
Attachment #779269 - Flags: feedback?(faraojh)
Attachment #778404 - Flags: feedback?(justin.lebar+bug) → feedback+
Attachment #779269 - Flags: feedback?(jeremy.kim.leo)
Hi Justin, 
I'm Sorry. I could not respond immediately, I was out of the office for a week. 
The patch you attached looks okay with me. 

But, there is one thing I want you to know...
After, I came back to the office I got other test results. 
With/Without the patch (attachment 778404 [details] [diff] [review]), after some days we ran the test scripts there were a few channels which piled up messages on it.

And after we applied the patch(attachment 772478 [details] [diff] [review]) for bug 811636, we couldn't find any channels which makes this problem on b2g process.

Hence, I'm not sure about this patch's effectiveness on this app launching test cases and wanna ask your opinion.
How do you think on this result? do we need to keep applying this patch and testing it?
> How do you think on this result? do we need to keep applying this patch and testing it?

It sounds like you're saying that we don't need this patch for 1.1.  Maybe we should check this in for 1.2 and not fix it for 1.1.
Already those problem was fixed by Bug 893012. 
so, i think it doesn't need for 1.1.
Comment on attachment 779269 [details] [diff] [review]
Patch, v1: Let ContentParent call CloseWithError() even if it's already called Close().

Already those problem was fixed by Bug 893012. 
so, i think it doesn't need for 1.1.
Attachment #779269 - Flags: feedback-
Comment on attachment 779269 [details] [diff] [review]
Patch, v1: Let ContentParent call CloseWithError() even if it's already called Close().

sorry, wrong operation regarding as feedback
Attachment #779269 - Flags: feedback?(jeremy.kim.leo) → feedback-
Comment on attachment 779269 [details] [diff] [review]
Patch, v1: Let ContentParent call CloseWithError() even if it's already called Close().

I still think we want to do this in general.
Attachment #779269 - Flags: review?(khuey)
Attachment #779269 - Flags: feedback?(faraojh)
Attachment #779269 - Flags: feedback-
Attachment #778404 - Attachment is obsolete: true
Assignee: nobody → justin.lebar+bug
Summary: Make sure that closing channel is done successfully by ShutDownProcess() → Make sure that ContentParent::ShutDownProcess() successfully closes its channel
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P3]
This had merge conflicts on b2g-inbound, so I pushed to regular inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/411e364cd365
https://hg.mozilla.org/mozilla-central/rev/411e364cd365
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.