After bug 1015466, we should use STS thread in content process for HttpChannel IPC.

draft version, able to load web page, image, mp4 video, MSE/EME video, youtube video successfully without crash on Linux.

HttpBackgroundChannelChild is moved from main thread to socket transport thread and all the operation to HttpChannelChild.mBgChild is moved to socket transport thread as well.

On try server I found the timing of webRequest is changed a little bit. The original test case is checking that CSS/JS resources is loaded and executed after webRequest.onCompleted. After ODA OMT, the webRequest.onCompleted may fired before the CSS/JS is executed. Not sure if this violates with the definition of WebRequest API.

Comment on attachment 8870267 [details] [diff] [review] pbg-m3.patch Review of attachment 8870267 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp @@ +367,5 @@ > HttpBackgroundChannelChild::ActorDestroy(ActorDestroyReason aWhy) > { > LOG(("HttpBackgroundChannelChild::ActorDestroy[this=%p]\n", this)); > + > + if (NS_WARN_IF(!OnSocketThread())) { if this is legal, according the comment, do you really want to NS_WARN? ::: netwerk/protocol/http/HttpChannelChild.cpp @@ +455,5 @@ > + MOZ_ASSERT(self->mBgChild); > + if (self->mBgChild) { > + self->mBgChild->OnStartRequestReceived(); > + } > + }), NS_DISPATCH_NORMAL); this needs an explanation (i'm lost among all your patches.. sorry) is it done this way just to keep ordering right since it all ends up in the child's mEventQueue eventually? remember you have to call the final listener's OnStartRequest on the main thread @@ +1321,5 @@ > } > } > > void > +HttpChannelChild::CreateBackgroundChannel(bool aAssertExistingBgChild) |assert *not* existing| rather?

Comment on attachment 8870268 [details] [diff] [review] bug1338493-debug.patch Review of attachment 8870268 [details] [diff] [review]: ----------------------------------------------------------------- I'm not the right person to assess this.

Comment on attachment 8870268 [details] [diff] [review] bug1338493-debug.patch Hi Kris, I found test_ext_webrequest_basic.html failed after my HTTP off-main-threading work. The original test case is checking that CSS/JS resources is loaded and executed after webRequest.onCompleted. After ODA OMT, the webRequest.onCompleted may fired before the CSS/JS is executed. Not sure if this violates with the definition of WebRequest API. Maybe you can shed some light on this.

Comment on attachment 8870267 [details] [diff] [review] pbg-m3.patch Review of attachment 8870267 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp @@ +367,5 @@ > HttpBackgroundChannelChild::ActorDestroy(ActorDestroyReason aWhy) > { > LOG(("HttpBackgroundChannelChild::ActorDestroy[this=%p]\n", this)); > + > + if (NS_WARN_IF(!OnSocketThread())) { Yes should remove the NS_WARN_IF. ::: netwerk/protocol/http/HttpChannelChild.cpp @@ +455,5 @@ > + MOZ_ASSERT(self->mBgChild); > + if (self->mBgChild) { > + self->mBgChild->OnStartRequestReceived(); > + } > + }), NS_DISPATCH_NORMAL); yes, child's mEventQueue is the place to control the order of events from both main thread IPC and Pbackground IPC. Here is the comment I am adding: // Child's mEventQ is to control the execution order of the IPC messages // from both main thread IPDL and PBackground IPDL. // To guarantee the ordering, PBackground IPC messages that sent after // OnStartRequest will be throttled until OnStartRequest hits the Child's // mEventQ. @@ +1321,5 @@ > } > } > > void > +HttpChannelChild::CreateBackgroundChannel(bool aAssertExistingBgChild) sure, that would be more clear.

Comment on attachment 8870268 [details] [diff] [review] bug1338493-debug.patch Review of attachment 8870268 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6) > I found test_ext_webrequest_basic.html failed after my HTTP > off-main-threading work. > > The original test case is checking that CSS/JS resources is loaded and > executed after webRequest.onCompleted. After ODA OMT, the > webRequest.onCompleted may fired before the CSS/JS is executed. > > Not sure if this violates with the definition of WebRequest API. Maybe you > can shed some light on this. It doesn't violate any expectations of the API, no. In fact, I'm surprised that we were relying on that timing before. I think it only worked because events are dispatched asynchronously, and wind up happening on the next tick after internal onStop listeners trigger the injection/execution. And in the case of CSS, we're just getting lucky that the script is small enough that OMT parsing isn't used. Is there any deterministic way to tell when the request has been handled by the child, though? I'd rather not resort to a timeout loop if we don't have to.

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #8) > Is there any deterministic way to tell when the request has been handled by > the child, though? I'd rather not resort to a timeout loop if we don't have > to.

