911009 - Add error handling for state of ChannelError in AsyncChannel::ProcessLink::Open

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shelly Lin [:shelly]]

It has been observed that b2g sometimes freezes while trying to launch an app.

We've found that the b2g process hangs at:
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/AsyncChannel.cpp#l163

When launching a child process, ContentParent issues three channel openings, the process itself, compositor, and image bridge.

If something goes wrong *in between* the openings, this monitor is waited and never notified.

Comment 1

[Shelly Lin [:shelly]]

Attachment: Handle the ChannelError when opening a ProcessLink

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

This looks to be a right direction. Just the check
> +               mChan->mChannelState != AsyncChannel::ChannelError) {
doesn't look good. It doesn't handle all errors, either. We also need to ensure what to do when we bail out from this loop.

Do each added Monitor::Notify() have error condition to resolve?

Comment 3

[Shelly Lin [:shelly]]

(In reply to Cervantes Yu from comment #2)
> This looks to be a right direction. Just the check
> > +               mChan->mChannelState != AsyncChannel::ChannelError) {
> doesn't look good. It doesn't handle all errors, either. We also need to
> ensure what to do when we bail out from this loop.
Yeah it is not the prettiest fix. However, it only turns to ChannelClosing state when receiving a "GoodbyeMessage", which means a proper app kill, and turns to ChannelClosed at a normal channel close or some abnormal shutdown. The ipc communication of PContentParent, PCompositor, and PImageBridge are all continue-if-timeout, so the channel state won't set to ChannelTimeout in this case. As a result, I *think* it's fine to check just the ChaneelError state here.

> 
> Do each added Monitor::Notify() have error condition to resolve?
Yes.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 797733 [details] [diff] [review]
Handle the ChannelError when opening a ProcessLink

>@@ -574,16 +575,17 @@ AsyncChannel::NotifyMaybeChannelError()
>         // warning us about it. no worries
>         mChannelState = ChannelClosed;
>         NotifyChannelClosed();
>         return;
>     }
> 
>     // Oops, error!  Let the listener know about it.
>     mChannelState = ChannelError;
>+    mMonitor->Notify();

AsyncChannel::NotifyMaybeChannelError() only runs on the "worker"
thread, so it looks like this shouldn't affect the underlying bug
here.  Can you please confirm?

Or if does affect the bug, please explain why :).  It's not obvious to
me.

I think I see what the problem is here so I'm comfortable reviewing
after my question above is answered.

Comment 5

[Shelly Lin [:shelly]]

Attachment: Handle the ChannelError when opening a ProcessLink

Yep, you're right. |AsyncChannel::NotifyMaybeChannelError()| asserts for running on non-owner thread, so shouldn't block the main thread theoretically. Also, I found out that |AsyncChannel::CloseWithError()| runs only on the worker thread too, thus I've removed the Notify() call there.

Thank you! :)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 800556 [details] [diff] [review]
Handle the ChannelError when opening a ProcessLink

I wish we could write a test, but I can't think of a way off the top of my head :/.

Comment 7

[Shelly Lin [:shelly]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=03fff7751c95

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a3fcdf6cd36

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a3fcdf6cd36