1104999 - ContentChild aborts in response to async message processing errors

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, defect)

Comment 1

[Jim Mathies [:jimm]]

This seems wrong to me since, we have sub protocols that are created on the child side and receive async messages from the parent. If the child side gets torn down with one of these messages in transit, all tabs crash. But really we don't care about these messages getting dropped, we should continue to run.

Comment 2

[Jim Mathies [:jimm]]

Attachment: provide some context for ipc protocol errors

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8528599 [details] [diff] [review]
provide some context for ipc protocol errors

This adds a new ProcessingErrorWithType to ipdl that protocol classes can implement to get more information on the error. I'm using this with ContentChild to avoid crashing on async message errors.

Comment 4

[Jim Mathies [:jimm]]

An alternative fix that would be cleaner, but also bigger, would be to go ahead and update all ProcessingError methods throughout the tree to include the new error value. Then we wouldn't need the added error handler. There aren't that many - 

http://mxr.mozilla.org/mozilla-central/search?string=ProcessingError%28

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8528599 [details] [diff] [review]
provide some context for ipc protocol errors

I think I'm going to go ahead and make those changes.

Comment 6

[Jim Mathies [:jimm]]

I think we can expose even more information here:

issue: connection, dispatching, route
msg type: async, sync, intr, unknown

This way ContentChild can target dropping just dispatch errors for async messages.

Comment 7

[Andrew McCreight [:mccr8]]

I don't know if it was expected to, but this didn't fix the bug 1103279 failure for me.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I don't know if it was expected to, but this didn't fix the bug 1103279
> failure for me.

Isn't that tied to a sync constructor call? In which case I wouldn't expect that it would.

Comment 9

[Jim Mathies [:jimm]]

Attachment: expose more detail to ProcessingError calls

Comment 10

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69963999397c

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8528677 [details] [diff] [review]
expose more detail to ProcessingError calls

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

In general I'm not a fan of relaxing our IPC rules like this. Dropping messages seems like an invitation for leaks and strange behavior to me because 1) it will probably only happen in race conditions, and 2) we won't get the normal ActorDestroy messages in the parent that we rely on to do last ditch cleanup since we won't be killing the child process.

IMO we should try to make it impossible for the parent to send messages to a child actor that has been torn down.

Benjamin, do you have opinions here?

::: ipc/glue/MessageLink.h
@@ +46,5 @@
> +
> +    // OnProcessingError message type codes - for ERROR_CLASS_DISPATCH
> +    // errors, indicates the type of message the channel was trying
> +    // to dispatch.
> +    enum DispatchType

This shouldn't be needed, you can ask the message for its type directly (is_sync(), is_interrupt())

Comment 12

[Jim Mathies [:jimm]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #11)
> Comment on attachment 8528677 [details] [diff] [review]
> expose more detail to ProcessingError calls
> 
> Review of attachment 8528677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general I'm not a fan of relaxing our IPC rules like this. Dropping
> messages seems like an invitation for leaks and strange behavior to me
> because 1) it will probably only happen in race conditions, and 2) we won't
> get the normal ActorDestroy messages in the parent that we rely on to do
> last ditch cleanup since we won't be killing the child process.

In the particular sub protocol I'm working with, all the normal ActorDestroy calls trigger. We're just left with a message in transit that doesn't have a target when it arrives. Our solution here is crash every tab in the browser. I think I'd take a risk of leaks over that experience.

> IMO we should try to make it impossible for the parent to send messages to a
> child actor that has been torn down.

I'm not sure this is possible with the way ipdl works. In this particular case:

- child initiated sub protocol connection
- the parent side sends an async event randomly from the main thread
- tab gets torn down, child sub protocol gets deleted
- async message arrives
- all tabs crashed

Comment 13

[Jim Mathies [:jimm]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #11)
> ::: ipc/glue/MessageLink.h
> @@ +46,5 @@
> > +
> > +    // OnProcessingError message type codes - for ERROR_CLASS_DISPATCH
> > +    // errors, indicates the type of message the channel was trying
> > +    // to dispatch.
> > +    enum DispatchType
> 
> This shouldn't be needed, you can ask the message for its type directly
> (is_sync(), is_interrupt())

Hmm not sure how this helps, ProccessingError() doesn't have access to the message. I need to hand something to ProccessingError, is the current message stashed someplace on ContentChild?

Comment 14

[Benjamin Smedberg]

I think that dropping messages is definitely unacceptable, and dealing with the correct way to shut down IPDL trees is an important project that we share with B2G. The protocols involved must be designed with two-stage shutdown systems so that we send an "intent to shutdown" which doesn't actually delete the protocols until the opposite side replies.

Ideally this would be done with IPDL state machines, but since those are very difficult to design, we should at least do it with handcrafted state passing (and assertions to validate them).