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

RESOLVED FIXED in 1.2 FC (16sep)

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shelly, Assigned: shelly)

Tracking

unspecified
1.2 FC (16sep)
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 797733 [details] [diff] [review]
Handle the ChannelError when opening a ProcessLink
(Assignee)

Updated

5 years ago
Attachment #797733 - Flags: feedback?(cyu)
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?
Attachment #797733 - Flags: feedback?(cyu)
(Assignee)

Comment 3

5 years ago
(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 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.
Flags: needinfo?(slin)
(Assignee)

Comment 5

5 years ago
Created attachment 800556 [details] [diff] [review]
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! :)
Assignee: nobody → slin
Attachment #797733 - Attachment is obsolete: true
Attachment #800556 - Flags: review?(cjones.bugs)
Flags: needinfo?(slin)
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 :/.
Attachment #800556 - Flags: review?(cjones.bugs) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

5 years ago
Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=03fff7751c95
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a3fcdf6cd36
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.