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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] pbg-m3.patch

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

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: pbg-m3.patch

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.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: bug1338493-debug.patch

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 4

[Honza Bambas (:mayhemer)]

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 5

[Honza Bambas (:mayhemer)]

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 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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 8

[Kris Maglione [:kmag]]

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.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1338493 - Part 1, make webReqeust test to fiddle the main event loop after loading CSS/JS.

Making ODA completely off-main-thread will make the order webRequest.onCompleted and CSS/JS execution unpredictable.
|window.requestIdleCallback| is used to fiddle the main thread event queue.

Review commit: https://reviewboard.mozilla.org/r/149798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149798/

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1338493 - Part 2, move HttpBackgroundChannelChild to socket transport thread.

Move HttpBackgroundChannelChild from main thread to socket transport thread.
Allow HttpChannelChild.mBgChild to be used on both main thread and STS thread under mutex protection.

Review commit: https://reviewboard.mozilla.org/r/149800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149800/

Comment 12

[Kris Maglione [:kmag]]

(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.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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 14

[Honza Bambas (:mayhemer)]

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.

Comment 15

[Honza Bambas (:mayhemer)]

And sorry for the delay!

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 17

[Kris Maglione [:kmag]]

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

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8878449 [details]
Bug 1338493 - Part 1, make webReqeust test to fiddle the main event loop after loading CSS/JS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149798/diff/1-2/

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8878450 [details]
Bug 1338493 - Part 2, move HttpBackgroundChannelChild to socket transport thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149800/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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

Comment 22

[Ionuț Goldan [:igoldan]]

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