Closed Bug 1015466 Opened 5 years ago Closed 2 years ago

Sending HTTP OnDataAvailable over PBackground IPC

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
e10s + ---
firefox55 --- fixed

People

(Reporter: khuey, Assigned: schien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active][necko-quantum][PBg-HTTP-M2])

Attachments

(10 files, 30 obsolete files)

59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
72.94 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
389.15 KB, image/png
Details
44.01 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
MattN
: review+
Details
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.
Alex is actively working on this.
Assignee: nobody → atifrea
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.
Attachment #8460343 - Flags: review?(mrbkap)
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.
Attachment #8460343 - Flags: review?(mrbkap) → feedback+
Attachment #8460343 - Attachment is obsolete: true
Attachment #8464885 - Flags: review?(mrbkap)
Sorry. This is the correct second version of the patch (I messed up the previous one).
Attachment #8464885 - Attachment is obsolete: true
Attachment #8464885 - Flags: review?(mrbkap)
Attachment #8464897 - Flags: review?(mrbkap)
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.
Attachment #8464897 - Flags: review?(mrbkap)
Attachment #8464897 - Attachment is obsolete: true
Attachment #8470318 - Flags: review?(mrbkap)
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.
Attachment #8470318 - Flags: review?(mrbkap) → feedback+
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 :)
Attachment #8470318 - Attachment is obsolete: true
Attachment #8475599 - Flags: feedback?(sworkman)
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.
Attachment #8475599 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #8475599 - Attachment is obsolete: true
Attachment #8482970 - Flags: feedback?(sworkman)
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.
Attachment #8482970 - Flags: feedback?(sworkman) → feedback+
Depends on: 1035454
No longer depends on: PBackground
Depends on: PBackground
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 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.
Attached patch Part 1 - PHttpBackgroundChannel (obsolete) — Splinter Review
Create PHttpBackgroundChannel.ipdl on necko thread in content process.
Attached patch Part 1 - PHttpBackgroundChannel (obsolete) — Splinter Review
Skeleton of PHttpBackgroundChannel.
Attachment #8515481 - Attachment is obsolete: true
Attachment #8516990 - Flags: feedback?(sworkman)
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 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.
Attachment #8516990 - Flags: feedback?(sworkman) → feedback+
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?
Attachment #8516992 - Flags: feedback?(sworkman) → feedback+
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
Attached patch Part 1 - PHttpBackgroundChannel (obsolete) — Splinter Review
Allow |ProcessOnTransportAndData| to be invoked on main thread because of comment #21.
Attachment #8516990 - Attachment is obsolete: true
Allow retargeting failure because of comment #21.
Attachment #8516992 - Attachment is obsolete: true
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
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
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
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.
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.
Assignee: atifrea → schien
Attachment #8482970 - Attachment is obsolete: true
Attachment #8526428 - Attachment is obsolete: true
Attachment #8526429 - Attachment is obsolete: true
Attachment #8535151 - Flags: review?(sworkman)
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.
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.
(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 on attachment 8535151 [details] [diff] [review]
Part 1 - PHttpBackgroundChannel, v1

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

First patch looks sane to me.
Attachment #8535151 - Flags: review?(sworkman) → review+
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.
Attachment #8535152 - Flags: review?(sworkman) → review-
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.
Is there a new patch coming?
Flags: needinfo?(schien)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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)
Flags: needinfo?(schien)
rebase to latest m-c.
Attachment #8535151 - Attachment is obsolete: true
update according to review comment, rebase to latest m-c.
Attachment #8535152 - Attachment is obsolete: true
Attached patch xhr_onprogress_workaround.patch (obsolete) — Splinter Review
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.
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?
Flags: needinfo?(sworkman)
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.
Attached file MozReview Request: bz://1015466/schien (obsolete) —
/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/
Attachment #8594573 - Flags: review?(sworkman)
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/
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)
(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.
(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.
Flags: needinfo?(sworkman)
(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 :)
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
Attachment #8590201 - Attachment is obsolete: true
Attachment #8590203 - Attachment is obsolete: true
Attachment #8590208 - Attachment is obsolete: true
Attachment #8598408 - Flags: feedback?(sworkman)
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
Attachment #8594573 - Flags: review?(jduell.mcbugs)
Attachment #8594573 - Flags: review?(bent.mozilla)
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/
Attachment #8598408 - Attachment is obsolete: true
Attachment #8598408 - Flags: feedback?(sworkman)
I've fixed all the error found in try result, let's start the review process.
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!
Attachment #8594573 - Flags: review?(bent.mozilla) → review?(khuey)
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?
Attachment #8594573 - Flags: review?(sworkman)
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).
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.
Attachment #8594573 - Attachment is obsolete: true
Attachment #8594573 - Flags: review?(khuey)
Attachment #8594573 - Flags: review?(jduell.mcbugs)
Attachment #8618161 - Flags: review?(khuey)
Attachment #8618161 - Flags: review?(jduell.mcbugs)
Attachment #8618162 - Flags: review?(khuey)
Attachment #8618162 - Flags: review?(jduell.mcbugs)
Attachment #8618163 - Flags: review?(khuey)
Attachment #8618163 - Flags: review?(jduell.mcbugs)
Attachment #8618164 - Flags: review?(khuey)
Attachment #8618164 - Flags: review?(jduell.mcbugs)
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).
Jason, I'm back at work.  I can take the reviews if you want (despite those are in the st**id review board.)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(jduell.mcbugs)
Attachment #8618161 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attachment #8618162 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attachment #8618163 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attachment #8618164 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
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
Attachment #8618161 - Flags: review?(honzab.moz) → review+
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)
> +  }
Attachment #8618163 - Flags: review?(honzab.moz)
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
Attachment #8618164 - Flags: review?(honzab.moz)
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!
Attachment #8618162 - Flags: review?(honzab.moz) → review+
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....
Attachment #8618161 - Flags: review+
Is there anything more up to date than what is in this bug?
Flags: needinfo?(schien)
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.
Is there a plan for moving forward here?
Flags: needinfo?(jduell.mcbugs)
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).
Flags: needinfo?(jduell.mcbugs)
Attachment #8618163 - Attachment description: MozReview Request: Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,bent. → MozReview Request: Bug 1015466 - Part 0, fix build error for non-unified build. r=mayhammer.
Attachment #8618163 - Flags: review?(sworkman)
Attachment #8618163 - Flags: review?(bent.mozilla)
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/
Attachment #8618164 - Attachment description: MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=sworkman. → MozReview Request: Bug 1015466 - Part 1, create off-main-thread HTTP IPC Channel. r=sworkman,khuey.
Attachment #8618164 - Flags: review?(sworkman)
Attachment #8618164 - Flags: review?(khuey)
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 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/
Attachment #8618161 - Attachment description: MozReview Request: Bug 1015466 - waiting for channel open event explicitly. r=jduell. → MozReview Request: Bug 1015466 - Part 2, off-main-thread IPC for nsHttpChannel. r=mayhammer.
Attachment #8618161 - Flags: review?(jduell.mcbugs)
Attachment #8618161 - Flags: review?(honzab.moz)
Attachment #8618161 - Flags: review?(jduell.mcbugs)
Attachment #8618161 - Flags: review?(honzab.moz)
Flags: needinfo?(schien)
Attachment #8618163 - Flags: review?(sworkman)
Attachment #8618163 - Flags: review?(bent.mozilla)
Attachment #8618164 - Flags: review?(sworkman)
Attachment #8618164 - Flags: review?(khuey)
Attachment #8618162 - Attachment is obsolete: true
patch updated to latest m-c, build ok but not tested. will add notes later.
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?
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(jduell.mcbugs)
Attachment #8618161 - Flags: review?(honzab.moz)
Attachment #8618163 - Flags: review?(honzab.moz)
Attachment #8618164 - Flags: review?(honzab.moz)
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!
Attachment #8618163 - Flags: review?(honzab.moz) → review+
(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!
Attachment #8618164 - Attachment is obsolete: true
Attachment #8618164 - Flags: review?(honzab.moz)
Attachment #8715817 - Flags: review?(honzab.moz)
Attachment #8618161 - Attachment is obsolete: true
Attachment #8618161 - Flags: review?(honzab.moz)
Attachment #8715819 - Flags: review?(honzab.moz)
Whiteboard: [necko-backlog]
Rebased to something approximating master.
Attachment #8725424 - Attachment is obsolete: true
Are you actively working on this?
Flags: needinfo?(nfroyd)
Forgot to remove myself from assignee after handing off the work.
Assignee: schien → nobody
Nathan said he's not picking this up.
Flags: needinfo?(nfroyd)
(Note for me - review drafts in laptop)
this is an even bigger problem in the multiple run queue quantum view of the world
Summary: PBackground-ify Necko transport to off-main-thread consumers → PBackground-ify Necko transport to off-main-thread consumers (e10s breaks retargetable delivery)
Whiteboard: [necko-backlog] → [necko-next][necko-quantum]
tracking-e10s: --- → +
Priority: -- → P2
Assignee: nobody → schien
Keywords: perf
Keywords: perf
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
Attachment #8715817 - Flags: review?(honzab.moz) → review+
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
Attachment #8715819 - Flags: review?(honzab.moz) → feedback+
See Also: → 1320744
See Also: 1320744
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.
No longer blocks: 1320744
Blocks: PBg-HTTP
No longer blocks: QuantumDOM, Quantum
Attachment #8618163 - Attachment is obsolete: true
Attachment #8725425 - Attachment is obsolete: true
Attachment #8725707 - Attachment is obsolete: true
Attachment #8715817 - Attachment is obsolete: true
Attachment #8715819 - Attachment is obsolete: true
TODOs:
1) lifecycle handling while redirecting
2) OnStart/ODA/OnStop handling
3) channel diversion handling
4) service worker interception handling
Whiteboard: [necko-next][necko-quantum] → [necko-next][necko-quantum][PBg-HTTP-M2]
Whiteboard: [necko-next][necko-quantum][PBg-HTTP-M2] → [necko-active][necko-quantum][PBg-HTTP-M2]
Is there anything here that would require manual testing?
If so, could you please provide us some testing steps/scenarios?
Flags: qe-verify?
Flags: needinfo?(schien)
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
Flags: needinfo?(schien)
Flags: qe-verify? → qe-verify+
This bug will be focusing on building PHttpBackgroundChannel.ipdl over PBackground.
Summary: PBackground-ify Necko transport to off-main-thread consumers (e10s breaks retargetable delivery) → Sending HTTP OnDataAvailable over PBackground IPC
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.
(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.
Attachment #8860900 - Attachment is obsolete: true
Attachment #8860900 - Flags: review?(honzab.moz)
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.
(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).
Attachment #8840805 - Flags: review?(honzab.moz) → review?(dd.mozilla)
Attachment #8864473 - Flags: review?(honzab.moz) → review?(dd.mozilla)
Attachment #8840805 - Flags: review?(dd.mozilla) → review?(honzab.moz)
Attachment #8864473 - Flags: review?(dd.mozilla) → review?(honzab.moz)
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?
Attachment #8840805 - Flags: review?(honzab.moz) → review+
Comment on attachment 8840806 [details]
Bug 1015466 - Part 1, skeleton of PHttpBackgroundChannel.

https://reviewboard.mozilla.org/r/115242/#review140184
Attachment #8840806 - Flags: review?(honzab.moz) → review+
Attachment #8865494 - Attachment is obsolete: true
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.
Attachment #8864473 - Flags: review?(honzab.moz) → review+
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 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
Attachment #8840807 - Flags: review?(honzab.moz) → review+
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 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.
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.
The last part for actually move ODA/OnStopRequest/OnDiversion is coming.
(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.
(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.
(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!
Testing for all Quantum Network stuff is done by Platform QA in collaboration with Shih-Chiang Chien.
Flags: qe-verify+
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.
With the current implementation we no longer depend on bug 1364812 and bug 1362732.
No longer depends on: 1364812, 1362732
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.
Flags: needinfo?(schien)
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
Flags: needinfo?(schien)
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.
Attachment #8871241 - Flags: feedback?(MattN+bmo)
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 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)
Attachment #8868106 - Flags: review?(honzab.moz) → review+
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 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.
Attachment #8871241 - Flags: feedback?(MattN+bmo) → feedback-
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 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`.
(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 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
Attachment #8871605 - Flags: review?(honzab.moz) → review+
Attachment #8871241 - Attachment is obsolete: true
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
Attachment #8871606 - Flags: review?(MattN+bmo) → review+
Pushed by schien@mozilla.com:
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
Depends on: 1369878
Depends on: 1370211
See Also: → 1409553
No longer depends on: 1369632
You need to log in before you can comment on or make changes to this bug.