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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jinho Hwang [:jeffhwang]]

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.

Comment 1

[Jinho Hwang [:jeffhwang]]

Attachment: Do_Not_Set_mIsDestroyed_to_true.patch

Don't set mIsDestroyed to true until closing channel done

Comment 2

[Jinho Hwang [:jeffhwang]]

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.

Comment 3

[Justin Lebar (not reading bugmail)]

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

Comment 4

[Youngwoo Jo]

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

Comment 5

[Youngwoo Jo]

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.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1: Let ContentParent call CloseWithError() even if it's already called Close().

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?

Comment 7

[Jinho Hwang [:jeffhwang]]

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?

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[leo.bugzilla.gecko]

Already those problem was fixed by Bug 893012. 
so, i think it doesn't need for 1.1.

Comment 10

[jeremy.kim.leo [:jeremykimleo]]

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.

Comment 11

[jeremy.kim.leo [:jeremykimleo]]

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

Comment 12

[Justin Lebar (not reading bugmail)]

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.

Comment 13

[Justin Lebar (not reading bugmail)]

This had merge conflicts on b2g-inbound, so I pushed to regular inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/411e364cd365

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/411e364cd365