1015466 - Sending HTTP OnDataAvailable over PBackground IPC

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Since bug 497003 was fixed Necko has had the ability to deliver data from the socket threads to consumers on other threads (such as the HTML parser, which runs off the main thread) without touching the main thread.  This is all thrown out the window in the IPC case though, where we have to go to the main thread in the parent process to send the IPC message and the main thread in the child process to receive it.  That means we have to wait for *two* threads event queues to process in order to deliver data to the parser.

But thanks to PBackground we can do direct non-main-thread to non-main-thread transport now, so we should be able to skip both main threads and deliver data directly to the off main thread consumers.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Alex is actively working on this.

Comment 2

[atifrea]

Attachment: Bug 1015466 - Patch 1: Added the PHttpRetargetChannel protocol. r=mrbkap

In this first patch I have added the PHttpRetargetChannel protocol as a subprotocol of PBackground and I have written the code that initializes the actors in the right place.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8460343 [details] [diff] [review]
Bug 1015466 - Patch 1: Added the PHttpRetargetChannel protocol. r=mrbkap

Review of attachment 8460343 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any major comments to make here. I have a bunch of nitpicks and a few questions that need to be answered, but this approach looks pretty good so far!

::: dom/ipc/ContentParent.cpp
@@ +3767,5 @@
> +void
> +ContentParent::AddHttpRetargetChannel(uint32_t aKey,
> +                                      PHttpRetargetChannelParent* aData)
> +{
> +   mHttpRetargetChannels.Put(aKey, aData);

Where are you going to call Remove for this retarget channel?

::: dom/ipc/ContentParent.h
@@ +679,4 @@
>      bool mCalledKillHard;
>  
>      friend class CrashReporterParent;
> +    friend class SendMyselfToMainThread;

Why does this need to be a friend?

@@ +685,5 @@
>      nsConsoleService* GetConsoleService();
>  
>      nsDataHashtable<nsUint64HashKey, nsRefPtr<ParentIdleListener> > mIdleListeners;
>  
> +    nsDataHashtable<nsUint32HashKey, PHttpRetargetChannelParent* > mHttpRetargetChannels;

Nit: No space after *.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +120,5 @@
>  }
>  
> +PHttpRetargetChannelChild*
> +BackgroundChildImpl::AllocPHttpRetargetChannelChild(const uint32_t& channelId)
> +{   

Uber-nit: trailing whitespace.

::: ipc/glue/BackgroundImpl.cpp
@@ +955,4 @@
>      // will run before the reference we hand out can be released, and the
>      // ContentParent can't die as long as the existing reference is maintained.
>      nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewNonOwningRunnableMethod(actor->mContent.get(), &ContentParent::AddRef);

You can get rid of this change with the patch for bug 1041881 right?

::: ipc/glue/BackgroundParentImpl.h
@@ +8,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ipc/PBackgroundParent.h"
> +#include "mozilla/net/PHttpRetargetChannelParent.h"
> +
> +using namespace mozilla::net;

Unfortunately, the style guide prohibits using namespace declarations in header files.

::: netwerk/ipc/HttpRetargetChannelChild.cpp
@@ +54,5 @@
> +
> +        HttpChannelChild* tmp = static_cast<HttpChannelChild*>(mHttpChannel);
> +        tmp->OnTransportAndData(mChannelStatus, mTransportStatus,
> +                                         mProgress, mProgressMax, mData,
> +                                        mOffset, mCount);

The indentation seems wrong here (in general, too: this class is indented to 4 spaces and the rest of the file to 2).

@@ +96,5 @@
> +
> +HttpRetargetChannelChild::HttpRetargetChannelChild(uint32_t aChannelId)
> +{
> +  MOZ_COUNT_CTOR(HttpRetargetChannelChild);
> +  mChannelId = aChannelId;

I'd use initializer syntax here.

::: netwerk/ipc/HttpRetargetChannelChild.h
@@ +24,5 @@
> +                                      const nsCString& data,
> +                                      const uint64_t& offset,
> +                                      const uint32_t& count);
> +
> +  HttpRetargetChannelChild();

This isn't actually needed, right? We should get rid of it to prevent confusion.

::: netwerk/ipc/HttpRetargetChannelParent.cpp
@@ +40,5 @@
> +// Helper class for sending a reference to the HttpRetargetChannelParent to the
> +// Content actor on the main thread
> +//-----------------------------------------------------------------------------
> +
> +class SendMyselfToMainThread : public nsRunnable

I'm not a huge fan of this name. How about "AddToHashtableRunnable" or something like that? Something to indicate the desired effect of the runnable.

@@ +70,5 @@
> +    HttpRetargetChannelParent* tmpHttpRetarget =
> +        static_cast<HttpRetargetChannelParent*>(mHttpRetargetChannelParent);
> +
> +    mContentParent->AddHttpRetargetChannel(/* key */ tmpHttpRetarget->GetChannelId(),
> +                                           /* value */ mHttpRetargetChannelParent);

I don't think these comments add much value.

@@ +71,5 @@
> +        static_cast<HttpRetargetChannelParent*>(mHttpRetargetChannelParent);
> +
> +    mContentParent->AddHttpRetargetChannel(/* key */ tmpHttpRetarget->GetChannelId(),
> +                                           /* value */ mHttpRetargetChannelParent);
> +    mContentParent = NULL;

Nit: s/NULL/nullptr/.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +52,4 @@
>  {
>    LOG(("Creating HttpChannelChild @%x\n", this));
>  
> +  mBackgroundChild = BackgroundChild::GetForCurrentThread();

Instead of holding the background child as a member, I'd just get it in HttpChannelChild::ConnectParent.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1687,5 @@
>          return rv;
>  
>      if (IsNeckoChild()) {
> +        uint32_t channelID = GenerateChannelID();
> +        httpRetargetChannel = new HttpRetargetChannelChild(channelID);

This doesn't seem to do anything as far as I can tell. I guess you're going to add to it?

::: parser/html/nsHtml5StreamParser.h
@@ +495,4 @@
>       * The thread this stream parser runs on.
>       */
>      nsCOMPtr<nsIThread>           mThread;
> +

This looks like it snuck in.

Comment 4

[atifrea]

Attachment: Bug 1015466 - Patch 1 v2: Added the PHttpRetargetChannel protocol. r=mrbkap

Comment 5

[atifrea]

Attachment: Bug 1015466 - Patch 1 v2: Added the PHttpRetargetChannel protocol. r=mrbkap

Sorry. This is the correct second version of the patch (I messed up the previous one).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8464897 [details] [diff] [review]
Bug 1015466 - Patch 1 v2: Added the PHttpRetargetChannel protocol. r=mrbkap

Review of attachment 8464897 [details] [diff] [review]:
-----------------------------------------------------------------

Getting closer. One bug fix and a few nits and I don't see too much else to fix with this part.

::: dom/ipc/ContentParent.cpp
@@ +1794,4 @@
>      InitInternal(aInitialPriority,
>                   true, /* Setup off-main thread compositing */
>                   true  /* Send registered chrome */);
> +

Why add these newlines?

::: dom/ipc/ContentParent.h
@@ +30,5 @@
>  
>  #define CHILD_PROCESS_SHUTDOWN_MESSAGE NS_LITERAL_STRING("child-process-shutdown")
>  
> +typedef mozilla::net::PHttpRetargetChannelParent PHttpRetargetChannelParent;
> +typedef mozilla::net::PHttpChannelParent PHttpChannelParent;

These seem out of place in the .h here. Can we do without them? (They basically are using declarations, which we don't put in .h files).

::: ipc/glue/PBackground.ipdl
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +//using namespace mozilla::net;

?

::: netwerk/ipc/HttpRetargetChannelChild.cpp
@@ +105,5 @@
> +HttpRetargetChannelChild::Init()
> +{
> +  PBackgroundChild* backgroundChild = BackgroundChild::GetForCurrentThread();
> +
> +  if (backgroundChild)

Nit: I'd write this as:

if (!backgroundChild)
  return NS_ERROR_FAILURE;

...;

::: netwerk/ipc/HttpRetargetChannelChild.h
@@ +24,5 @@
> +                                      const nsCString& data,
> +                                      const uint64_t& offset,
> +                                      const uint32_t& count);
> +
> +  HttpRetargetChannelChild(uint32_t aChannelId);

Uber-nit: I'd move the constructor and destructor up above the function declaration.

::: netwerk/ipc/HttpRetargetChannelParent.cpp
@@ +70,5 @@
> +
> +    mContentParent->AddHttpRetargetChannel(mHttpRetargetChannelParent->GetChannelId(),
> +                                           mHttpRetargetChannelParent);
> +
> +    if (mContentParent->GetMustCallAsyncOpen()) {

Content parents might have several HTTP connections opening at the same time. This bit of information probably needs to be per-channel-id instead of global for the entirety of the content parent.

@@ +72,5 @@
> +                                           mHttpRetargetChannelParent);
> +
> +    if (mContentParent->GetMustCallAsyncOpen()) {
> +      // | HttpChannelParent::DoAsyncOpen1 | could not call
> +      // | HttpChannelParent::DoAsyncOpen2 |; that's why it will be called now.

This comment needs to be fleshed out to explain how this happens.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +158,4 @@
>  //-----------------------------------------------------------------------------
>  
>  bool
> +HttpChannelParent::DoAsyncOpen1(  const URIParams&           aURI,

Nit: undo this.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +75,5 @@
>  
> +  void SetChannelID(uint32_t channelID) { mChannelID = channelID; }
> +  uint32_t GetChannelId() { return mChannelID; }
> +
> +  bool DoAsyncOpen2(const PHttpRetargetChannelParent* aHttpRetargetChannelParent);

We're going to have to work on these names :)

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1684,4 @@
>                                   nsIChannel **result)
>  {
>      nsRefPtr<HttpBaseChannel> httpChannel;
> +    HttpRetargetChannelChild* httpRetargetChannel;

Nit: Declare this down where you assign to it.

@@ +1704,5 @@
> +        uint32_t channelID = GenerateChannelID();
> +        httpRetargetChannel = new HttpRetargetChannelChild(channelID);
> +        httpChannel = new HttpChannelChild(channelID);
> +
> +        rv = httpRetargetChannel->Init();

Please add a comment that IPDL now owns this object.

Comment 7

[atifrea]

Attachment: Bug 1015466 - Patch 1 v3: Added the PHttpRetargetChannel protocol. r=mrbkap

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8470318 [details] [diff] [review]
Bug 1015466 - Patch 1 v3: Added the PHttpRetargetChannel protocol. r=mrbkap

Review of attachment 8470318 [details] [diff] [review]:
-----------------------------------------------------------------

We're definitely getting there! I think this is now ready to be looked at by sworkman on the next iteration.

::: dom/ipc/ContentParent.cpp
@@ +3807,5 @@
> +}
> +
> +void
> +ContentParent::AddHttpChannel(uint32_t aKey,
> +                                      PHttpChannelParent* aData)

Uber-nit: the P should line up with the 'u' in uint32_t.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1704,5 @@
>          return rv;
>  
>      if (IsNeckoChild()) {
> +        uint32_t channelID = GenerateChannelID();
> +        HttpRetargetChannelChild* httpRetargetChannel

Nit: Put the '=' on the first line.

Comment 9

[atifrea]

Attachment: Bug 1015466 - Patch 1 v4: Retarget the messages to the background thread.

This is the current version of the patch. Redirecting works now. I still have some problems with OnProgress (it seems that the IPDL methods are not called on the worker thread as they should) but I will sort that out soon. Could you please take a look on what I did so far an tell me what you think about it? Thanks a lot :)

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8475599 [details] [diff] [review]
Bug 1015466 - Patch 1 v4: Retarget the messages to the background thread.

Review of attachment 8475599 [details] [diff] [review]:
-----------------------------------------------------------------

Cool. The basic idea seems sound to me. I added some comments, but I know you're still working on it, so ignore them if you've already change the code.

::: dom/ipc/ContentParent.cpp
@@ +3873,5 @@
> +ContentParent::AddHttpRetargetChannel(uint32_t aKey,
> +                                      PHttpRetargetChannelParent* aData)
> +{
> +  mHttpRetargetChannels.Put(aKey, aData);
> +}

Blake or Ben can look over ContentParent/Child. But looks ok to me :)

::: ipc/glue/BackgroundParent.h
@@ +8,4 @@
>  #include "base/process.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ipc/Transport.h"
> +#include "mozilla/StaticPtr.h"

Looks like you don't need this.

::: netwerk/ipc/HttpRetargetChannelChild.cpp
@@ +36,5 @@
> +                       progressMax, data, offset, count);
> +  return true;
> +}
> +
> +HttpRetargetChannelChild::HttpRetargetChannelChild(uint32_t aChannelId)

Contructor, destructor and init at the top of the file please.

@@ +51,5 @@
> +
> +nsresult
> +HttpRetargetChannelChild::Init(PHttpChannelChild* aHttpChannel)
> +{
> +  LOG(("HttpRetargetChannelChild::Init [this=%p httpChannel=%p]\n",this,aHttpChannel));

I assume these functions run on the main thread. Add MOZ_ASSERT(NS_IsMainThread(), "Must run on main thread!");

Check the other functions too.

@@ +61,5 @@
> +
> +  backgroundChild->SendPHttpRetargetChannelConstructor(this, mChannelId);
> +
> +  if (!aHttpChannel)
> +    return NS_ERROR_FAILURE;

Let's fail on this at the top of the function. Also, add braces for all ifs. Also also, add a MOZ_ASSERT_UNREACHABLE statement, which will crash the child process if nullptr is passed in.

@@ +66,5 @@
> +
> +  mHttpChannel = aHttpChannel;
> +
> +  // Add a new entry in the hashtable.
> +  ContentChild* contentChild = ContentChild::GetSingleton();

The comment isn't needed. Or else be more specific about which hashtable :)

::: netwerk/ipc/HttpRetargetChannelParent.cpp
@@ +72,5 @@
> +    uint32_t channelId = mHttpRetargetChannelParent->GetChannelId();
> +    mContentParent->AddHttpRetargetChannel(channelId,
> +                                           mHttpRetargetChannelParent);
> +
> +    if (mContentParent->GetMustCallAsyncOpen(channelId)) {

Would it be possible to put this in HttpChannelParent? It can call out internally to nsHttpChannel. It feels like this is a function of HttpChannelParent rather than ContentParent.

@@ +130,5 @@
> +  : mChannelId(0)
> +  , mBackgroundThread(nullptr)
> +  , mIPCClosed(false)
> +{
> +  AssertIsInMainProcess();

This should be on the background thread as well, right?

::: netwerk/ipc/PHttpRetargetChannel.ipdl
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +async protocol PHttpRetargetChannel

nit: Any reason why not PBackgroundHttpChannel or PHttpChannelBackground? I think I prefer something in the name to link it to PBackground.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +227,4 @@
>      , mConcurentCacheAccess(0)
>      , mIsPartialRequest(0)
>      , mHasAutoRedirectVetoNotifier(0)
> +    , mRetargetableProgressSink(nullptr)

Should already be init'd to nullptr since it's an nsCOMPtr.

@@ +973,5 @@
>      }
>  
> +    // cache the progress sink so we don't have to query for it each time.
> +    if (!mProgressSink)
> +        GetCallback(mProgressSink);

I *think* it should be ok to put this here, but why not just put it in OnDataAvailable?

if (!mRetargetableProgressSink) {
   GetCallback(mRetargetableProgressSink);
}

GetCallback should be doing a do_QI anyway, so may as well let it.

@@ -5431,5 @@
>  {
> -    MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread only");
> -    // cache the progress sink so we don't have to query for it each time.
> -    if (!mProgressSink)
> -        GetCallback(mProgressSink);

OnTransportStatus is called by more than OnDataAvailable. For now it might be ok to dispatch it make sure it's on the background thread or else dispatch it there. However, it might be a good idea to make sure it's called on PBackground. But we can come back to that.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1706,5 @@
>      if (IsNeckoChild()) {
> +        uint32_t channelID = GenerateChannelID();
> +        HttpRetargetChannelChild* httpRetargetChannel =
> +            new HttpRetargetChannelChild(channelID);
> +        httpChannel = new HttpChannelChild(channelID);

Add a comment:
// Create IPDL actors for main and background thread IPDL protocols.

@@ +1711,5 @@
> +
> +        // IPDL now owns this object (more specifically, the actor in the parent
> +        // process is now created and the connection between the two actors is
> +        // now established)
> +        rv = httpRetargetChannel->Init(static_cast<HttpChannelChild*>(httpChannel.get()));

Rather than static_cast, declare httpChannelChild above, and then set httpChannel = httpChannelChild before leaving the if branch.

::: parser/html/nsHtml5StreamParser.h
@@ +499,2 @@
>      nsCOMPtr<nsIRunnable>         mExecutorFlusher;
>      

Looks like no changes in this file. You can remove it from the patch please.

Comment 11

[atifrea]

Attachment: Bug 1015466 - Patch 1 v4: Retarget the messages to the background thread.

This version of the patch should be the final one as far as the parent is concerned. Redirecting works fine now and I have also fixed the nits suggested to my previous patch.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8482970 [details] [diff] [review]
Bug 1015466 - Patch 1 v4: Retarget the messages to the background thread.

Review of attachment 8482970 [details] [diff] [review]:
-----------------------------------------------------------------

Very good. I think we're in the cleanup stage for this patch now, assuming that tests are passing.

My comments are mostly nits, some queries about possible nsRefPtr usage (speak to mrbkap to get his thoughts) and then some thoughts about OnTransportStatus on the main thread.

Good work; keep it up!

::: dom/ipc/ContentParent.cpp
@@ +3881,5 @@
> +PHttpBackgroundChannelParent*
> +ContentParent::GetHttpBackgroundChannel(uint32_t aKey)
> +{
> +  PHttpBackgroundChannelParent* ret = mHttpBackgroundChannels.Get(aKey);
> +  mHttpBackgroundChannels.Remove(aKey);

nit: Since you're removing these, should the function be RemoveHttpBackgroundChannel? Or Take~?

::: dom/ipc/ContentParent.h
@@ +288,5 @@
> +    virtual void AddHttpChannel(uint32_t aKey, mozilla::net::PHttpChannelParent* aData);
> +
> +    // Returns the `PHttpChannelParent` reference stored in the
> +    // hashtable and implicitly deletes it from the hashtable afterwards
> +    virtual mozilla::net::PHttpChannelParent* GetHttpChannel(uint32_t aKey);

Do these need to be virtual?

@@ +299,5 @@
> +      mMustCallAsyncOpen.RemoveEntry(aChannelId);
> +    }
> +
> +    bool GetMustCallAsyncOpen(uint32_t aChannelId) {
> +      return mMustCallAsyncOpen.GetEntry(aChannelId) ? true : false;

Maybe inline these functions? MOZ_ALWAYS_INLINE - they're pretty small.

@@ +718,5 @@
>      bool mCalledKillHard;
>  
> +    // If | mMustCallAsyncOpen | is true, then AddToHashtableRunnable::Run()
> +    // needs to call AsyncOpen for a given http channel
> +    nsTHashtable<nsUint32HashKey> mMustCallAsyncOpen;

nit: Add "Used in conjunction with HTTP channel hash-tables: mHttpChannels and mHttpBackgroundChannels."

Any reason not to put this beside those member vars?

::: ipc/glue/BackgroundChildImpl.cpp
@@ +35,5 @@
> +void
> +AssertIsInChildProcess()
> +{
> +  MOZ_ASSERT(IsChildProcess());
> +}

Inline these small functions too?

::: netwerk/ipc/HttpBackgroundChannelChild.cpp
@@ +45,5 @@
> +  LOG(("HttpBackgroundChannelChild::Init [this=%p httpChannel=%p channelId=%d]\n",
> +        this,aHttpChannel,mChannelId));
> +
> +  PBackgroundChild* backgroundChild =
> +    mozilla::ipc::BackgroundChild::GetForCurrentThread();

mrbkap has probably looked at this already, but is PBackgroundChild guaranteed at this stage? You don't need to use GetOrCreateForCurrentThread?

@@ +131,5 @@
> +void
> +HttpBackgroundChannelChild::NotifyRedirect(PHttpChannelChild* newChannel)
> +{
> +  uint32_t newChannelId = static_cast<HttpChannelChild*>(newChannel)->GetChannelId();
> +  LOG(("HttpBackgroundChannelChild::Redirect [this=%p,oldHttpChannel=%p,newHttpChannel=%p,oldChannelId=%d,newChannelId=%d]\n",

nit: Long line. Split up the format string into two lines, please.

::: netwerk/ipc/HttpBackgroundChannelChild.h
@@ +14,5 @@
> +namespace net {
> +
> +class PHttpChannelChild;
> +
> +class HttpBackgroundChannelChild MOZ_FINAL :

Comment for this class please.

@@ +51,5 @@
> +  virtual bool RecvOnStatusBackground(const nsresult& aStatus) MOZ_OVERRIDE;
> +
> +  virtual bool RecvOnStopRequestBackground(const nsresult& aStatusCode) MOZ_OVERRIDE;
> +
> +  virtual void NotifyRedirect(mozilla::net::PHttpChannelChild* newChannel);

This is not a Recv~ function, so please add a short comment.

::: netwerk/ipc/HttpBackgroundChannelParent.cpp
@@ +114,5 @@
> +  const nsCString                 mSecurityInfoSerialization;
> +  const NetAddr                   mSelfAddr;
> +  const NetAddr                   mPeerAddr;
> +  const int16_t                   mRedirectCount;
> +  HttpBackgroundChannelParent*      mHttpBackgroundChannelParent;

These don't need to be ref ptrs? Check with mrbkap.

::: netwerk/ipc/HttpBackgroundChannelParent.h
@@ +13,5 @@
> +namespace mozilla {
> +namespace net {
> +
> +class HttpBackgroundChannelParent MOZ_FINAL :
> +  public PHttpBackgroundChannelParent

Comment for this class please.

@@ +23,5 @@
> +  virtual void ActorDestroy(ActorDestroyReason aWhy);
> +
> +  virtual bool Init(uint32_t aChannelId);
> +
> +  virtual bool ProcessOnStartRequestBackground(const nsresult& channelStatus,

Do these functions need to be virtual? Please check all in this class.

::: netwerk/ipc/NeckoChannelParams.ipdlh
@@ +52,5 @@
> +  // `channelID` is an identifier for each http channel that is created.
> +  // Do not mistake this channelId for the one used when redirecting; they
> +  // serve different purposes and there is no connection between them
> +  // whatsoever.
> +  uint32_t                    channelId;

Yup - they've been warned....!

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +341,5 @@
>    // A flag that should be false only if a cross-domain redirect occurred
>    uint32_t                          mAllRedirectsSameOrigin     : 1;
>  
> +  // The ID of the HTTP channel
> +  uint32_t                          mChannelId;

// ... for IPDL purposes.

Right?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +845,4 @@
>  
>    mRedirectChannelChild = do_QueryInterface(newChannel);
>    if (mRedirectChannelChild) {
> +    static_cast<HttpChannelChild*>(mRedirectChannelChild.get())->ConnectParent(newChannelId);

Do you need this cast?

@@ +948,5 @@
> +    static_cast<HttpChannelChild*>(mRedirectChannelChild.get());
> +  httpChannelChild->SetHttpBackgroundChannel(mHttpBackgroundChannel);
> +  static_cast<HttpBackgroundChannelChild*>(mHttpBackgroundChannel)->
> +    NotifyRedirect(httpChannelChild);
> +  mHttpBackgroundChannel = nullptr;

Add a comment: // Transferring reference of mHttpBackgroundChannel to mRedirectChannelChild.

@@ +1242,5 @@
> +  // Create the `HttpBackgroundChannelChild` object.
> +  mHttpBackgroundChannel = new HttpBackgroundChannelChild();
> +  // IPDL now owns this object (more specifically, the actor in the parent
> +  // process is now created and the connection between the two actors is
> +  // now established)

I'm concerned about this. If the parent goes away, the child will also be deleted, but HttpChannelChild might still have a ref to it. Is this guaranteed to never happen?

This relates to my other comments about ref ptrs, of course.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +30,5 @@
>  #include "nsIDivertableChannel.h"
>  #include "mozilla/net/DNS.h"
> +#include "mozilla/ipc/PBackgroundChild.h"
> +#include "mozilla/net/PHttpBackgroundChannelChild.h"
> +#include "mozilla/net/HttpBackgroundChannelChild.h"

Do you need all three of these includes?

@@ +177,5 @@
>  
> +  // The corresponding `HttpBackgroundChannelChild`. It is created during
> +  // `HttpChannelChild::AsyncOpen` and is transfered from the old channel over
> +  // to the new one when redirecting (in `HttpChannelChild::Redirect3Complete`).
> +  HttpBackgroundChannelChild* mHttpBackgroundChannel;

Ref ptr?

@@ +179,5 @@
> +  // `HttpChannelChild::AsyncOpen` and is transfered from the old channel over
> +  // to the new one when redirecting (in `HttpChannelChild::Redirect3Complete`).
> +  HttpBackgroundChannelChild* mHttpBackgroundChannel;
> +
> +  uint32_t mChannelId;

HttpBaseChannel already has this field, no?

@@ +219,1 @@
>    void DeleteSelf();

Looks like nothing changed here.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +32,5 @@
> +#include "mozilla/net/HttpBackgroundChannelParent.h"
> +#include "mozilla/dom/ContentParent.h"
> +#include "mozilla/ipc/BackgroundParent.h"
> +
> +#include <ctime>

nit: Can you pull these timing changes into a separate patch please? It will be great to have these measurements, but it will also be great to be able to back them out quickly. Or even just have a reference for where the references should be deleted.

But, not high priority. Only if you have time.

@@ +109,5 @@
>    {
>      const HttpChannelConnectArgs& cArgs = aArgs.get_HttpChannelConnectArgs();
> +    const uint32_t newChannelId = cArgs.newHttpChannelId();
> +    mChannelID = newChannelId;
> +    LOG(("HttpChannelParent::Init REDIRECTING [newChannelId=%d]\n", newChannelId));

Nice log statements. Thanks.

@@ +664,4 @@
>  
>    uint16_t redirectCount = 0;
>    mChannel->GetRedirectCount(&redirectCount);
> +  LOG(("HttpChannelParent::I have sent OnStartRequest to the child: [this=%p, channelId=%d]\n",this,mChannelID));

No you haven't. You're going to, but not until the end of the function. Right?

@@ +671,5 @@
> +    do_QueryInterface(aRequest, &rv);
> +  nsCOMPtr<nsIThread> backgroundThread = static_cast<HttpBackgroundChannelParent*>(
> +      mHttpBackgroundChannel)->GetBackgroundThread();
> +  if (threadRetargetableRequest) {
> +    rv = threadRetargetableRequest->RetargetDeliveryTo(backgroundThread);

Move backgroundThread = GetBackgroundThread() into the if branch. Then you won't need to null it out after.

@@ +677,5 @@
> +
> +  backgroundThread = nullptr;
> +
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to retarget data delivery to the background thread.");

I think we can MOZ_ASSERT(NS_SUCCEEDED(rv)) here - Retarget should always succeed right?

Also, you can add a similar MOZ_ASSERT for getting threadRetargetableRequest:
MOZ_ASSERT(NS_SUCCEEDED(rv) && threadRetargetableRequest);

Can we assert backgroundThread as well?

@@ +685,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +
> +  // Send the message to the child via `HttpBackgroundChannel`.
> +  if (!mHttpBackgroundChannel ||
> +        !static_cast<HttpBackgroundChannelParent*>(mHttpBackgroundChannel)->

Looks like you do this static_cast a few times. Maybe better to have the var as an HttpBackgroundChannelParent in the class definition?

@@ +695,5 @@
> +                                          mCacheEntry ? true : false,
> +                                          expirationTime, cachedCharset, secInfoSerialization,
> +                                          mChannel->GetSelfAddr(), mChannel->GetPeerAddr(),
> +                                          redirectCount))
> +    return NS_ERROR_UNEXPECTED;

Speak to mrbkap - can we crash here or assume unreachable?
Maybe we can assert mHttpBackgroundChannel at least...?

@@ +716,1 @@
>      return NS_ERROR_UNEXPECTED;

nit: Braces please. Check other ifs.

@@ +763,5 @@
> +                                                              mStoredProgress,
> +                                                              mStoredProgressMax,
> +                                                              data,
> +                                                              aOffset,
> +                                                              aCount)) {

nit: indentation.

@@ +1124,5 @@
> +HttpChannelParent::CheckListenerChain()
> +{
> +    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread!");
> +    nsresult rv = NS_OK;
> +    return rv;

You can just return NS_OK; No need for the nsresult local.
And MOZ_ASSERT - let's crash if we're not on the main thread.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +20,5 @@
> +#include "mozilla/net/PHttpBackgroundChannelParent.h"
> +#include "nsIThreadRetargetableStreamListener.h"
> +#include "nsPIThreadRetargetableProgressSink.h"
> +
> +#include <ctime>

Needed?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5222,4 @@
>  
>      ReleaseListeners();
>  
> +    mRetargetableProgressSink = nullptr;

Should this be in ReleaseListeners? It feels like a listener object to me. What do you think?

@@ +5443,5 @@
> +    if (!mozilla::ipc::IsOnBackgroundThread()) {
> +     //XXX: I am not sure I understand what needs to be done here. From
> +     //sworkman's review I understand that OnTransportStatus may sometimes be
> +     //called by somewhere other than OnDataAvailable. What should be done in
> +     //this case?   

Hmm. Yeah, I'm unsure how to proceed here. OnTransportStatus can be called from the socket, in which case it will be on the main thread. But it looks like you allow OnProgress and OnStatus to be called on the main thread and sent using PHttpChannel.

What I think you can do is this: The first part of the function we should do on the main thread, i.e. getting the self and peer addresses. So, if (status ...) { MOZ_ASSERT(NS_IsMainThread()); These particular statuses should be coming from the socket, so they should be on the main thread.
The second part of the function - well, you could dispatch these to the background thread if mRetargetableProgressSink. Or you could leave that to the progressSink's OnProgress and OnStatus functions. Note: I'm not sure why you allow them to run on the main thread - or rather, why you allow them to be sent over PHttpChannel and not the background one. Come and talk to me about this one if you think there's a problem.

Comment 13

[Jason Duell]

See bug 1062713 comment 10 for interesting performance numbers that are relevant here.  We're actually not doing so badly with the existing main-thread e10s (OTOH doing off-main might make it even better.)

(There are a couple of wrong things I say in that comment: 1) the different is not just IPDL: we're also doing off-main OnDataAvailable in single-process for HTML/Images.  And it's still slower than e10s which isn't doing that. 2) the on-modify-request/on-examine-response callbacks are actually called when we're on the main thread anyway, so they shouldn't be adding additional main thread latency per se.).

I've honestly been puzzled by those results, but they've seemed consistent (at least for youtube.com).  The only thing that seems to explain it is a shorter average main thread event loop latency when we're using e10s.

Comment 14

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

Comment on attachment 8482970 [details] [diff] [review]
Bug 1015466 - Patch 1 v4: Retarget the messages to the background thread.

Review of attachment 8482970 [details] [diff] [review]:
-----------------------------------------------------------------

I'm doing some nit pick, build script clean up, refactory, and split patch in to smaller logical pieces. Stay tuned!

::: dom/ipc/ContentParent.cpp
@@ +3881,5 @@
> +PHttpBackgroundChannelParent*
> +ContentParent::GetHttpBackgroundChannel(uint32_t aKey)
> +{
> +  PHttpBackgroundChannelParent* ret = mHttpBackgroundChannels.Get(aKey);
> +  mHttpBackgroundChannels.Remove(aKey);

Rename to |TakeHttpBackgroundChannel|.

::: dom/ipc/ContentParent.h
@@ +288,5 @@
> +    virtual void AddHttpChannel(uint32_t aKey, mozilla::net::PHttpChannelParent* aData);
> +
> +    // Returns the `PHttpChannelParent` reference stored in the
> +    // hashtable and implicitly deletes it from the hashtable afterwards
> +    virtual mozilla::net::PHttpChannelParent* GetHttpChannel(uint32_t aKey);

Yeah it's not necessary to be virtual.

@@ +299,5 @@
> +      mMustCallAsyncOpen.RemoveEntry(aChannelId);
> +    }
> +
> +    bool GetMustCallAsyncOpen(uint32_t aChannelId) {
> +      return mMustCallAsyncOpen.GetEntry(aChannelId) ? true : false;

Done.

@@ +718,5 @@
>      bool mCalledKillHard;
>  
> +    // If | mMustCallAsyncOpen | is true, then AddToHashtableRunnable::Run()
> +    // needs to call AsyncOpen for a given http channel
> +    nsTHashtable<nsUint32HashKey> mMustCallAsyncOpen;

Done. Comment added and move down beside those member vars.

::: dom/ipc/moz.build
@@ +100,4 @@
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
> +    '../../netwerk/ipc',

This line is removed because there is no need to include header files under this directory after fine tuning the type declaration in ContentParent.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +35,5 @@
> +void
> +AssertIsInChildProcess()
> +{
> +  MOZ_ASSERT(IsChildProcess());
> +}

Extracting a single line assertion doesn't make the code simpler. Will use MOZ_ASSERT directly.

::: netwerk/ipc/HttpBackgroundChannelChild.cpp
@@ +45,5 @@
> +  LOG(("HttpBackgroundChannelChild::Init [this=%p httpChannel=%p channelId=%d]\n",
> +        this,aHttpChannel,mChannelId));
> +
> +  PBackgroundChild* backgroundChild =
> +    mozilla::ipc::BackgroundChild::GetForCurrentThread();

Yes, we should use |GetOrCreateForCurrentThread| here and make the initialization asynchronous.

@@ +131,5 @@
> +void
> +HttpBackgroundChannelChild::NotifyRedirect(PHttpChannelChild* newChannel)
> +{
> +  uint32_t newChannelId = static_cast<HttpChannelChild*>(newChannel)->GetChannelId();
> +  LOG(("HttpBackgroundChannelChild::Redirect [this=%p,oldHttpChannel=%p,newHttpChannel=%p,oldChannelId=%d,newChannelId=%d]\n",

Done.

::: netwerk/ipc/HttpBackgroundChannelChild.h
@@ +51,5 @@
> +  virtual bool RecvOnStatusBackground(const nsresult& aStatus) MOZ_OVERRIDE;
> +
> +  virtual bool RecvOnStopRequestBackground(const nsresult& aStatusCode) MOZ_OVERRIDE;
> +
> +  virtual void NotifyRedirect(mozilla::net::PHttpChannelChild* newChannel);

This function is not necessary to be a virtual function. Remove virtual function specifier and add comment.

@@ +56,5 @@
> +
> +  uint32_t GetChannelId() { return mChannelId; }
> +
> +private:
> +  uint32_t mChannelId;

We don't need |mChannelId| because it can be derived from |mHttpChannel|.

::: netwerk/ipc/HttpBackgroundChannelParent.h
@@ +23,5 @@
> +  virtual void ActorDestroy(ActorDestroyReason aWhy);
> +
> +  virtual bool Init(uint32_t aChannelId);
> +
> +  virtual bool ProcessOnStartRequestBackground(const nsresult& channelStatus,

Done. remove virtual function specifier.

::: netwerk/ipc/moz.build
@@ +24,5 @@
>  
> +SOURCES += [
> +    'HttpBackgroundChannelChild.cpp',
> +    'HttpBackgroundChannelParent.cpp',
> +]

Can be put into UNIFIED_SOURCES after cleaning up the namespace.

@@ +53,4 @@
>  FINAL_LIBRARY = 'xul'
>  
>  LOCAL_INCLUDES += [
> +    '../../dom/ipc',

This can be removed.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +341,5 @@
>    // A flag that should be false only if a cross-domain redirect occurred
>    uint32_t                          mAllRedirectsSameOrigin     : 1;
>  
> +  // The ID of the HTTP channel
> +  uint32_t                          mChannelId;

ChannelId is only used in e10s situation. I'll suggest to remove it from HttpBaseChannel.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +845,4 @@
>  
>    mRedirectChannelChild = do_QueryInterface(newChannel);
>    if (mRedirectChannelChild) {
> +    static_cast<HttpChannelChild*>(mRedirectChannelChild.get())->ConnectParent(newChannelId);

No, this can be removed.

@@ +948,5 @@
> +    static_cast<HttpChannelChild*>(mRedirectChannelChild.get());
> +  httpChannelChild->SetHttpBackgroundChannel(mHttpBackgroundChannel);
> +  static_cast<HttpBackgroundChannelChild*>(mHttpBackgroundChannel)->
> +    NotifyRedirect(httpChannelChild);
> +  mHttpBackgroundChannel = nullptr;

Move into a method for better understanding.

@@ +1242,5 @@
> +  // Create the `HttpBackgroundChannelChild` object.
> +  mHttpBackgroundChannel = new HttpBackgroundChannelChild();
> +  // IPDL now owns this object (more specifically, the actor in the parent
> +  // process is now created and the connection between the two actors is
> +  // now established)

I'm doing some refactory to make sure all the references have correct lifecycle.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +30,5 @@
>  #include "nsIDivertableChannel.h"
>  #include "mozilla/net/DNS.h"
> +#include "mozilla/ipc/PBackgroundChild.h"
> +#include "mozilla/net/PHttpBackgroundChannelChild.h"
> +#include "mozilla/net/HttpBackgroundChannelChild.h"

Removed.

@@ +179,5 @@
> +  // `HttpChannelChild::AsyncOpen` and is transfered from the old channel over
> +  // to the new one when redirecting (in `HttpChannelChild::Redirect3Complete`).
> +  HttpBackgroundChannelChild* mHttpBackgroundChannel;
> +
> +  uint32_t mChannelId;

Yep and this can be removed. Getter and Setter function are also moved to HttpBaseChannel.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +664,4 @@
>  
>    uint16_t redirectCount = 0;
>    mChannel->GetRedirectCount(&redirectCount);
> +  LOG(("HttpChannelParent::I have sent OnStartRequest to the child: [this=%p, channelId=%d]\n",this,mChannelID));

Move this log statement to the end of this function.

@@ +671,5 @@
> +    do_QueryInterface(aRequest, &rv);
> +  nsCOMPtr<nsIThread> backgroundThread = static_cast<HttpBackgroundChannelParent*>(
> +      mHttpBackgroundChannel)->GetBackgroundThread();
> +  if (threadRetargetableRequest) {
> +    rv = threadRetargetableRequest->RetargetDeliveryTo(backgroundThread);

Done.

@@ +677,5 @@
> +
> +  backgroundThread = nullptr;
> +
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to retarget data delivery to the background thread.");

Yes, retarget should always success in this case. Otherwise we'll have to main both main thread IPC and background thread IPC in the same code base, which will be lots of pain.

@@ +685,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +
> +  // Send the message to the child via `HttpBackgroundChannel`.
> +  if (!mHttpBackgroundChannel ||
> +        !static_cast<HttpBackgroundChannelParent*>(mHttpBackgroundChannel)->

Yes and will do.

@@ +695,5 @@
> +                                          mCacheEntry ? true : false,
> +                                          expirationTime, cachedCharset, secInfoSerialization,
> +                                          mChannel->GetSelfAddr(), mChannel->GetPeerAddr(),
> +                                          redirectCount))
> +    return NS_ERROR_UNEXPECTED;

Assert mHttpBackgroundChannel should be enough.

@@ +801,5 @@
> +    // parent, then send the message to the child via `HttpBackgroundChannel`.
> +    if (IsOnBackgroundThread() && (!mHttpBackgroundChannel ||
> +                                   !mHttpBackgroundChannel->
> +                                     SendOnProgressBackground(aProgress,
> +                                                              aProgressMax)))

Rearrange this part to make it clear.

@@ +1124,5 @@
> +HttpChannelParent::CheckListenerChain()
> +{
> +    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread!");
> +    nsresult rv = NS_OK;
> +    return rv;

Done.

::: netwerk/protocol/http/moz.build
@@ +105,4 @@
>  
>  LOCAL_INCLUDES += [
>      '../../base/src',
> +    '../../ipc',

This is not necessary. Removed.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5222,4 @@
>  
>      ReleaseListeners();
>  
> +    mRetargetableProgressSink = nullptr;

ReleaseListeners() only handles the listener in HttpBaseChannel. In current patch mRetargetableProgressSink is only existed in nsHttpChannel, therefore, we cannot move this line to ReleaseListeners().

@@ +5443,5 @@
> +    if (!mozilla::ipc::IsOnBackgroundThread()) {
> +     //XXX: I am not sure I understand what needs to be done here. From
> +     //sworkman's review I understand that OnTransportStatus may sometimes be
> +     //called by somewhere other than OnDataAvailable. What should be done in
> +     //this case?   

Yes progressSink for IPC case should only run on background thread in this patch because we're abandon main thread IPC for HTTP.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1707,5 @@
> +        // redirect, then the IPDL actor for the background thread will be
> +        // created in `HttpChannelChild::AsyncOpen`, otherwise the already
> +        // existing PHttpBackgroundChannel actors will be used both in the parent
> +        // and the child.
> +        uint32_t channelID = GenerateChannelID();

ID generation in child process cannot guarantee the uniqueness. You'll need to do it in the parent side.

Comment 15

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

Attachment: Part 1 - PHttpBackgroundChannel

Create PHttpBackgroundChannel.ipdl on necko thread in content process.

Comment 16

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

Attachment: Part 1 - PHttpBackgroundChannel

Skeleton of PHttpBackgroundChannel.

Comment 17

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

Attachment: Part 2 - OMT HttpChannel via PHttpChannelBackground

Comment 18

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

Patches are compiled but not tested yet, Include following handling:

1. PHttpBackgroundChannel connects Necko thread on content process and Background thread on chrome process, delivers OnStartRequest/OnStopRequest/OnTransportAndData/OnProgress/OnStatus events.
2. HttpChannelParent retargets OnDataAvailable to Background thread.
3. HttpChannelChild allows OnTransportsAndData to be retargeted to other thread.
4. PHttpBackgroundChannel is guaranteed to be created after the creation of PHttpChannel.

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8516990 [details] [diff] [review]
Part 1 - PHttpBackgroundChannel

Review of attachment 8516990 [details] [diff] [review]:
-----------------------------------------------------------------

Some comments; basically looks good. f+

::: ipc/glue/BackgroundParentImpl.cpp
@@ +243,5 @@
> +  MOZ_ASSERT(aActor);
> +
> +  HttpBackgroundChannelParent* p =
> +    static_cast<HttpBackgroundChannelParent*>(aActor);
> +  p->Release();

nit: We shouldn't use Release directly, right?

nsRefPtr<HttpBackground...> = dont_AddRef(...);

Same for BackgroundChildImpl::DeallocPHttpBackgroundChannelChild

::: netwerk/ipc/HttpBackgroundChannelChild.h
@@ +83,5 @@
> +  nsRefPtr<HttpChannelChild> mHttpChannel;
> +  nsCOMPtr<nsIEventTarget> mTargetThread;
> +
> +  nsCOMArray<nsIRunnable> mPendingRunnables;
> +  bool mPending;

Comments for these member vars please. Same for other classes.

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8516992 [details] [diff] [review]
Part 2 - OMT HttpChannel via PHttpChannelBackground

Review of attachment 8516992 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. f+

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1562,5 @@
>    }
>  
> +  // Create background channel.
> +  mHttpBackgroundChannel = new HttpBackgroundChannelChild();
> +  MOZ_ASSERT(mHttpBackgroundChannel);

You shouldn't need to do this; new is infallible.

@@ +1571,1 @@
>    return NS_OK;

return rv;

@@ +1575,5 @@
> +HttpChannelChild::TransferBackgroundChannel(nsIHttpChannelChild* aTarget)
> +{
> +  aTarget->SetBackgroundChannel(mHttpBackgroundChannel);
> +  mHttpBackgroundChannel = nullptr;
> +  mHttpBackgroundChannel->NotifyRedirect(static_cast<HttpChannelChild*>(aTarget));

Function call after null?

Comment 21

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

Comment on attachment 8516992 [details] [diff] [review]
Part 2 - OMT HttpChannel via PHttpChannelBackground

Review of attachment 8516992 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +704,5 @@
> +  MOZ_ASSERT(threadRetargetableRequest);
> +
> +  nsCOMPtr<nsIThread> backgroundThread = mHttpBackgroundChannel->GetBackgroundThread();
> +  nsresult rv = threadRetargetableRequest->RetargetDeliveryTo(backgroundThread);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

Retargeting in parent side might be failed [1] because nsHttpChannel is closed before OnStartRequest being invoked. Therefore, we need to handle OnDataRequest on both main thread and background thread correctly.

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?from=nsHttpChannel.cpp&case=true#5703

Comment 22

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

Attachment: Part 1 - PHttpBackgroundChannel

Allow |ProcessOnTransportAndData| to be invoked on main thread because of comment #21.

Comment 23

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

Attachment: Part 2 - OMT HttpChannel via PHttpChannelBackground

Allow retargeting failure because of comment #21.

Comment 24

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

After applying patch for bug 1102439 and fix for comment #21, all test cases under netwerk/unit_ipc/ are passed except for test_synthesized_response_wrap.js.

In test_synthesized_response_wrap.js we'll hit the assertion at [1]. The root cause is that HttpChannelParent::OnDataAvailable is not preceded by OnStatus and OnProgress [2] after OMT. I'm still thinking about how to resolve this issue.

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp?from=HttpChannelChild.cpp&case=true#499

[2] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp?from=HttpChannelParent.cpp#761

Comment 25

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

Actually, HttpChannelParent::OnStatus and OnProgress will always be preceded by OnDataAvailable because the task is submitted back to main thread [1].

Making HttpChannelChild skip DoOnStatus and DoOnProgress [2] if retargeting is configured at parent-side can avoid hitting that assertion. However test case still failed because one of the onprogress event is not received at expected timing [3]. Need to check if the onprogress is missing or is just delayed.

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5636
[2] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp?from=HttpChannelChild.cpp&case=true#460
[3] http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_synthesized_response.js?from=test_synthesized_response.js&case=true#102

Comment 26

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

Ah, actually there are four test cases failed with current patch.

test_dns_cancel_wrap.js
test_reentrancy_wrap.js (crash)
test_synthesized_response_wrap.js (crash)
test_redirect-caching_passing_wrap.js

And one more fail after applying patch for comment #25, but test_synthesized_response_wrap.js is not crashed anymore.

test_progress_wrap.js

Comment 27

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

I use http://people.mozilla.org/~schien/omt-perf.html to do the performance measurement, which will execute a time-consuming JS while loading a huge PNG file. I found image repainting is still blocked by JS even though the OnDataAvailable events are all delivered to imgRequest before the JS is finished from log. This means image rendering pipeline can still be stuck by JS with HttpChannel OMT, however it should be an independent bug for people to follow-up.

Comment 28

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

Bug 1109332 is filed for comment #27.

Comment 29

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

Attachment: Part 1 - PHttpBackgroundChannel, v1

I decide to separate chrome process OMT and content process OMT into different patch files. Doing chrome process OMT is safer so that I can start the review cycle in parallel while debugging content process OMT.

Comment 30

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

Attachment: Part 2 - OMT HttpChannel via PHttpChannelBackground, v1

Comment 31

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

netwerk/test/unit_ipc/test_app_offline_http.js is failed while applying these patches. I found it is because this test case executes all test steps at once instead of in sequence, therefore, |do_test_finished| is invoked earlier than all test steps are finished. The test case is passed if I delay the |do_test_finished| for 5s. I'm trying to rewrite this test case to make it execute as our expectation.

Comment 32

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

dom/base/test/test_xhr_progressevents.html is also affected by the change of message sequence in parent process. In this test case it'll check if the length of XHR response is matched with latest onprogress event. However, this no longer holds true because |OnDataAvailable| and |OnProgress| is not synchronized after PBackground-ify.

Comment 33

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #32)
> dom/base/test/test_xhr_progressevents.html is also affected by the change of
> message sequence in parent process. In this test case it'll check if the
> length of XHR response is matched with latest onprogress event. However,
> this no longer holds true because |OnDataAvailable| and |OnProgress| is not
> synchronized after PBackground-ify.
I dug into our implementation of XHR and found it depends on the original event sequence (OnProgress first) [1]. This is definitely something we want to fix.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#3606

Comment 34

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8535151 [details] [diff] [review]
Part 1 - PHttpBackgroundChannel, v1

Review of attachment 8535151 [details] [diff] [review]:
-----------------------------------------------------------------

First patch looks sane to me.

Comment 35

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8535152 [details] [diff] [review]
Part 2 - OMT HttpChannel via PHttpChannelBackground, v1

Review of attachment 8535152 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just marking r- because of the questions you mentioned offline to do with OnProgress and XHR. Otherwise, looks good.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +24,5 @@
>  #include "nsSerializationHelper.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ipc/InputStreamUtils.h"
>  #include "mozilla/ipc/URIUtils.h"
> +//#include "mozilla/ipc/BackgroundChild.h"

Remove this line if it's not needed.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +808,5 @@
>  // HttpChannelParent::nsIProgressEventSink
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
>  HttpChannelParent::OnProgress(nsIRequest *aRequest,

We were talking about OnProgress offline - I can't remember the reason for not calling OnProgress and OnStatus on the background thread. I remember that in an earlier patch, Alex had added a new IDL to verify that nsHttpChannel::mProgressSink was HttpChannelParent - can we do that to keep the ordering, using nsIHttpChannelParent? I remember that keeping the code simple was one reason - was there another?

::: netwerk/protocol/http/HttpChannelParentListener.cpp
@@ +216,5 @@
>  
>    if (succeeded) {
> +    // Create the link between the newly created `HttpChannelParent` and the
> +    // `HttpBackgroundChannelParent` that belonged to the old `HttpChannelParent`.
> +    HttpChannelParent* originalChannelParent = static_cast<HttpChannelParent*>(mNextListener.get());

You could change this so that originalChannelParent is an nsCOMPtr<nsIHttpChannelParent> and do a do_QueryInterface. It's a little more overhead, but it means you can check that it's the right class.

You'll need to add TransferBackgroundChannel to nsIHttpChannelParent.

::: netwerk/test/unit/test_progress.js
@@ -51,5 @@
>      this._listener.onStartRequest(request, context);
>    },
>  
>    onDataAvailable: function(request, context, data, offset, count) {
> -    do_check_eq(this._last_callback_handled, TYPE_ONPROGRESS);

So I guess you might need to rethink this based on the XHR assumption that OnProgress comes before OnDataAvailable.

Comment 36

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

Comment on attachment 8535152 [details] [diff] [review]
Part 2 - OMT HttpChannel via PHttpChannelBackground, v1

Review of attachment 8535152 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +24,5 @@
>  #include "nsSerializationHelper.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ipc/InputStreamUtils.h"
>  #include "mozilla/ipc/URIUtils.h"
> +//#include "mozilla/ipc/BackgroundChild.h"

Removed.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +808,5 @@
>  // HttpChannelParent::nsIProgressEventSink
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
>  HttpChannelParent::OnProgress(nsIRequest *aRequest,

Another reason is several attributes in nsHttpChannel is not designed for multi-thread accessing, e.g. mURI. Making another "OnBackgroundProgress" might help keeping the ordering but will definitely make nsHttpChannel difficult to maintain.

::: netwerk/protocol/http/HttpChannelParentListener.cpp
@@ +216,5 @@
>  
>    if (succeeded) {
> +    // Create the link between the newly created `HttpChannelParent` and the
> +    // `HttpBackgroundChannelParent` that belonged to the old `HttpChannelParent`.
> +    HttpChannelParent* originalChannelParent = static_cast<HttpChannelParent*>(mNextListener.get());

Will do.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is there a new patch coming?

Comment 38

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #37)
> Is there a new patch coming?

Not recently no. Stuck in TV support now. :(
(trying to allocate a longer time slot on this bug)

Comment 39

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

Attachment: Part 1 - PHttpBackgroundChannel, v1.1

rebase to latest m-c.

Comment 40

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

Attachment: Part 2 - OMT HttpChannel via PHttpChannelBackground, v2

update according to review comment, rebase to latest m-c.

Comment 41

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

Attachment: xhr_onprogress_workaround.patch

temp workaround for the timing dependency mentioned in comment #33.

With this patch, test_xhr_progressevents.html can pass on both e10s and non-e10s environment.

Comment 42

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

My next revision will include the modification for handling channel diversion and I'd like to discuss the implementation first.

While triggering channel diversion, we'll 1) suspend HttpChannelChild and enqueue all incoming events and 2) suspend the nsHttpChannel in parent process. After diversion listener is hooked, we'll divert all the pending events in child process back to parent.

There are five IPC messages involved:
[parent] DivertOnDataAvailable, DivertOnStopRequest, DivertComplete
[child] FlushedForDiversion, DivertMessages

At minimal, we'll need to move FlushedForDiversion and DivertMessages to PHttpBackgroundChannel because the message sequence between "OnXXX" and these two are relevant.

@sworkman, do you think we also need to off-main-thread DivertOnDataAvailable, DivertOnStopRequest, DivertComplete, too?

Comment 43

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

There is one more thing we need to handle for channel diversion. The stream listener chain is replaced after OnStartRequest and the new listener chain might not be able to retarget.

I don't think we can call |RetargetDeliveryTo(mainThread)| again on the same request. So the only thing I can do is re-dispatch these OnDataAvailable events back to main thread after diversion.

Comment 44

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

Attachment: MozReview Request: bz://1015466/schien

/r/7269 - Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,bent.
/r/7271 - Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=sworkman.
/r/7273 - workaround timing dependency of OnProgress and OnDataAvailable
/r/7275 - Bug 1015466 - waiting for channel open event explicitly. r=jduell.

Pull down these commits:

hg pull -r 4122c5b6defe904167bc0445655d273ede767ae1 https://reviewboard-hg.mozilla.org/gecko/

Comment 45

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

Comment on attachment 8594573 [details]
MozReview Request: bz://1015466/schien

/r/7269 - Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,bent.
/r/7271 - Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=sworkman.
/r/7273 - workaround timing dependency of OnProgress and OnDataAvailable
/r/7275 - Bug 1015466 - waiting for channel open event explicitly. r=jduell.

Pull down these commits:

hg pull -r 4122c5b6defe904167bc0445655d273ede767ae1 https://reviewboard-hg.mozilla.org/gecko/

Comment 46

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

From current try results, there are two remaining issues: 1. xhr onprogress event, 2. memory leakage found on OS X 10.6 debug browser chrome test chunk-3. However, I would like some feedback for those code related to comment #42 and #43. (I couldn't find the f? setting on reviewboard so I use r? directly)

Comment 47

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #41)
> Created attachment 8590208 [details] [diff] [review]
> xhr_onprogress_workaround.patch
> 
> temp workaround for the timing dependency mentioned in comment #33.
> 
> With this patch, test_xhr_progressevents.html can pass on both e10s and
> non-e10s environment.

After double checking the test case and spec, the downloading progress needs to be synchronized with response body. In our current implementation, we update response body via OnDataAvailable and update downloading progress via OnProgress. Since response body can be accessed via XHR js object directly, it'll be easier to send progress event at OnDataAvailable, otherwise we'll need to delay the timing of updating response body.

Looks like it's difficult to make XHR right if OnDataAvailable arrives earlier than OnProgress. I'll try modify HttpBackgroundChannel to make the those events come in order.

Comment 48

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #42)
> My next revision will include the modification for handling channel
> diversion and I'd like to discuss the implementation first.
> 
> While triggering channel diversion, we'll 1) suspend HttpChannelChild and
> enqueue all incoming events and 2) suspend the nsHttpChannel in parent
> process. After diversion listener is hooked, we'll divert all the pending
> events in child process back to parent.

Sounds good.

> There are five IPC messages involved:
> [parent] DivertOnDataAvailable, DivertOnStopRequest, DivertComplete
> [child] FlushedForDiversion, DivertMessages
> 
> At minimal, we'll need to move FlushedForDiversion and DivertMessages to
> PHttpBackgroundChannel because the message sequence between "OnXXX" and
> these two are relevant.
> 
> @sworkman, do you think we also need to off-main-thread
> DivertOnDataAvailable, DivertOnStopRequest, DivertComplete, too?

I think it should be ok to leave them on the main thread. My concern is what happens once they get to the parent process and how to deal with RetargetDeliveryTo if it is called by the new listener on the parent. I *think* that the new listener will expect that all the OnDataAvailables will be called on the retargeted thread. But in your current patches, I think the diverted OnDataAvailables will be called on the main thread and the remaining OnDataAvailables will be called on the retargeted thread. It might be ok because we don't have many consumers of ChannelDiversion, but it would not be good if someone uses it in the future and expects a certain behavior.

Right now, a quick fix would be to always call RetargetDeliveryTo(mainThread) after OnStartRequest is called in StartDiversion. That way, you force nsHttpChannel to deliver the remaining OnDataAvailables to the main thread. If you do this, I'd want jduell to confirm that it's ok.

Comment 49

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #43)
> There is one more thing we need to handle for channel diversion. The stream
> listener chain is replaced after OnStartRequest and the new listener chain
> might not be able to retarget.
> 
> I don't think we can call |RetargetDeliveryTo(mainThread)| again on the same
> request. So the only thing I can do is re-dispatch these OnDataAvailable
> events back to main thread after diversion.

Oh - didn't read this completely before replying in comment #48. I think you can call Retarget twice - I don't remember a reason why that should be forbidden. Try it and see if it works :)

Comment 50

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

Attachment: Part 0, fix-progress-status-timing.patch

To address the OnProgress/OnDataAvailable sequencing issue mentioned in comment #47. The most straightforward solution I came out is dispatching OnTransportStatusAsyncEvent synchronously [1]. Because we forcing a context switch, their is a chance that nsHttpChannel.Cancel() is called before continuing the OnDataAvailable/OnTransportStatus handler. We'll have to double check the |mCanceled| flag again (few crash found on try server).

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5754

Comment 51

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

Comment on attachment 8598408 [details] [diff] [review]
Part 0, fix-progress-status-timing.patch

This patch looks sane on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d4e97ea9621

Comment 52

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

Comment on attachment 8594573 [details]
MozReview Request: bz://1015466/schien

/r/7269 - Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,bent.
/r/7271 - Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=sworkman.
/r/7273 - Bug 1015466 - waiting for channel open event explicitly. r=jduell.

Pull down these commits:

hg pull -r 4e0d46467bc0bf9792ec9e86f79e7971b278451b https://reviewboard-hg.mozilla.org/gecko/

Comment 53

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

I've fixed all the error found in try result, let's start the review process.

Comment 54

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8594573 [details]
MozReview Request: bz://1015466/schien

Thanks for sticking with this schien! I'd like khuey to review this PBackground stuff. Please let me know if you need any help!

Comment 55

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8594573 [details]
MozReview Request: bz://1015466/schien

https://reviewboard.mozilla.org/r/7267/#review7119

::: netwerk/protocol/http/nsHttpChannel.cpp
(Diff revision 2)
> -        return NS_OK;

You can get rid of the warning here, but I think you should still return NS_OK. No need to update the target if we're on the thread, right? Or are you calling from somewhere other that OnStartRequest?

::: netwerk/protocol/http/nsHttpChannel.cpp:5775
(Diff revision 2)
> -                                                progress, progressMax));
> +                NS_DISPATCH_SYNC);

Are you trying to ensure that HttpChannelParent::OnProgress is called on the main thread, in parent process, and sent to the child BEFORE HttpChannelParent::OnDataAvailable is called on the necko background thread in the parent process?

If so, I'm concerned that this patch will revert any benefit of processing ODA on the background thread. If ODA has to wait for OnProgress to be added to the main thread's event queue, wait until it's dequeued and then processed before ODA can complete. So, it's blocked by other events on the main thread again.

If I rememeber correctly, OnProgress has to be processed on the main thread in the parent for some reason - is that right?

But some of the use cases are ok with OnProgress being called asynchronously with OnDataAvailable. So, could you have XHR set a flag on the channel? And then check that flag in ODA - if it's set, use NS_DISPATCH_SYNC, else use NS_DISPATCH_NORMAL. What do you think?

Comment 56

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

https://reviewboard.mozilla.org/r/7267/#review8179

> You can get rid of the warning here, but I think you should still return NS_OK. No need to update the target if we're on the thread, right? Or are you calling from somewhere other that OnStartRequest?

For channel deverting, the |RetargetDeliveryTo(mainthread)| is called on mainthread. So we do need to remove this statement entirely. nsInputStreamPump is the only place knows previous targeting thread and already did the early return (https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsInputStreamPump.cpp#747).

Comment 57

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

https://reviewboard.mozilla.org/r/7267/#review8181

> Are you trying to ensure that HttpChannelParent::OnProgress is called on the main thread, in parent process, and sent to the child BEFORE HttpChannelParent::OnDataAvailable is called on the necko background thread in the parent process?
> 
> If so, I'm concerned that this patch will revert any benefit of processing ODA on the background thread. If ODA has to wait for OnProgress to be added to the main thread's event queue, wait until it's dequeued and then processed before ODA can complete. So, it's blocked by other events on the main thread again.
> 
> If I rememeber correctly, OnProgress has to be processed on the main thread in the parent for some reason - is that right?
> 
> But some of the use cases are ok with OnProgress being called asynchronously with OnDataAvailable. So, could you have XHR set a flag on the channel? And then check that flag in ODA - if it's set, use NS_DISPATCH_SYNC, else use NS_DISPATCH_NORMAL. What do you think?

Yes, by spec XHR required a synchronous behavior for progress event and its attribute. I'll try your proposal in next revision.

Comment 59

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

Attachment: MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=mayhammer.

Comment 60

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

Attachment: MozReview Request: Bug 1015466 - waiting for channel open event explicitly. r=jduell.

Comment 61

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

Attachment: MozReview Request: Bug 1015466 - Part 0, fix build error for non-unified build. r=mayhammer.

Comment 62

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

Attachment: MozReview Request: Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,khuey.

Comment 63

[Jason Duell]

Note: Honza is the best necko reviewer here, but he's out sick right now.  If he's not back in action in a few days I'll give these a once-over at least (or if I feel confident enough about them, a full +r).

Comment 64

[Honza Bambas (:mayhemer)]

Jason, I'm back at work.  I can take the reviews if you want (despite those are in the st**id review board.)

Comment 65

[Honza Bambas (:mayhemer)]

Comment on attachment 8618161 [details]
MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=mayhammer.

https://reviewboard.mozilla.org/r/7273/#review10865

Comment 66

[Honza Bambas (:mayhemer)]

Comment on attachment 8618163 [details]
MozReview Request: Bug 1015466 - Part 0, fix build error for non-unified build. r=mayhammer.

https://reviewboard.mozilla.org/r/7269/#review10745

I'm missing comments.  how can you write so much code and not write enough good explanatory comments?  each class, method and member declarations have to have a good comment like lifetime/ownership description, callers, arguments, few words on order of call.  the more the better.

::: netwerk/ipc/HttpBackgroundChannelChild.h:12
(Diff revision 2)
> +//#include "nsCOMPtr.h"

delete

::: netwerk/ipc/HttpBackgroundChannelChild.h:79
(Diff revision 2)
> +  void CreateBackgroundChannel(const uint32_t aChannelId);

comments....

::: netwerk/ipc/HttpBackgroundChannelChild.cpp:58
(Diff revision 2)
> +  // hold reference to HttpBackgroundChannelChild for IPDL

please refer where this is realead

::: netwerk/ipc/HttpBackgroundChannelParent.cpp:23
(Diff revision 2)
> + * Helper class for sending OnStartRequest event on background thread.

"... to the/a background thread." ?

::: netwerk/ipc/HttpBackgroundChannelChild.h:38
(Diff revision 2)
> +  void NotifyRedirect(HttpChannelChild* aNewChannel);

NotifyHTTPRedirect

::: netwerk/protocol/http/HttpChannelChild.cpp:588
(Diff revision 2)
> +  }

Comment 67

[Honza Bambas (:mayhemer)]

Comment on attachment 8618164 [details]
MozReview Request: Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,khuey.

https://reviewboard.mozilla.org/r/7271/#review10757

r- for general lack of any comments.  it's hard to catch and inspect all details when I have to understand the code fully my self w/o a word of any explanation.

as far as I understand how this works the patch looks otherwise good, but I'd like to check the next version too

::: dom/ipc/ContentParent.h:381
(Diff revision 2)
> +    void AddHttpChannel(const uint32_t aKey, nsIHttpChannelParent* aData);

comments lacking..

::: dom/ipc/ContentParent.h:942
(Diff revision 2)
> +    nsInterfaceHashtable<nsUint32HashKey, nsIHttpChannelParent> mHttpChannels;

same here

::: dom/ipc/ContentParent.h:385
(Diff revision 2)
> +    // hashtable and implicitly deletes it from the hashtable afterwards

what is this used for?  what is the key?  who adds/removes?  so much to say here...

::: dom/ipc/ContentParent.cpp:5156
(Diff revision 2)
> +    mHttpChannels.Put(aKey, channel);

when you manipulate the hashtable assert the thread please

::: netwerk/ipc/HttpBackgroundChannelParent.cpp:439
(Diff revision 2)
> -  return true;
> + return true;

nit: indent?

::: netwerk/protocol/http/HttpChannelChild.cpp:63
(Diff revision 2)
> +  return sUniqueId++;

assert thread or make atomic

::: netwerk/protocol/http/HttpChannelChild.cpp:1692
(Diff revision 2)
> +  mHttpBackgroundChannel = nullptr;

nit: better swap to a local refptr before call (precaution).

::: netwerk/protocol/http/HttpChannelParent.h:217
(Diff revision 2)
> +  // is looked up in the mHttpBackgroundChannels hashtable.

looked up how? when?

::: netwerk/protocol/http/HttpChannelParent.h:220
(Diff revision 2)
> +  uint32_t mBgChannelId;

what is this?

::: netwerk/protocol/http/HttpChannelParent.cpp:846
(Diff revision 2)
> +  if (mIPCClosed) {

maybe check this first?

::: netwerk/protocol/http/HttpChannelParent.cpp:850
(Diff revision 2)
> +  MOZ_ASSERT(mHttpBackgroundChannel);

should you check this before you access it few lines above?

::: netwerk/protocol/http/HttpChannelParent.cpp:1408
(Diff revision 2)
> +  return NS_OK;

this should delegate to the listener or (better) to the http channel

or explain (in a comment..) why this method is empty

::: netwerk/protocol/http/HttpChannelParentListener.cpp:219
(Diff revision 2)
> +    // `HttpBackgroundChannelParent` that belonged to the old `HttpChannelParent`.

first, explain how can you be sure that mNextListener is the original nsHttpChannelParent then also add a non-null check for it

::: netwerk/protocol/http/nsIHttpChannelChild.idl:27
(Diff revision 2)
> +  void setBackgroundChannel(in HttpBackgroundChannelChild backgroundChannel);

comment...

::: netwerk/protocol/http/HttpChannelParent.cpp:914
(Diff revision 2)
>      "Cannot call OnDataAvailable if diverting is set!");

please update the message and also add a comment why the assertion is formed like this

::: netwerk/protocol/http/HttpChannelParent.cpp:916
(Diff revision 2)
> +  bool canceled;

you must either read the data (and return NS_OK) or return a failure code.  this breaks the stream listener contract.

and this needs a good comment why it is here

::: netwerk/protocol/http/HttpChannelParent.cpp:104
(Diff revision 2)
> +  nsCOMPtr<nsIHttpChannelParent> parent = contentParent->TakeHttpChannel(mBgChannelId);

is the key correct?

::: netwerk/protocol/http/HttpChannelParent.cpp:1232
(Diff revision 2)
> +  nsresult rv = mChannel->RetargetDeliveryTo(mainThread);

just a note, this can be called only on the main thread

::: netwerk/protocol/http/nsHttpChannel.cpp:5781
(Diff revision 2)
> +        if (NS_WARN_IF(mCanceled)) {

if mCanceled is worked with concurrently then it needs to be either atomic or the overall logic must be protected.  what if Cancel() is called on another thread right after this line is executed?  this code doesn't make much sense to me.

and this is definitely not a good multi-thread programming.

and if you want this in, then it should not be part of this patch but a separate bug and have a test case that exercise this code.

::: netwerk/protocol/http/nsIHttpChannelParent.idl:18
(Diff revision 2)
> +[uuid(79baa656-1686-45c5-bab0-a94e8451c2a2)]

should be a built-in class marked?

::: netwerk/protocol/http/nsIHttpChannelParent.idl:27
(Diff revision 2)
> +  void transferBackgroundChannel(in nsIHttpChannelParent target);

comment, and please for (at least) new files use doxygen style

Comment 68

[Honza Bambas (:mayhemer)]

Comment on attachment 8618162 [details]
MozReview Request: Bug 1015466 - waiting for channel open event explicitly. r=jduell.

https://reviewboard.mozilla.org/r/7275/#review10867

Ship It!

Comment 69

[Honza Bambas (:mayhemer)]

Comment on attachment 8618161 [details]
MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=mayhammer.

Until someone tells me which patch is which I rather drop the r here....

ReviewBoard really is something....

Comment 70

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is there anything more up to date than what is in this bug?

Comment 71

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

Per discussion with @jduell, I'll put my latest local patch and comments and @jduell will find Necko people to continue the work. Keep the ni? for me to remember this task.

Comment 72

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is there a plan for moving forward here?

Comment 73

[Jason Duell]

khuey: Sadly there isn't, really.  I mentioned this in our 2016 planning, but it didn't generate enough excitement to get into the planning spreadsheet.  Perhaps it should have?

Shih-Chiang: can you post your latest patch here (comments would be great too, but at least the latest code would be a good start)?  (Ah, he's needinfo'd already--I'll email him too).

Comment 74

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

Comment on attachment 8618163 [details]
MozReview Request: Bug 1015466 - Part 0, fix build error for non-unified build. r=mayhammer.

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

Comment 75

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

Comment on attachment 8618164 [details]
MozReview Request: Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,khuey.

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

Comment 76

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

Comment on attachment 8618161 [details]
MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=mayhammer.

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

Comment 77

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

patch updated to latest m-c, build ok but not tested. will add notes later.

Comment 78

[Steve Workman [:sworkman] (INACTIVE)]

Hey SC! - apologies, I should have commented earlier in the week. I don't think I'm going to get to this anytime soon, so it might be best to find someone in Necko to take the reviews from here. Jason, who could take this?

Comment 79

[Jason Duell]

Honza: Shih-Chiang is just handing off the work here, so I assume these patches are incomplete.  Mostly I want you to see what you think of the overall approach here before we find someone else to finish this work (I'm guessing that person won't be you, since you're the most likely reviewer for a final version of the patches).

We don't have immediate plans to work on this--we'll find time for it as people have time.  So not urgent, review when you have time.

I would be fine with converting these patches from reviewboard to regular diffs if you want.  I'm not a RB fan either :)

Shih-Chiang--thanks for your work here!

Comment 80

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #79)
> Honza: Shih-Chiang is just handing off the work here, so I assume these
> patches are incomplete.  Mostly I want you to see what you think of the
> overall approach here before we find someone else to finish this work (I'm
> guessing that person won't be you, since you're the most likely reviewer for
> a final version of the patches).
> 
> We don't have immediate plans to work on this--we'll find time for it as
> people have time.  So not urgent, review when you have time.

OK, I'll look them over.  Still, the sooner the better.  I like keeping my r q 3 :)

> 
> I would be fine with converting these patches from reviewboard to regular
> diffs if you want.  I'm not a RB fan either :)

Yes!  I don't even know how to give an r+ anymore!!

> 
> Shih-Chiang--thanks for your work here!

Comment 81

[Honza Bambas (:mayhemer)]

Attachment: Part 1, create off-main-thread HTTP IPC Channel

Comment 82

[Honza Bambas (:mayhemer)]

Attachment: Part 2, off-main-thread IPC for nsHttpChannel

Comment 83

[Nathan Froyd [:froydnj]]

Attachment: part 1 - create off-main-thread HTTP IPC channel

Rebased to something approximating master.

Comment 84

[Nathan Froyd [:froydnj]]

Attachment: part 2 - off-main-thread IPC for nsHttpChannel

Rebased.

Comment 85

[Nathan Froyd [:froydnj]]

Attachment: part 1 - create off-main-thread HTTP IPC channel

Really rebased.

Comment 86

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are you actively working on this?

Comment 87

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

Forgot to remove myself from assignee after handing off the work.

Comment 88

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Nathan said he's not picking this up.

Comment 89

[Honza Bambas (:mayhemer)]

(Note for me - review drafts in laptop)

Comment 90

[Patrick McManus [:mcmanus]]

this is an even bigger problem in the multiple run queue quantum view of the world

Comment 91

[Honza Bambas (:mayhemer)]

Comment on attachment 8715817 [details] [diff] [review]
Part 1, create off-main-thread HTTP IPC Channel

Review of attachment 8715817 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewing these patches is hard because there is no description in a bugzilla comment on how they are structured (classes, references, lifetime, structure, overall timeline)

I didn't go too deep, so I didn't check for any use after free etc problems (tho, seems like there are not many).. generally looks good

::: netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ +40,5 @@
> +
> +  nsresult Init(HttpChannelChild* aHttpChannel, const uint32_t& aChannelId);
> +
> +  // redirect all following events to another HttpChannelChild instance
> +  void NotifyRedirect(HttpChannelChild* aNewChannel);

can be confused with HTTP redirect, find a better name please

::: netwerk/protocol/http/HttpBackgroundChannelParent.h
@@ +63,5 @@
> +  nsresult ProcessOnStatus(const nsresult& aStatus);
> +
> +  already_AddRefed<nsIThread> GetBackgroundThread();
> +
> +  bool IPCClosed() { return mIPCClosed; }

nit: make const

Comment 92

[Honza Bambas (:mayhemer)]

Comment on attachment 8715819 [details] [diff] [review]
Part 2, off-main-thread IPC for nsHttpChannel

Review of attachment 8715819 [details] [diff] [review]:
-----------------------------------------------------------------

this patch needs more care.  and again, some description so that it's clearer what's going on.

::: dom/ipc/ContentParent.h
@@ +450,5 @@
>                                    bool* aWindowIsNew,
>                                    InfallibleTArray<FrameScriptInfo>* aFrameScripts,
>                                    nsCString* aURLToLoad) override;
>  
> +    void AddHttpChannel(const uint32_t aKey, nsIHttpChannelParent* aData);

comments...

@@ +1021,5 @@
>      nsConsoleService* GetConsoleService();
>  
>      nsTArray<nsCOMPtr<nsIObserver>> mIdleListeners;
>  
> +    nsDataHashtable<nsUint32HashKey, nsCOMPtr<nsIHttpChannelParent>> mHttpChannels;

comments

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +66,5 @@
> +GetUniqueId()
> +{
> +  static uint32_t sUniqueId = 0;
> +
> +  return sUniqueId++;

++sUniqueId ?

@@ +252,5 @@
> +        HttpChannelChild* mChild;
> +      };
> +      RefPtr<SendDeleteRunnable> runnable = new SendDeleteRunnable(this);
> +      NS_DispatchToMainThread(runnable);
> +      return mRefCnt;

err! return 1 or 0 here.  after the dispatch |this| may be deleted

@@ -844,5 @@
>    }
>  
> -  // Hold queue lock throughout all three calls, else we might process a later
> -  // necko msg in between them.
> -  AutoEventEnqueuer ensureSerialDispatch(mEventQ);

why this has been removed?

@@ +894,5 @@
> +      const nsCString mData;
> +      const uint64_t mOffset;
> +      const uint32_t mCount;
> +    };
> +    NS_DispatchToMainThread(new DoOnDataAvailableRunnable(this, data, offset, count));

you need to go through a RefPtr with the event
and should this go to the target thread?

@@ -1103,5 @@
>  
> -  { // We must flush the queue before we Send__delete__
> -    // (although we really shouldn't receive any msgs after OnStop),
> -    // so make sure this goes out of scope before then.
> -    AutoEventEnqueuer ensureSerialDispatch(mEventQ);

again, why removed?  because the background now takes care?

@@ +2217,5 @@
>    mSchedulingContextID.ToProvidedString(scid);
>    openArgs.schedulingContextID().AssignASCII(scid);
>  
> +  const uint32_t channelId = GetUniqueId();
> +  openArgs.channelId() = channelId;

don't we already have some id'ing infra here?

@@ +2251,5 @@
> +void
> +HttpChannelChild::TransferBackgroundChannel(nsIHttpChannelChild* aTarget)
> +{
> +  aTarget->SetBackgroundChannel(mHttpBackgroundChannel);
> +  mHttpBackgroundChannel->NotifyRedirect(static_cast<HttpChannelChild*>(aTarget));

could this be inside SetBackgroundChannel?  actually should - you get rid of the static_cast!

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +611,5 @@
>  
> +  // Waiting for PHttpBackgroundChannel ready
> +  ContentParent* contentParent =
> +    static_cast<ContentParent*>(this->Manager()->Manager());
> +  contentParent->AddHttpChannel(aChannelId, this);

this place needs explanation (in bugzilla at least)

IIUC, you are expecting to get asyncopen from the background channel that will connect with this parent, right?  is the table accessed only on a single thread?  does it need any synchronization?  how does it work exactly?

@@ +1191,5 @@
>    }
>  
> +  nsCOMPtr<nsIThreadRetargetableRequest> threadRetargetableRequest = do_QueryInterface(aRequest);
> +  if (threadRetargetableRequest) {
> +    nsCOMPtr<nsIThread> backgroundThread = mHttpBackgroundChannel->GetBackgroundThread();

do you always have mHttpBackgroundChannel here?

::: netwerk/protocol/http/HttpChannelParentListener.cpp
@@ +234,5 @@
>  
>    if (succeeded) {
> +    // Create the link between the newly created `HttpChannelParent` and the
> +    // `HttpBackgroundChannelParent` that belonged to the old `HttpChannelParent`.
> +    HttpChannelParent* originalChannelParent = static_cast<HttpChannelParent*>(mNextListener.get());

I don't think you can do this...  the next listener can well be a different impl..

::: netwerk/protocol/http/nsIHttpChannelChild.idl
@@ +19,5 @@
>  
>  interface nsIPrincipal;
>  interface nsIURI;
>  
> +[uuid(7d1672ac-b834-4b92-86b8-baf1872ef0ec)]

nit: no need for it anymore, but leave it

@@ +39,5 @@
>    // This method is called by nsCORSListenerProxy if we need to remove
>    // an entry from the CORS preflight cache in the parent process.
>    void removeCorsPreflightCacheEntry(in nsIURI aURI, in nsIPrincipal aRequestingPrincipal);
> +
> +  void setBackgroundChannel(in HttpBackgroundChannelChild backgroundChannel);

idls always need comments

Comment 93

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

The current HTTP data flow in e10s mode is:
    parent STS -> parent main -> child main (IPC) -> child main (ODA callback)

My plan is to make it like:
    parent STS -> parent PBackground -> child STS (IPC) -> child any (ODA callback, retargeted)
    parent STS -> parent PBackground -> child STS (IPC) -> child main (ODA callback, no retarget)

I'll file more dependency bugs for break-down working items.

Comment 94

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b04e3f41627

Comment 95

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

Attachment: Bug 1015466 - Part 0, fix warning/error covered by unified build.

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

Comment 96

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

Attachment: Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

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

Comment 97

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

Attachment: Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

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

Comment 98

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

TODOs:
1) lifecycle handling while redirecting
2) OnStart/ODA/OnStop handling
3) channel diversion handling
4) service worker interception handling

Comment 99

[Cornel Ionce [:noni] [Hubs QA]]

Is there anything here that would require manual testing?
If so, could you please provide us some testing steps/scenarios?

Comment 100

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

Hi @cornel_ionce, I'm still cultivating the full test plan. Here is some initial thoughts on the manual testing part:

monitoring perceivable performance/stability downgrade on selective websites:
 a) Google search
 b) Google doc editing
 c) Facebook browsing (images and videos) and messaging
 d) Youtube browsing
 e) Flickr image browsing and uploading
 f) Netflix video browsing

Comment 101

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

This bug will be focusing on building PHttpBackgroundChannel.ipdl over PBackground.

Comment 102

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

Attachment: Bug 1015466 - Part 3, implement OnStartRequest/OnStopRequest/OnTransportAndData/OnProgress/OnStatus/OnRedirect3Complete/OnDiversion on background channel.

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

Comment 103

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

Comment on attachment 8840805 [details]
Bug 1015466 - Part 0, fix warning/error covered by unified build.

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

Comment 104

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

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

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

Comment 105

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

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

Comment 106

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

Comment on attachment 8860900 [details]
Bug 1015466 - Part 3, implement OnStartRequest/OnStopRequest/OnTransportAndData/OnProgress/OnStatus/OnRedirect3Complete/OnDiversion on background channel.

https://reviewboard.mozilla.org/r/132912/#review136170

::: netwerk/protocol/http/HttpChannelChild.cpp:437
(Diff revision 1)
>    // stage, as they are set in the listener's OnStartRequest.
>    MOZ_RELEASE_ASSERT(!mFlushedForDiversion,
>      "mFlushedForDiversion should be unset before OnStartRequest!");
>    MOZ_RELEASE_ASSERT(!mDivertingToParent,
>      "mDivertingToParent should be unset before OnStartRequest!");
> +  MOZ_ASSERT(mWasOpened);

hit this assertion on try, need to figure out the root cause.

::: netwerk/protocol/http/HttpChannelParent.cpp:1347
(Diff revision 1)
>    // performing a document load. We can't do that is mIPCClosed is set.
>    if (!mIPCClosed) {
>      PContentParent* pcp = Manager()->Manager();
>      MOZ_ASSERT(pcp, "We should have a manager if our IPC isn't closed");
>      DebugOnly<nsresult> rv =
>        static_cast<ContentParent*>(pcp)->TransmitPermissionsFor(chan);

During test I found we need to ensure permission arrives content process before OnStartRequest, otherwise content process will be force terminated.

Comment 107

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #106)
> ::: netwerk/protocol/http/HttpChannelParent.cpp:1347
> (Diff revision 1)
> >    // performing a document load. We can't do that is mIPCClosed is set.
> >    if (!mIPCClosed) {
> >      PContentParent* pcp = Manager()->Manager();
> >      MOZ_ASSERT(pcp, "We should have a manager if our IPC isn't closed");
> >      DebugOnly<nsresult> rv =
> >        static_cast<ContentParent*>(pcp)->TransmitPermissionsFor(chan);
> 
> During test I found we need to ensure permission arrives content process
> before OnStartRequest, otherwise content process will be force terminated.
This turns out to be a major blocker for moving OnStartRequest to PBackground IPDL.

All the PContent::SetPermissionsWithKey is assumed to be handled before OnStartRequest so that all the permission associated with that origin is propagated to content process. Otherwise, content process will hit a MOZ_CRASH by not having permission entries beforehand. https://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/extensions/cookie/nsPermissionManager.cpp#664

There is no easy way to ensure the ordering of two IPDL message on different threads without queueing the message.
The first approach I'm going to try is to move OnStartRequest back to main thread IPC and using MozPromise mechanism for delaying IPC messaging on PBackground until OnStartRequest is received in content process.

Comment 108

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

Attachment: Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

AsyncOpen procedure can failed and trigger FailedAsyncOpen IPC to do the clean up.
However FailedAsyncOpen might not complete if content process is destroyed at the meantime.
We can delay the timing of holding the strong reference to parent listener and channel object
to make sure no reference cycle is created by HttpChannelParent. In addition, clean up the
strong reference as soon as FailedAsyncOpen IPC is triggered.

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

Comment 109

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

Comment on attachment 8840805 [details]
Bug 1015466 - Part 0, fix warning/error covered by unified build.

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

Comment 110

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

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

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

Comment 111

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

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

Comment 112

[Honza Bambas (:mayhemer)]

sc, sorry for the delay here.  I will review the patches next week.  I simply can't find a moment to sit and look at your patches uninterrupted by anything else.

Comment 113

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

(In reply to Honza Bambas (:mayhemer) from comment #112)
> sc, sorry for the delay here.  I will review the patches next week.  I
> simply can't find a moment to sit and look at your patches uninterrupted by
> anything else.

I can change the review of Part 0 and Part 2 to other people since they are supplementary and independent, so you'll have more time focusing on the major patches (part 1, part 3, and upcoming part 4).

Comment 114

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c3fabd5688fa271a3fa26a7189707390d63792e

Comment 115

[Honza Bambas (:mayhemer)]

Comment on attachment 8840805 [details]
Bug 1015466 - Part 0, fix warning/error covered by unified build.

https://reviewboard.mozilla.org/r/115240/#review140166

::: netwerk/protocol/http/HSTSPrimerListener.cpp:6
(Diff revision 3)
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "HttpLog.h"

maybe a blank like before and a comment that this needs to include first?

Comment 116

[Honza Bambas (:mayhemer)]

Attachment: (for splinter review) Part 1, skeleton of PHttpBackgroundChannel. (Diff Revision 3)

Comment 117

[Honza Bambas (:mayhemer)]

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

https://reviewboard.mozilla.org/r/115242/#review140184

Comment 118

[Honza Bambas (:mayhemer)]

Comment on attachment 8864473 [details]
Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

https://reviewboard.mozilla.org/r/136154/#review140186

::: netwerk/protocol/http/HttpChannelParent.cpp:244
(Diff revision 1)
> +HttpChannelParent::AsyncOpenFailed(nsresult aRv)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(NS_FAILED(aRv));
> +
> +  // Break the reference cyccle among HttpChannelParent,

cyccle

::: netwerk/protocol/http/HttpChannelParent.cpp:455
(Diff revision 1)
>    }
>  
> -  mParentListener = new HttpChannelParentListener(this);
> +  RefPtr<HttpChannelParentListener> parentListener
> +    = new HttpChannelParentListener(this);
>  
> -  mChannel->SetNotificationCallbacks(mParentListener);
> +  httpChannel->SetNotificationCallbacks(parentListener);

maybe this step (setting callbacks on the channel) should be done at the bottom as well.

Comment 119

[Honza Bambas (:mayhemer)]

Attachment: (For splinter review) Part 3, PHttpBackgroundChannel lifecycle management. (Diff Revision 3)

Comment 120

[Honza Bambas (:mayhemer)]

Comment on attachment 8865520 [details] [diff] [review]
(For splinter review) Part 3, PHttpBackgroundChannel lifecycle management. (Diff Revision 3)

Review of attachment 8865520 [details] [diff] [review]:
-----------------------------------------------------------------

pretty good job!  locally tested, no crashes/assert/malfunctions.

so, the next step would be push this to talos and check if there is any major regression or even improvement.

just for completeness, can you please comment on which of the milestones you have to PBg-http does this patch impl?  thanks.

r is mostly about missing comments and some unclear stuff explanation in the code.

::: netwerk/base/nsIParentRedirectingChannel.idl
@@ +55,5 @@
> +   * @param callback
> +   *    redirect ready callback, will be called when redirect verification
> +   *    procedure can proceed.
> +   */
> +  void continueVerification(in nsIAsyncVerifyRedirectReadyCallback callback);

add a note it can be called immediately

::: netwerk/protocol/http/BackgroundChannelRegistrar.h
@@ +31,5 @@
> +private:
> +  virtual ~BackgroundChannelRegistrar();
> +
> +  // Notify linked channel found to both HttpChannelParent and
> +  // HttpBackgroundChannelParent.

when/by whom called?  what does it do?  what are the args?

::: netwerk/protocol/http/BackgroundChannelRegistrar.cpp
@@ +23,5 @@
> +}
> +
> +BackgroundChannelRegistrar::~BackgroundChannelRegistrar()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

because we want to release the channel on the main thread?  have a comment

@@ +26,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mChannels.Clear();
> +  mBgChannels.Clear();

no need

::: netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ +26,5 @@
> +
> +  nsresult Init(HttpChannelChild* aChannelChild);
> +
> +  void OnChannelClosed();
> +  void NotifyActorFailed();

comments please, on all added methods

@@ +59,5 @@
> +
> +private:
> +  virtual ~HttpBackgroundChannelChild();
> +
> +  void CreateBackgroundChannel();

and here too

@@ +62,5 @@
> +
> +  void CreateBackgroundChannel();
> +
> +  // associated HttpChannelChild for handling the channel events
> +  RefPtr<HttpChannelChild> mChannelChild;

when is this released?  more richer comment please

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ +58,5 @@
> +    // abort the rest of steps.
> +    return;
> +  }
> +
> +  if(!aActor->SendPHttpBackgroundChannelConstructor(mChild,

if(!

@@ +110,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  //XXX not sure if this is still necessary
> +  // HttpChannelChild is not going to handle any incoming message.
> +  mChannelChild = nullptr;

because mChannelChild is nullified?  or why?

@@ +114,5 @@
> +  mChannelChild = nullptr;
> +}
> +
> +void
> +HttpBackgroundChannelChild::NotifyActorFailed()

when is this called?

@@ +136,5 @@
> +    new BackgroundChannelCreateCallback(this);
> +
> +  if (NS_WARN_IF(!BackgroundChild::GetOrCreateForCurrentThread(callback))) {
> +    MOZ_ASSERT(false);
> +    return;

?

::: netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ +72,5 @@
>  }
>  
>  HttpBackgroundChannelParent::~HttpBackgroundChannelParent()
>  {
> +  MOZ_ASSERT(!mIPCOpened);

do we need to assert thread in any way here?

::: netwerk/protocol/http/HttpChannelChild.h
@@ +317,5 @@
>    // is synthesized.
>    bool mSuspendParentAfterSynthesizeResponse;
>  
> +  RefPtr<HttpBackgroundChannelChild> mBgChild;
> +  void CleanupBackgroundChannel();

when called?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1284,5 @@
>  
> +  // Might be called twice in race condition in theory.
> +  // (one by RecvFailedAsyncOpen, another by
> +  // HttpBackgroundChannelChild::ActorFailed)
> +  if (NS_WARN_IF(NS_FAILED(mStatus))) {

so mStatus has to be atomic?

::: netwerk/protocol/http/HttpChannelParent.h
@@ +291,5 @@
>    RefPtr<ChannelEventQueue> mEventQ;
> +
> +  RefPtr<HttpBackgroundChannelParent> mBgParent;
> +  // Number of events to wait before actually invoking AsyncOpen on the main
> +  // channel. This attribute is main thread only.

this needs a better explanation.  totally unclear what's the purpose here.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +248,5 @@
> +    registrar->DeleteChannel(mChannel->ChannelId());
> +
> +    // Background channel establishment for AsyncOpen is aborted,
> +    // we need to abort the entire AsyncOpen as well.
> +    if (mAsyncOpenBarrier) {

This part needs an explanation, the comment is pretty insufficient

@@ +251,5 @@
> +    // we need to abort the entire AsyncOpen as well.
> +    if (mAsyncOpenBarrier) {
> +      TryInvokeAsyncOpen(NS_ERROR_FAILURE);
> +    }
> +

remove this blank

@@ +1002,5 @@
> +  nsCOMPtr<nsIRedirectChannelRegistrar> redirectReg =
> +    do_GetService(NS_REDIRECTCHANNELREGISTRAR_CONTRACTID);
> +  MOZ_ASSERT(redirectReg);
> +
> +  nsCOMPtr<nsIParentChannel> parentChannel;

s/parentChannel/redirectParentChannel/

@@ +1007,5 @@
> +  rv = redirectReg->GetParentChannel(mRedirectRegistrarId,
> +                                     getter_AddRefs(parentChannel));
> +  MOZ_ASSERT(parentChannel);
> +  if (!parentChannel) {
> +    ContinueRedirect2Verify(rv);

assert ns_failed(rv)?

@@ +1020,5 @@
> +    return IPC_OK();
> +  }
> +
> +  // Ask redirected channel if verification can proceed.
> +  redirectedParent->ContinueVerification(this);

note in the comment that when this done (callback is called on us) we continue in ContinueRedirect2Verify just below

nit: might be good to move the ContinueVerification method between RecvRedirect2Verify and ContinueRedirect2Verify

@@ +1444,5 @@
>    nsAutoCString altDataType;
>    chan->GetAlternativeDataType(altDataType);
>    int64_t altDataLen = chan->GetAltDataLength();
>  
> +  MOZ_ASSERT(mIPCClosed || mBgParent);

explain this assert well (everywhere)

@@ +1553,5 @@
>      if (mIPCClosed || !SendOnTransportAndData(channelStatus, transportStatus,
>                                                aOffset, toRead, data)) {
>        return NS_ERROR_UNEXPECTED;
>      }
> +    MOZ_ASSERT(mBgParent);

explain this assert

@@ +2071,5 @@
>      Unused << DoSendDeleteSelf();
>    }
> +
> +  mParentListener = nullptr;
> +  mChannel = nullptr;

was this a bug??

::: netwerk/protocol/http/nsIBackgroundChannelRegistrar.idl
@@ +20,5 @@
> +
> +/*
> + */
> +[builtinclass, uuid(8acaa9b1-f0c4-4ade-baeb-39b0d4b96e5b)]
> +interface nsIBackgroundChannelRegistrar : nsISupports

needs comments, everything new needs comment, all the time: purpose, lifetime, description of the function.  if this is the same as the redir channel registrar, then add a comment referring it.

@@ +23,5 @@
> +[builtinclass, uuid(8acaa9b1-f0c4-4ade-baeb-39b0d4b96e5b)]
> +interface nsIBackgroundChannelRegistrar : nsISupports
> +{
> +  [noscript,notxpcom,nostdcall] void linkHttpChannel(in uint64_t aKey,
> +                                                     in HttpChannelParent aChannel);

what is what, when called.

Comment 121

[Honza Bambas (:mayhemer)]

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

https://reviewboard.mozilla.org/r/115244/#review140256

https://bugzilla.mozilla.org/show_bug.cgi?id=1015466#c120

Comment 122

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

Comment on attachment 8864473 [details]
Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

https://reviewboard.mozilla.org/r/136154/#review140512

::: netwerk/protocol/http/HttpChannelParent.cpp:244
(Diff revision 1)
> +HttpChannelParent::AsyncOpenFailed(nsresult aRv)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(NS_FAILED(aRv));
> +
> +  // Break the reference cyccle among HttpChannelParent,

fixed

::: netwerk/protocol/http/HttpChannelParent.cpp:455
(Diff revision 1)
>    }
>  
> -  mParentListener = new HttpChannelParentListener(this);
> +  RefPtr<HttpChannelParentListener> parentListener
> +    = new HttpChannelParentListener(this);
>  
> -  mChannel->SetNotificationCallbacks(mParentListener);
> +  httpChannel->SetNotificationCallbacks(parentListener);

sure we can do that. done.

Comment 123

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

Comment on attachment 8865520 [details] [diff] [review]
(For splinter review) Part 3, PHttpBackgroundChannel lifecycle management. (Diff Revision 3)

Review of attachment 8865520 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsIParentRedirectingChannel.idl
@@ +55,5 @@
> +   * @param callback
> +   *    redirect ready callback, will be called when redirect verification
> +   *    procedure can proceed.
> +   */
> +  void continueVerification(in nsIAsyncVerifyRedirectReadyCallback callback);

done.

::: netwerk/protocol/http/BackgroundChannelRegistrar.h
@@ +31,5 @@
> +private:
> +  virtual ~BackgroundChannelRegistrar();
> +
> +  // Notify linked channel found to both HttpChannelParent and
> +  // HttpBackgroundChannelParent.

This is a helper function for BackgroundChannelRegistrar itself to callback HttpChannelParent and HttpBackgroundChannelParent.
aChannelParent and aBgParent is the pair of HttpChannelParent and HttpBackgroundChannelParent that should be linked together.

::: netwerk/protocol/http/BackgroundChannelRegistrar.cpp
@@ +23,5 @@
> +}
> +
> +BackgroundChannelRegistrar::~BackgroundChannelRegistrar()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

BackgroundChannelRegistrar is a main-thread-only object. All the functions have main thread assertion.

@@ +26,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mChannels.Clear();
> +  mBgChannels.Clear();

fixed.

::: netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ +26,5 @@
> +
> +  nsresult Init(HttpChannelChild* aChannelChild);
> +
> +  void OnChannelClosed();
> +  void NotifyActorFailed();

done.

@@ +59,5 @@
> +
> +private:
> +  virtual ~HttpBackgroundChannelChild();
> +
> +  void CreateBackgroundChannel();

done.

@@ +62,5 @@
> +
> +  void CreateBackgroundChannel();
> +
> +  // associated HttpChannelChild for handling the channel events
> +  RefPtr<HttpChannelChild> mChannelChild;

done.

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ +58,5 @@
> +    // abort the rest of steps.
> +    return;
> +  }
> +
> +  if(!aActor->SendPHttpBackgroundChannelConstructor(mChild,

fixed.

@@ +110,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  //XXX not sure if this is still necessary
> +  // HttpChannelChild is not going to handle any incoming message.
> +  mChannelChild = nullptr;

This code is necessary because HttpChannelChild can dissociate with the HttpBackgroundChannelChild in any error handling case. HttpBackgroundChannelChild can simply use a null check to know if those IPC message should be discard.

@@ +114,5 @@
> +  mChannelChild = nullptr;
> +}
> +
> +void
> +HttpBackgroundChannelChild::NotifyActorFailed()

This is called by BackgroundChannelCreateCallback when either PBackground or PHttpBackgroundChannel is fail to create. Rename it to "OnBackgroundChannelCreationFailed' might avoid the confusion between this one and ActorDestroy.
Comment added in the header file.

@@ +136,5 @@
> +    new BackgroundChannelCreateCallback(this);
> +
> +  if (NS_WARN_IF(!BackgroundChild::GetOrCreateForCurrentThread(callback))) {
> +    MOZ_ASSERT(false);
> +    return;

better to return boolean value here so that we can return failure cause to the caller of HttpBackgroundChannelChild::Init.

::: netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ +72,5 @@
>  }
>  
>  HttpBackgroundChannelParent::~HttpBackgroundChannelParent()
>  {
> +  MOZ_ASSERT(!mIPCOpened);

HttpBackgroundChannelParent is designed to be hold by main thread and PBackground thread, so it can be release on either one.
Not sure if adding MOZ_ASSERT(NS_IsMainThread() || IsOnBackgroundThread()) can help people understand the threading model.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +317,5 @@
>    // is synthesized.
>    bool mSuspendParentAfterSynthesizeResponse;
>  
> +  RefPtr<HttpBackgroundChannelChild> mBgChild;
> +  void CleanupBackgroundChannel();

After OnStopRequest (non-diversion case) and HandleAsyncAbort, no message is expected and can be ignored.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1284,5 @@
>  
> +  // Might be called twice in race condition in theory.
> +  // (one by RecvFailedAsyncOpen, another by
> +  // HttpBackgroundChannelChild::ActorFailed)
> +  if (NS_WARN_IF(NS_FAILED(mStatus))) {

HttpBAckgroundChannelChild will guarantee only call this function on main thread, so that we don't need to make mStatus atomic. I'll add main thread assertion in this function.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +291,5 @@
>    RefPtr<ChannelEventQueue> mEventQ;
> +
> +  RefPtr<HttpBackgroundChannelParent> mBgParent;
> +  // Number of events to wait before actually invoking AsyncOpen on the main
> +  // channel. This attribute is main thread only.

AsyncOpen procedure includes at most two asynchronous steps currently:
 1) copy upload stream, if any
 2) wait for background channel

After both steps are completed, HttpChannelParent can then continue finish the AsyncOpen procedure by |InvokeAsyncOpen|.
For extensibility, I create this barrier mechanism to allow variable numbers of asynchronous steps to be completed before InvokeAsyncOpen.

comment added here and TryInvokeAsyncOpen.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +248,5 @@
> +    registrar->DeleteChannel(mChannel->ChannelId());
> +
> +    // Background channel establishment for AsyncOpen is aborted,
> +    // we need to abort the entire AsyncOpen as well.
> +    if (mAsyncOpenBarrier) {

done.

If mAsyncOpenBarrier is greater than zero, it means AsyncOpen procedure is still on going. we need to abort AsyncOpen with failure to destroy PHttpChannel actor.

@@ +251,5 @@
> +    // we need to abort the entire AsyncOpen as well.
> +    if (mAsyncOpenBarrier) {
> +      TryInvokeAsyncOpen(NS_ERROR_FAILURE);
> +    }
> +

fixed.

@@ +1002,5 @@
> +  nsCOMPtr<nsIRedirectChannelRegistrar> redirectReg =
> +    do_GetService(NS_REDIRECTCHANNELREGISTRAR_CONTRACTID);
> +  MOZ_ASSERT(redirectReg);
> +
> +  nsCOMPtr<nsIParentChannel> parentChannel;

done.

@@ +1007,5 @@
> +  rv = redirectReg->GetParentChannel(mRedirectRegistrarId,
> +                                     getter_AddRefs(parentChannel));
> +  MOZ_ASSERT(parentChannel);
> +  if (!parentChannel) {
> +    ContinueRedirect2Verify(rv);

Already have a MOZ_ASSERT(parentChannel). If ns_failed(rv) it should hit that assertion as well.

@@ +1020,5 @@
> +    return IPC_OK();
> +  }
> +
> +  // Ask redirected channel if verification can proceed.
> +  redirectedParent->ContinueVerification(this);

done.

@@ +1444,5 @@
>    nsAutoCString altDataType;
>    chan->GetAlternativeDataType(altDataType);
>    int64_t altDataLen = chan->GetAltDataLength();
>  
> +  MOZ_ASSERT(mIPCClosed || mBgParent);

Plan to move these assertion to next part of my patch, which has real code that using mBgParent to send these event via background channel IPC. It will make more sense without adding any comments.

@@ +1553,5 @@
>      if (mIPCClosed || !SendOnTransportAndData(channelStatus, transportStatus,
>                                                aOffset, toRead, data)) {
>        return NS_ERROR_UNEXPECTED;
>      }
> +    MOZ_ASSERT(mBgParent);

This assertion is added to ensure the lifecycle of background channel is matched with our expectation before actually moving the event notification to background channel.
It'll be removed.

@@ +2071,5 @@
>      Unused << DoSendDeleteSelf();
>    }
> +
> +  mParentListener = nullptr;
> +  mChannel = nullptr;

DoSendDeleteSelf will trigger background channel clean up procedure. We need channel Id to clean up the registrar while removing background channel. Thus, we need to delay the nullify of mChannel until DoSendDeleteSelf. Otherwise HttpChannelParent will still have strong reference held by BackgroundChannelRegistrar until shutdown.
Comment added to explain the order dependency between DoSendDeleteSelf and nullify statement.

::: netwerk/protocol/http/nsIBackgroundChannelRegistrar.idl
@@ +20,5 @@
> +
> +/*
> + */
> +[builtinclass, uuid(8acaa9b1-f0c4-4ade-baeb-39b0d4b96e5b)]
> +interface nsIBackgroundChannelRegistrar : nsISupports

done.

@@ +23,5 @@
> +[builtinclass, uuid(8acaa9b1-f0c4-4ade-baeb-39b0d4b96e5b)]
> +interface nsIBackgroundChannelRegistrar : nsISupports
> +{
> +  [noscript,notxpcom,nostdcall] void linkHttpChannel(in uint64_t aKey,
> +                                                     in HttpChannelParent aChannel);

done.

Comment 124

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

All the patches on this bug is for PBg-Http Milestone 2, which is to move ODA related IPC on chrome main thread to chrome pbackground thread. For content process the IPC messages still go to main thread, but will be moved to STS thread in bug 1338493.

Comment 125

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

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115242/diff/3-4/

Comment 126

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

Comment on attachment 8864473 [details]
Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

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

Comment 127

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115244/diff/3-4/

Comment 128

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

The last part for actually move ODA/OnStopRequest/OnDiversion is coming.

Comment 129

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #123)
> Comment on attachment 8865520 [details] [diff] [review]

Useful comments lost among all those useless "done" responses, sorry, no time to read it.  What I wanted was to add comments to the code, not to bugzilla.  But thanks, for the reply, if not in the code we have a documentation at least in comment 123.

Comment 130

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

(In reply to Honza Bambas (:mayhemer) from comment #129)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #123)
> > Comment on attachment 8865520 [details] [diff] [review]
> 
> Useful comments lost among all those useless "done" responses, sorry, no
> time to read it.  What I wanted was to add comments to the code, not to
> bugzilla.  But thanks, for the reply, if not in the code we have a
> documentation at least in comment 123.

Sorry about the confusion, those "done" and "fixed" are just for a record that those issues are addressed. Pretty much a side note for me to not skipping any action item. The comments I added on the bugzilla are all added in the the code as well. I intentionally leave it on bugzilla as well so that people can do quick search on bug without going to mozreview.

Comment 131

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #130)
> (In reply to Honza Bambas (:mayhemer) from comment #129)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #123)
> > > Comment on attachment 8865520 [details] [diff] [review]
> > 
> > Useful comments lost among all those useless "done" responses, sorry, no
> > time to read it.  What I wanted was to add comments to the code, not to
> > bugzilla.  But thanks, for the reply, if not in the code we have a
> > documentation at least in comment 123.
> 
> Sorry about the confusion, those "done" and "fixed" are just for a record
> that those issues are addressed. Pretty much a side note for me to not
> skipping any action item. The comments I added on the bugzilla are all added
> in the the code as well. I intentionally leave it on bugzilla as well so
> that people can do quick search on bug without going to mozreview.

Thanks!

Comment 132

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

Attachment: Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

OnStartRequestSent is used to ensure the execution order between OnStartRequest and all other messsages sent over PBackground.
IPC messages sent before OnStartRequestSent can be processed immediately while received by content process.
IPC messages sent after OnSartRequestSent need to be pending until OnStartRequestReceived is called.

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

Comment 133

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

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115242/diff/4-5/

Comment 134

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

Comment on attachment 8864473 [details]
Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

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

Comment 135

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115244/diff/4-5/

Comment 136

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Testing for all Quantum Network stuff is done by Platform QA in collaboration with Shih-Chiang Chien.

Comment 137

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

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

https://reviewboard.mozilla.org/r/139714/#review143744

::: testing/mochitest/mach_commands.py:143
(Diff revision 1)
>          for handler in remove_handlers:
>              logging.getLogger().removeHandler(handler)
>  
>          options = Namespace(**kwargs)
>          options.topsrcdir = self.topsrcdir
> +        options.e10s = True

This one is merged by accident. Will remove it on next upload.

Comment 138

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=283288b4fef87f12aa0a2b058929ff217155cc17

Comment 139

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

With the current implementation we no longer depend on bug 1364812 and bug 1362732.

Comment 140

[Honza Bambas (:mayhemer)]

sc, can you please add a description on how this all works after this patch?  I can see that the bck child still calls many things on the main thread, so it's no clear to me what stage this is.  It also blocks feedback on the two other patches in the bug 1338493.  thanks.

Comment 141

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

Attachment: message-flow-after-m2.png

Here is the message flow chart (over-simplified one) after landing this patch.
As you can see we are only moving chrome process IPC from main thread to PBackground thread.
The content process IPC is kept on main thread until milestone 3 (bug 1338493).
This is mentioned in comment #124 previously.

Here is the full document on how message flow changes for each milestone.
https://docs.google.com/a/mozilla.com/presentation/d/1doCTX2JtdpGQ8zCeb_ouMJAh10K3Y_55dnQj4-NDpJU/edit?usp=sharing

Comment 142

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

Attachment: trackingProtection_tour_workaround.patch

Hi @MattN,

browser_trackingProtection_tour.js will be frequently failed on try server after off-main-threading HTTP channel. The test case will failed in |test_unblock_target|, at |testTargetAvailability(true);| after showMenuPromise.

The tracking protection status will be updated after nsHttpChannel.asyncOpen on chrome process, this is asynchronously triggered by inserting the tracker iframe. This is not guaranteed to be happened before the |showMenuPromise|.

Before my patch, all the HTTP channel IPC run on main thread. It's highly likely that it creates an implicit execution order guarantee in this test case. However this will not be guaranteed after my off-main-thread work.

Here is a workaround I found can make try server happy but it uses flaky timeout. One proper way to fix it is to wait for "http-on-opening-request" on chrome process before "showMenuPromise", however I don't know how to integrate with this test case since the |checkToggleTarget| runs in the scope of addon. It'll be great if you can tell me how to do it, Or we can land this workaround and file a follow-up bug to fix this test case later on.

Comment 143

[Honza Bambas (:mayhemer)]

Attachment: (For spliter review) Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

Comment 144

[Honza Bambas (:mayhemer)]

Comment on attachment 8871242 [details] [diff] [review]
(For spliter review)  Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

Review of attachment 8871242 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ +71,5 @@
>    // Initiate the creation of the PBckground IPC channel.
>    // Return false if failed.
>    bool CreateBackgroundChannel();
>  
> +  bool IsWaitingOnStartRequest();

add a comment please when this returns what

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ +132,5 @@
> +  runnables.SwapElements(mQueuedRunnables);
> +  MOZ_ASSERT(mQueuedRunnables.IsEmpty());
> +
> +  for (auto event = runnables.begin(); event != runnables.end(); ++event) {
> +    (*event)->Run();

Please add a comment that these runnables call Recv* methods (that go through mEventQueue->RunOrEnqueue()), and not the final Process* methods on the child channel.

@@ +261,5 @@
> +  if (NS_WARN_IF(!mChannelChild)) {
> +    return IPC_OK();
> +  }
> +
> +  if (mStartSent && !mStartReceived) {

IsWaitingOnStartRequest() ?

@@ +368,5 @@
> +    RefPtr<HttpBackgroundChannelChild> self = this;
> +    mQueuedRunnables.AppendElement(
> +        NS_NewRunnableFunction([self]() {
> +          MOZ_ASSERT(NS_IsMainThread());
> +          MOZ_ASSERT(self);

this is IMO pointless

::: netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ +67,5 @@
>  
>  HttpBackgroundChannelParent::HttpBackgroundChannelParent()
>    : mIPCOpened(true)
>    , mBackgroundThread(NS_GetCurrentThread())
> +

?

@@ +157,5 @@
> +
> +    return NS_SUCCEEDED(rv);
> +  }
> +
> +  AssertIsOnBackgroundThread();

is it necessary?  can it ever fail when you just did if (!IsOnBackgroundThread()) { ... return } ?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1562,5 @@
>                                     uint32_t aCount)
>  {
> +  LOG(("HttpChannelParent::OnDataAvailable [this=%p aRequest=%p offset=%" PRIu64
> +       " count=%" PRIu32 "]\n", this, aRequest, aOffset, aCount));
> +  MOZ_ASSERT(NS_IsMainThread());

assuming this assert will be removed as part of milestone 3?

Comment 145

[Honza Bambas (:mayhemer)]

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

https://reviewboard.mozilla.org/r/139714/#review145572

https://bugzilla.mozilla.org/show_bug.cgi?id=1015466#c144

+ few small comments here.  Thanks!  The reference to the document was useful.

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp:133
(Diff revision 1)
> +
> +  mStartReceived = true;
> +
> +  nsTArray<nsCOMPtr<nsIRunnable>> runnables;
> +  runnables.SwapElements(mQueuedRunnables);
> +  MOZ_ASSERT(mQueuedRunnables.IsEmpty());

definitely is.  I'd rather do this after you iterate the runnables local copy array to make sure nothing new has been posted

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp:135
(Diff revision 1)
> +
> +  nsTArray<nsCOMPtr<nsIRunnable>> runnables;
> +  runnables.SwapElements(mQueuedRunnables);
> +  MOZ_ASSERT(mQueuedRunnables.IsEmpty());
> +
> +  for (auto event = runnables.begin(); event != runnables.end(); ++event) {

auto event : runnables ?

or

nsIRunnable * event : runnables ? (not sure this is OK)

Comment 146

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

Thanks for the review!
During my tests on m3 patches these two days, I found there are three state update messages need to move to PBackground as well to ensure that they arrived before OnStopRequest.
They are all tracking protection / safe browsing related states.
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/PHttpChannel.ipdl#147,151,172
I'll put the modification in part 5.

Comment 147

[Matthew N. [:MattN]]

Comment on attachment 8871241 [details] [diff] [review]
trackingProtection_tour_workaround.patch

Review of attachment 8871241 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #142)
> Created attachment 8871241 [details] [diff] [review]
> trackingProtection_tour_workaround.patch
> 
> Hi @MattN,
> 
> browser_trackingProtection_tour.js will be frequently failed on try server
> after off-main-threading HTTP channel. The test case will failed in
> |test_unblock_target|, at |testTargetAvailability(true);| after
> showMenuPromise.
> 
> The tracking protection status will be updated after nsHttpChannel.asyncOpen
> on chrome process, this is asynchronously triggered by inserting the tracker
> iframe. This is not guaranteed to be happened before the |showMenuPromise|.
> 
> Before my patch, all the HTTP channel IPC run on main thread. It's highly
> likely that it creates an implicit execution order guarantee in this test
> case. However this will not be guaranteed after my off-main-thread work.
> 
> Here is a workaround I found can make try server happy but it uses flaky
> timeout. One proper way to fix it is to wait for "http-on-opening-request"
> on chrome process before "showMenuPromise", however I don't know how to
> integrate with this test case since the |checkToggleTarget| runs in the
> scope of addon.

It should be running in the browser's own scope. Maybe you are seeing the mochitest-related extension(s) but I think they may be misleading you.

> It'll be great if you can tell me how to do it, Or we can
> land this workaround and file a follow-up bug to fix this test case later on.

Less flaky solutions:
1) Add the "http-on-opening-request" observer… it should work fine in checkToggleTarget which runs in the parent process (as long as you don't do it in a content task function)
2) Wait for the iframe load in the content process by adding a load event listener after `doc.body.insertBefore(iframe, doc.body.firstChild);`
3) Use `yield BrowserTestUtils.waitForCondition(() => …)` to have the test wait for the correct state.

Comment 148

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

Comment on attachment 8871242 [details] [diff] [review]
(For spliter review)  Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

Review of attachment 8871242 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ +71,5 @@
>    // Initiate the creation of the PBckground IPC channel.
>    // Return false if failed.
>    bool CreateBackgroundChannel();
>  
> +  bool IsWaitingOnStartRequest();

following comment added:
// Check OnStartRequest is sent by parent process over main thread IPC
// but not yet received on child process.
// return true before RecvOnStartRequestSent is invoked.
// return false if RecvOnStartRequestSent is invoked but not
// OnStartRequestReceived.
// return true after both RecvOnStartRequestSend and OnStartRequestReceived
// are invoked.

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ +132,5 @@
> +  runnables.SwapElements(mQueuedRunnables);
> +  MOZ_ASSERT(mQueuedRunnables.IsEmpty());
> +
> +  for (auto event = runnables.begin(); event != runnables.end(); ++event) {
> +    (*event)->Run();

following comment added.
// Note: these runnables call Recv* methods on HttpBackgroundChannelChild
// but not the Process* methods on HttpChannelChild.

@@ +261,5 @@
> +  if (NS_WARN_IF(!mChannelChild)) {
> +    return IPC_OK();
> +  }
> +
> +  if (mStartSent && !mStartReceived) {

Yes it should be. fixed.

@@ +368,5 @@
> +    RefPtr<HttpBackgroundChannelChild> self = this;
> +    mQueuedRunnables.AppendElement(
> +        NS_NewRunnableFunction([self]() {
> +          MOZ_ASSERT(NS_IsMainThread());
> +          MOZ_ASSERT(self);

`MOZ_ASSERT(self);` is removed.

::: netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ +67,5 @@
>  
>  HttpBackgroundChannelParent::HttpBackgroundChannelParent()
>    : mIPCOpened(true)
>    , mBackgroundThread(NS_GetCurrentThread())
> +

extra line removed.

@@ +157,5 @@
> +
> +    return NS_SUCCEEDED(rv);
> +  }
> +
> +  AssertIsOnBackgroundThread();

I was trying to use the assertion as an inline document on what code runs on what thread, so that people can have more confident on the threading model even if they miss the if statement while reading the code. 
I can remove it if it is excessive.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1562,5 @@
>                                     uint32_t aCount)
>  {
> +  LOG(("HttpChannelParent::OnDataAvailable [this=%p aRequest=%p offset=%" PRIu64
> +       " count=%" PRIu32 "]\n", this, aRequest, aOffset, aCount));
> +  MOZ_ASSERT(NS_IsMainThread());

Yes, this will be removed by bug 1357689.

Comment 149

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

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

https://reviewboard.mozilla.org/r/139714/#review145572

> definitely is.  I'd rather do this after you iterate the runnables local copy array to make sure nothing new has been posted

`MOZ_ASSERT(mQueuedRunnables.IsEmpty());` is moved after the for-loop.

> auto event : runnables ?
> 
> or
> 
> nsIRunnable * event : runnables ? (not sure this is OK)

done. change to `auto event : runnables`.

Comment 150

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115244/diff/5-6/

Comment 151

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

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

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

Comment 152

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

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

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

Comment 153

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

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #147)
> 
> Less flaky solutions:
> 1) Add the "http-on-opening-request" observer… it should work fine in
> checkToggleTarget which runs in the parent process (as long as you don't do
> it in a content task function)
Thanks, I tried this approach again today and it works. Not sure what goes wrong yesterday.

Comment 154

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

Attachment: Bug 1015466 - Part 5, move tracking protection and safe browsing state update IPC to PBackground.

NotifyTrackingProtectionDisabled/NotifyTrackingResource/SetClassifierMatchedInfo updates tracking protection and safe browsing states from parent to child.
They should at least handled by child process before OnStopRequest. Move them to PHttpBackgroundChannel can ensure that.

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

Comment 155

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

Attachment: Bug 1015466 - Part 6, wait "http-on-opening-request" after tracker iframe is inserted.

The tracking protection status will be updated after nsHttpChannel.asyncOpen on chrome process and it is asynchronously triggered by inserting the tracker iframe.
This is not guaranteed to be happened before the |showMenuPromise|.
Waiting for "http-on-opening-request" on chrome process before "showMenuPromise" is invoked can ensure the tracking protection status is updated when the menu is shown.

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

Comment 156

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

Comment on attachment 8871606 [details]
Bug 1015466 - Part 6, wait "http-on-opening-request" after tracker iframe is inserted.

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

Comment 157

[Honza Bambas (:mayhemer)]

Comment on attachment 8871605 [details]
Bug 1015466 - Part 5, move tracking protection and safe browsing state update IPC to PBackground.

https://reviewboard.mozilla.org/r/143084/#review147282

Comment 158

[Matthew N. [:MattN]]

Comment on attachment 8871606 [details]
Bug 1015466 - Part 6, wait "http-on-opening-request" after tracker iframe is inserted.

https://reviewboard.mozilla.org/r/143086/#review148224

Comment 159

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

Comment on attachment 8840805 [details]
Bug 1015466 - Part 0, fix warning/error covered by unified build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115240/diff/3-4/

Comment 160

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

Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115242/diff/5-6/

Comment 161

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

Comment on attachment 8864473 [details]
Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136154/diff/3-4/

Comment 162

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

Comment on attachment 8840807 [details]
Bug 1015466 - Part 3, PHttpBackgroundChannel lifecycle management.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115244/diff/6-7/

Comment 163

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

Comment on attachment 8868106 [details]
Bug 1015466 - Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139714/diff/3-4/

Comment 164

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

Comment on attachment 8871605 [details]
Bug 1015466 - Part 5, move tracking protection and safe browsing state update IPC to PBackground.

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

Comment 165

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

Comment on attachment 8871606 [details]
Bug 1015466 - Part 6, wait "http-on-opening-request" after tracker iframe is inserted.

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

Comment 166

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f8ea2153c8
Part 0, fix warning/error covered by unified build. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/dc7ed794a3ea
Part 1, skeleton of PHttpBackgroundChannel. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/272dbc46892e
Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/3814e17ae85e
Part 3, PHttpBackgroundChannel lifecycle management. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/7ea4def6d4f0
Part 4, implement OnStartRequestSent/OnStopRequest/OnTransportAndData/OnDiversion on background channel. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/c4116710d7f4
Part 5, move tracking protection and safe browsing state update IPC to PBackground. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/12783b5a0fca
Part 6, wait "http-on-opening-request" after tracker iframe is inserted. r=MattN

Comment 167

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2f8ea2153c8
https://hg.mozilla.org/mozilla-central/rev/dc7ed794a3ea
https://hg.mozilla.org/mozilla-central/rev/272dbc46892e
https://hg.mozilla.org/mozilla-central/rev/3814e17ae85e
https://hg.mozilla.org/mozilla-central/rev/7ea4def6d4f0
https://hg.mozilla.org/mozilla-central/rev/c4116710d7f4
https://hg.mozilla.org/mozilla-central/rev/12783b5a0fca