1639072 - Let PHttpBackgroundChannel be able to trigger the HTTP request

Status: NEW

(Core :: Networking: HTTP, enhancement, P3)

Description

[Junior [inactive]]

We want HttpChannelChild::AsyncOpen not to touch the main thread, so we need to send the asyncopen information through pHttpBackgroundChannel. Currently we use PHttpChannelConstructor to do this, and use registar to linked Channel and BgChannel when mBgParent is ready.

So we need to do the oppisite. Bring the HttpChannelOpenArgs through SendPHttpBackgroundChannelConstructor, dispatch to main thread and create the nsHttpChannel to move on. In the mean time, SendPHttpChannelConstructor with Channel ID, and linked them later.

We want to try not to wait for the PHttpChannel to hit the net. We want to move network request forward when content process main thread is busy.

Comment 1

[Junior [inactive]]

Once we send AsyncOpen throught pBG, we might not have constructed PHttpChannel IPC.
Hence, I check all parent->child messages PHttpChannel.ipdl to see which might be racy:

1. SendRedirect1Begin: we should wait for PHttpChannel constructed in StartRedirect
   https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/HttpChannelParent.cpp#2102

2. Redirect3Complete should be later than Redirect1Begin, no race.

3. SendFailedAsyncOpen: I'm more preferring wait for PHttpChannel constructed, given it's a failed channel.

4. AssociateApplicationCache: this is racy. We should send the information through OnStartRequest.
   https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/HttpChannelChild.cpp#393

5. ReportSecurityMessage: this is interesting. Given this is for Document (I believe), main thread in child is involved. I'll wait for PHttpChannel constructed.

6. CancelRedirected: should be removed in Bug 1645254

7. DeleteSelf: no need to handle. This should be sent after PHttpChannel constructed

8. IssueDeprecationWarning: this is for appcache. I would wait for PHttpChannel.
   https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/nsHttpChannel.cpp#9529

9. LogBlockedCORSRequest this happens when the request is blocked. Simply wait. No performance lose.

10. LogMimeTypeMismatch: same

11. AttachStreamFilter: I'm not sure this one, but it touches PContent. I would wait.
    (Edit: see comment 4)

12. CancelDiversion: this is no op

13. async OriginalCacheInputStreamAvailable(IPCStream? stream);
async AltDataCacheInputStreamAvailable(IPCStream? stream);
    I didn't have a close look, but it should be fine since this is issued from child main thread IPC first.

14. OverrideReferrerInfoDuringBeginConnect: this is tricky. It's in BeginConnect and I don't know how frequently it happens to
    ReEvaluateReferrerAfterTrackingStatusIsKnown and not sure if it should be in BeginConnect
    If we wait for IPC constructed, that is an indirect main thread blocking in child.
    I'll ni? in next comment.

15. OnProgress, OnStatus, OnAfterLastPart: should be removed in Bug 1639072

16. async NotifyClassificationFlags(uint32_t aClassificationFlags, bool aIsThirdParty);

17. async NotifyFlashPluginStateChanged(FlashPluginState aState);

18. async SetClassifierMatchedInfo(ClassifierInfo info);

19. async SetClassifierMatchedTrackingInfo(ClassifierInfo info);
    These are from pBg. and moving to PHttpChannel for synchronization.
    https://hg.mozilla.org/mozilla-central/rev/2dda521c0591c002217453ea3e8358651ad6a45d
    We should move it back.

20. FinishInterceptedRedirect: it should be fine. it's a ping-pong IPC, initiated from child

21. SetPriority: Last but not least, this could be set in an arbitrary timing.
    Looks like this child will use the priority only in the code path. Hence we could bring the priority in SendRedirect1Begin

mozilla::net::HttpChannelChild::RecvRedirect1Begin
mozilla::net::HttpChannelChild::Redirect1Begin
mozilla::net::HttpChannelChild::SetupRedirect
mozilla::net::HttpBaseChannel::SetupReplacementChannel {
...
  // Preserve the loading order
  nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(newChannel);
  if (p) {
    p->SetPriority(mPriority);
  }
}

To conclude, only AttachStreamFilter could hurt the performance and I'll ask ni? for the referrer.
4, 16-19 should be done in bug 1639072.

Does it make sense, Honza?

Comment 2

[Junior [inactive]]

Comment 1

Comment 3

[Junior [inactive]]

https://phabricator.services.mozilla.com/D49773#inline-303635

For OverrideReferrerInfoDuringBeginConnect, looks like the key point is to get the referrerInfo before OnStartRequest

Comment 4

[Junior [inactive]]

To conclude, only AttachStreamFilter could hurt the performance and I'll ask ni? for the referrer.

For AttachStreamFilter, the WebRequest will suspend the channel in nsHttpChannel::PrepareToConnect(), i.e., http-on-modify-request -> onBeforeRequest.
Apply the stream filter, send the main thread IPC stream to child and resume.

That is, PHttpChannel::AttachStreamFilter should be sent before OnStartRequest.

4, 16-19 should be done in bug 1639072.

Actually it's bug 1645527.

Comment 5

[Junior [inactive]]

We might want the nsIStremListener to receive OnStart/StopRequest off the main thread.
Unfortunately, all loaders except workers are touching the main thread eventually in child process.
Therefore, we can't have performance gain by saving event dispatching.

That reminds us of another motivation.

Workers could avoid touching the main thread with respect to spec and performance.
That's more attractive, but we found that OMT AsyncOpen could be a complicated piece.

If we want to avoid touching main thread in AsyncOpen., the lifecycle of nsHttpChannel could not depend on HttpChannelParent, i.e., no HttpChannelParent is created.

To achieve that, we need to move out most of the code from HttpChannelParent to HttpChannelBackgroundParent, including OS*R/ODA/OnProgress/..., which could break lots of things
Moreover, we prompt the events through ParentChannelListener, which implements several interfaces
https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/ParentChannelListener.h#33-38

That is, we need to let HttpChannelBackgroundParent handles those events, including service worker and auth prompt.
Otherwise, we need to reconsider the structure with DocumentChannel, that would be another long story.

However, I know you mention OMT ShouldIntercept is possible, but there's more dependency.
We still need to handle getAuthPrompt in HttpChannelBackgroundParent, which happens on the main thread.
Moreover, it also depends on appcache deprecation and child intercept in the child side (bug 1639433 comment 0)

And even more, we need to handle all the races in comment 1
Touching any one of the IPC message which needs to wait HttpChannelParent could not have performance gain.
The most inevitable one is redirect, we have permission check/content security check/... in both threads, which is on the main thread now.
Not sure how much work on them.

On the other hand, workers is planned to have its own PFetch IPC which is managed by PBackground (see Bug 1613912 comment 4)
It might be not a strong motivation to avoid touching main thread for all necko requests.