1436486 - consider removing necko e10s diversion mechanism

Status: RESOLVED DUPLICATE of bug 1604447

(Core :: Networking, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Necko currently has "diversion" mechanism for cases where an e10s nsIChannel needs to be consumed by code in the parent process.  It works roughly like this:

1. Child process creates and opens nsIChannel
2. Parent process performs network request, etc
3. parent process sends response headers and start of body to child
4. Child process sees response headers and decides "OMG, the parent process needs to handle this"
5. Diversion is begun.
6. The channel is paused
7. Any data chunks received by the child are sent back to the parent
8. The parent channel is re-wired not to send remaining chunks to the child nsIChannel, but instead to the parent consumer.
9. Channel is unpaused

This mechanism optimizes in that we don't need to copy the entire body data to send it back to the parent consumer.  It does, however, have some downsides:

a. Its complex in that it must handle the racy question of "send chunks back up until I paused the channel", which may have chunks in-flight in IPC or the event queue.
b. Its complex in that it must keep a state machine in-sync across two processes.
c. It makes it difficult to add any kind of child-side data synthesis, such as service workers or <link rel=preload>, because in those cases there is no parent channel to keep in sync, etc.
d. Diversion only occurs in a small subset of network requests which means its often not tested and bites us in release channels.

An alternative would be to simply copy the data back to the parent:

1. Child process creates and opens nsIChannel
2. Parent process performs network request, etc
3. parent process sends response headers and start of body to child
4. Child process sees response headers and decides "OMG, the parent process needs to handle this"
5. Send data chunks back to the parent as they are received
6. Parent routes chunks to final consumer

A bit wasteful with the copying, but perhaps easier to maintain and more stable overall.

Comment 1

[Honza Bambas (:mayhemer)]

Diversion usually applies to downloads which are large and long living.  Copying back all the data will just waste memory (piling up of data in the IPC queue) and slow everything down (the IPC queue will have to handle the data load, even if those are just smaller chunks, and not the priority tasks)

But I really understand the frustration with the diversion code, it would be great to improve here.

Other option is to just cancel and open a new channel (or, maybe an internal redirect?) with a flag "target is on parent, don't send the data to child" and start over.  Wastes some of the bandwidth, tho, so not very mobile friendly when we start e10s'ing there.

I don't recall now how exactly the extprotocol service handles downloads in e10s.  I'd start there to understand the overall mechanism.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> Diversion usually applies to downloads which are large and long living. 
> Copying back all the data will just waste memory (piling up of data in the
> IPC queue) and slow everything down (the IPC queue will have to handle the
> data load, even if those are just smaller chunks, and not the priority tasks)

Downloads don't happen on every page load, etc.  Its unclear to me how important it is to optimize them.

I think you make a good point, though, that if we went to a copy-back-to-parent-always approach it would need to have some kind of back pressure.

Would it be possible to make the decision to download in the parent process before we send OnStartRequest to the child?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I also just want to note that we do the simple "send nsIChannel data to child and copy back to parent to write to disk" in other places in the system already.  For example, Cache.add() does this and doesn't have any back pressure currently.  We haven't seen any performance problems from it, although it probably gets smaller body sizes than downloading an ISO, etc.

Still, it would be nicest if we could just move the code in the child back to the parent now that legacy addons are gone.  There are currently only two callers of divertToParent():

PSMContentListener:
https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/security/manager/ssl/PSMContentListener.cpp#304

ExternalHelperAppChild:
https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/uriloader/exthandler/ExternalHelperAppChild.cpp#117

It would be nice if we could set an enum on the channel's open IPC message that just setup the appropriate listeners on the parent side instead of the content side.

Comment 4

[Honza Bambas (:mayhemer)]

Let you know that it's really not that simple to move the decision to the parent process.  We also had a similar (related) issue where to decide on decompression of the content what revealed number of complex issues and was left on the content process.

I have to look for the bugs and comments and re-summarize here (not a small task).  I still think a good solution could be to use the internal redirect and throw some of the data already downloaded simply away.  Maybe I missed counter arguments (remind me), and I also don't want to delegate the burden of writing a solution to someone else just like that.