move HttpChannel IPC from main thread to STS thread in content process

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: schien, Assigned: schien)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-active][PBg-HTTP-M3])

Attachments

(2 attachments, 3 obsolete attachments)

After bug 1015466, we should use STS thread in content process for HttpChannel IPC.
Whiteboard: [necko-next]
Whiteboard: [necko-next] → [necko-next][PBg-HTTP-M3]

Updated

2 years ago
Whiteboard: [necko-next][PBg-HTTP-M3] → [necko-active][PBg-HTTP-M3]
Posted patch [WIP] pbg-m3.patch (obsolete) — Splinter Review
draft version, able to load web page, image, mp4 video, MSE/EME video, youtube video successfully without crash on Linux.
Posted patch pbg-m3.patch (obsolete) — Splinter Review
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.
Attachment #8868496 - Attachment is obsolete: true
Attachment #8870267 - Flags: feedback?(honzab.moz)
Posted patch bug1338493-debug.patch (obsolete) — Splinter Review
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.
Attachment #8870268 - Flags: feedback?(honzab.moz)
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?
Attachment #8870267 - Flags: feedback?(honzab.moz) → feedback+
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.
Attachment #8870268 - Flags: feedback?(honzab.moz)
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.
Attachment #8870268 - Flags: feedback?(kmaglione+bmo)
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.
Attachment #8870268 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8870267 - Attachment is obsolete: true
Attachment #8870268 - Attachment is obsolete: true
(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.
Flags: needinfo?(schien)
(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.
Flags: needinfo?(schien)

Comment 14

2 years ago
mozreview-review
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.
Attachment #8878450 - Flags: review?(honzab.moz) → review+
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.
Flags: needinfo?(kmaglione+bmo)

Comment 17

2 years ago
mozreview-review
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
Attachment #8878449 - Flags: review?(kmaglione+bmo) → review+
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e46c37ddd5ff
https://hg.mozilla.org/mozilla-central/rev/6ad9b54d57a9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
Duplicate of this bug: 1369682
You need to log in before you can comment on or make changes to this bug.