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 1. `Redirect3Complete` should be later than `Redirect1Begin`, no race. 1. `SendFailedAsyncOpen`: I'm more preferring wait for PHttpChannel constructed, given it's a failed channel. 1. `AssociateApplicationCache`: this is racy. We should send the information through OnStartRequest. https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/HttpChannelChild.cpp#393 1. `ReportSecurityMessage`: this is interesting. Given this is for Document (I believe), main thread in child is involved. I'll wait for PHttpChannel constructed. 1. `CancelRedirected`: should be removed in Bug 1645254 1. `DeleteSelf`: no need to handle. This should be sent after PHttpChannel constructed 1. `IssueDeprecationWarning`: this is for appcache. I would wait for PHttpChannel. https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/nsHttpChannel.cpp#9529 1. `LogBlockedCORSRequest` this happens when the request is blocked. Simply wait. No performance lose. 1. `LogMimeTypeMismatch`: same 1. `AttachStreamFilter`: I'm not sure this one, but it touches PContent. I would wait. 1. `CancelDiversion`: this is no op 1. ` 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. 1. `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. 1. `OnProgress`, `OnStatus`, `OnAfterLastPart`: should be removed in Bug 1639072 1. ` async NotifyClassificationFlags(uint32_t aClassificationFlags, bool aIsThirdParty);` 1. ` async NotifyFlashPluginStateChanged(FlashPluginState aState);` 1. `async SetClassifierMatchedInfo(ClassifierInfo info);` 1. `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. 1. `FinishInterceptedRedirect`: it should be fine. it's a ping-pong IPC, initiated from child 1. `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?
Bug 1639072 Comment 1 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
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 1. `Redirect3Complete` should be later than `Redirect1Begin`, no race. 1. `SendFailedAsyncOpen`: I'm more preferring wait for PHttpChannel constructed, given it's a failed channel. 1. `AssociateApplicationCache`: this is racy. We should send the information through OnStartRequest. https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/HttpChannelChild.cpp#393 1. `ReportSecurityMessage`: this is interesting. Given this is for Document (I believe), main thread in child is involved. I'll wait for PHttpChannel constructed. 1. `CancelRedirected`: should be removed in Bug 1645254 1. `DeleteSelf`: no need to handle. This should be sent after PHttpChannel constructed 1. `IssueDeprecationWarning`: this is for appcache. I would wait for PHttpChannel. https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/netwerk/protocol/http/nsHttpChannel.cpp#9529 1. `LogBlockedCORSRequest` this happens when the request is blocked. Simply wait. No performance lose. 1. `LogMimeTypeMismatch`: same 1. `AttachStreamFilter`: I'm not sure this one, but it touches PContent. I would wait. (Edit: see comment 4) 1. `CancelDiversion`: this is no op 1. ` 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. 1. `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. 1. `OnProgress`, `OnStatus`, `OnAfterLastPart`: should be removed in Bug 1639072 1. ` async NotifyClassificationFlags(uint32_t aClassificationFlags, bool aIsThirdParty);` 1. ` async NotifyFlashPluginStateChanged(FlashPluginState aState);` 1. `async SetClassifierMatchedInfo(ClassifierInfo info);` 1. `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. 1. `FinishInterceptedRedirect`: it should be fine. it's a ping-pong IPC, initiated from child 1. `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?