(In reply to Kris Maglione [:kmag] from comment #12) > (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #8) > > Is there any deterministic way to tell when the request has been handled by > > the child, though? I'd rather not resort to a timeout loop if we don't have > > to. WebRequest.onComplete already uses httpChannel.onStopRequest event, which is the deterministic way to know the content is loaded completely. However there is still a small gap between content loaded to content executed. Right now window.requestIdleCallback() is the only way I found to avoid timer loop.

Comment on attachment 8878450 [details] Bug 1338493 - Part 2, move HttpBackgroundChannelChild to socket transport thread. https://reviewboard.mozilla.org/r/149800/#review156706 r+ with comments, based more on trust this is OK than as-careful-as-this-should-deserve inspection. ::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp:424 (Diff revision 1) > > + if (!OnSocketThread()) { > + // PBackgroundChild might be destroyed during shutdown and > + // ActorDestroy will be called on main thread directly. > + // Simply disconnect with HttpChannelChild to release memory. > + mChannelChild = nullptr; You must re-dispatch this release to the socket thread. If the dispatch fails, only then you can release on the main thread. This way it's too racy. ::: netwerk/protocol/http/HttpBaseChannel.h:499 (Diff revision 1) > > // Resumable channel specific data > nsCString mEntityID; > uint64_t mStartPos; > > - nsresult mStatus; > + Atomic<nsresult> mStatus; please make sure you really need full sequencing here. ::: netwerk/protocol/http/HttpChannelChild.cpp:293 (Diff revision 1) > - MOZ_ASSERT(mBgChild); > + > + { > + MutexAutoLock lock(mBgChildMutex); > + > + if (mBgChild != aBgChild) { > + return; this needs an explanatory comment ::: netwerk/protocol/http/HttpChannelChild.cpp:478 (Diff revision 1) > altDataType, altDataLen)); > > - MOZ_ASSERT(mBgChild); > + { > + // Child's mEventQ is to control the execution order of the IPC messages > + // from both main thread IPDL and PBackground IPDL. > + // To guarantee the ordering, PBackground IPC messages that sent after 'that ARE sent' ? ::: netwerk/protocol/http/HttpChannelChild.cpp:1695 (Diff revision 1) > { > LOG(("HttpChannelChild::ProcessNotifyTrackingProtectionDisabled [this=%p]\n", this)); > - MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(OnSocketThread()); > > - nsChannelClassifier::NotifyTrackingProtectionDisabled(this); > + RefPtr<HttpChannelChild> self = this; > + NS_DispatchToMainThread(NS_NewRunnableFunction([self]() { Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1343755#c9 NS_DispatchToMainThread is dead, AFAIK. ::: netwerk/protocol/http/HttpChannelChild.cpp:1889 (Diff revision 1) > + > + RefPtr<HttpChannelChild> self = this; > + nsresult rv = > + gSocketTransportService->Dispatch( > + NewRunnableMethod<RefPtr<HttpChannelChild>>( > + bgChild, &HttpBackgroundChannelChild::Init, self), nit: Move(self)? ::: netwerk/protocol/http/HttpChannelChild.cpp:2122 (Diff revision 1) > > NS_IMETHODIMP > HttpChannelChild::Suspend() > { > LOG(("HttpChannelChild::Suspend [this=%p, mSuspendCount=%" PRIu32 ", " > - "mDivertingToParent=%d]\n", this, mSuspendCount+1, mDivertingToParent)); > + "mDivertingToParent=%d]\n", this, mSuspendCount+1, when you are here, can you add spaces around the plus operator? ::: netwerk/protocol/http/HttpChannelChild.cpp:2148 (Diff revision 1) > > NS_IMETHODIMP > HttpChannelChild::Resume() > { > LOG(("HttpChannelChild::Resume [this=%p, mSuspendCount=%" PRIu32 ", " > - "mDivertingToParent=%d]\n", this, mSuspendCount-1, mDivertingToParent)); > + "mDivertingToParent=%d]\n", this, mSuspendCount-1, the same here ::: netwerk/protocol/http/HttpChannelChild.cpp:2548 (Diff revision 1) > + NS_DISPATCH_NORMAL); > } > > + mBgInitFailCallback = NewRunnableMethod<nsresult>( > + this, &HttpChannelChild::FailedAsyncOpen, > + NS_ERROR_FAILURE); assert !mBgInitFailCallback before this (do this always when you assign to it please.

And sorry for the delay!

Hi @kmag, please see my reply in comment #13. We can have a face-to-face discussion this week if necessary.

Comment on attachment 8878449 [details] Bug 1338493 - Part 1, make webReqeust test to fiddle the main event loop after loading CSS/JS. https://reviewboard.mozilla.org/r/149798/#review158106

Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e46c37ddd5ff Part 1, make webReqeust test to fiddle the main event loop after loading CSS/JS. r=kmag https://hg.mozilla.org/integration/autoland/rev/6ad9b54d57a9 Part 2, move HttpBackgroundChannelChild to socket transport thread. r=mayhemer

https://hg.mozilla.org/mozilla-central/rev/e46c37ddd5ff https://hg.mozilla.org/mozilla-central/rev/6ad9b54d57a9

Improvements noticed, thanks to this bug: == Change summary for alert #7647 (as of June 29 2017 23:13 UTC) == Improvements: 4% damp summary linux64 opt e10s 234.48 -> 224.62 4% damp summary linux64 pgo e10s 205.50 -> 197.53 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7647