Closed Bug 1338493 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: schien, Assigned: schien)

References

Details

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

Attachments

(2 files, 3 obsolete files)

Bug 1338493 - Part 1, make webReqeust test to fiddle the main event loop after loading CSS/JS.
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1338493 - Part 2, move HttpBackgroundChannelChild to socket transport thread.
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
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]
Whiteboard: [necko-next][PBg-HTTP-M3] → [necko-active][PBg-HTTP-M3]
Attached 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.
Attached 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)
Attached 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)
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 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 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)
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: