1343741 - Label runnables under netwerk/base/

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Kershaw Chang [:kershaw]]

Please see https://wiki.mozilla.org/Quantum/DOM.

This could include files below.
 - netwerk/base/ProxyAutoConfig.cpp
 - netwerk/base/nsInputStreamPump.cpp
 - netwerk/base/nsAsyncRedirectVerifyHelper.cpp
 - netwerk/base/nsBaseChannel.cpp
 - netwerk/base/nsPACMan.cpp

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It would be great to get the stuff in nsAsyncRedirectVerifyHelper.cpp labeled. I see that come up pretty often in testing.

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Part1: Label runnables in nsBaseChannel

Summary:
1. Make nsBaseChannel inherit from NeckoTargetHolder.
2. Change FtpChannelChild due to (1) and also don't use SystemGroup to dispatch nsFtpChildAsyncAlert runnable since nsIPrompt may touch js script.
3. Use NeckoTargetHolder::Dispatch to dispatch runnables in nsBaseChannel.
   Note that RedirectRunnable should be on main thread, so I changed the code from NS_DispatchToCurrentThread to NeckoTargetHolder::Dispatch.

Thanks.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8879906 [details] [diff] [review]
Part1: Label runnables in nsBaseChannel

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

::: netwerk/base/nsBaseChannel.cpp
@@ +991,5 @@
> +void
> +nsBaseChannel::SetupNeckoTarget()
> +{
> +  mNeckoTarget =
> +    nsContentUtils::GetEventTargetByLoadInfo(mLoadInfo, TaskCategory::Other);

why not ::Network ?

Comment 4

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 8879906 [details] [diff] [review]
> Part1: Label runnables in nsBaseChannel
> 
> Review of attachment 8879906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsBaseChannel.cpp
> @@ +991,5 @@
> > +void
> > +nsBaseChannel::SetupNeckoTarget()
> > +{
> > +  mNeckoTarget =
> > +    nsContentUtils::GetEventTargetByLoadInfo(mLoadInfo, TaskCategory::Other);
> 
> why not ::Network ?

Because nsBaseChannel's subclasses, such as nsDataChannel and nsFileChannel, seem to have nothing to do with networking.
I think TaskCategory::Other should be fine.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Part2: Label nsAsyncRedirectVerifyHelper

Summary:
1. Pass a labeled main thread event target to nsAsyncRedirectVerifyHelper::Init.
2. Use mainThreadEventTarget to dispatch runnables.

Thanks.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8880297 [details] [diff] [review]
Part2: Label nsAsyncRedirectVerifyHelper

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

::: netwerk/base/nsAsyncRedirectVerifyHelper.cpp
@@ +73,5 @@
>           "oldChan=%p newChan=%p", oldChan, newChan));
>      mOldChan           = oldChan;
>      mNewChan           = newChan;
>      mFlags             = flags;
> +    mCallbackEventTarget = NS_IsMainThread() && mainThreadEventTarget

why are you adding |NS_IsMainThread()| to the condition?

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> Comment on attachment 8880297 [details] [diff] [review]
> Part2: Label nsAsyncRedirectVerifyHelper
> 
> Review of attachment 8880297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsAsyncRedirectVerifyHelper.cpp
> @@ +73,5 @@
> >           "oldChan=%p newChan=%p", oldChan, newChan));
> >      mOldChan           = oldChan;
> >      mNewChan           = newChan;
> >      mFlags             = flags;
> > +    mCallbackEventTarget = NS_IsMainThread() && mainThreadEventTarget
> 
> why are you adding |NS_IsMainThread()| to the condition?

Because the original code here is "mCallbackEventTarget = GetCurrentThreadEventTarget()". I think that means that mCallbackEventTarget is not always the main thread target, so adding NS_IsMainThread() here to make sure we really want to dispatch to main thread.

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Part3: Label runnables in nsPACMan

Summary:
 - Add aMainThreadTarget parameter in nsIProtocolProxyService::asyncResolve
 - Make nsPACMan inherit NeckoTargetHolder

Please take a look. Thanks.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Part4: Label the timer in PACResolver

Summary:
 - Pass the necko target to ProxyAutoConfig and use it to label the timer.

Thanks.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8881816 [details] [diff] [review]
Part3: Label runnables in nsPACMan

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

What is this for when proxy resolution to my knowledge only executes on the parent thread?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +370,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsCOMPtr<nsIEventTarget> target = mParent->GetWindow()
> +      ? mParent->GetWindow()->EventTargetFor(TaskCategory::Other)

why not ::Network?

::: netwerk/base/nsPACMan.cpp
@@ +442,5 @@
> +    nsCOMPtr<nsIRunnable> runnable =
> +      NewRunnableMethod("nsPACMan::StartLoading", this, &nsPACMan::StartLoading);
> +    nsresult rv = NS_IsMainThread()
> +      ? Dispatch(runnable.forget())
> +      : GetCurrentThreadEventTarget()->Dispatch(runnable.forget());

I more and more see similar constructions (ismain ? maintrg->disp() : currect->disp()).  Could you think of some helper for this?  In a followup bug, of course :)

::: netwerk/base/nsProtocolProxyService.cpp
@@ +1394,5 @@
>  
>  NS_IMETHODIMP
>  nsProtocolProxyService::AsyncResolve(nsISupports *channelOrURI, uint32_t flags,
>                                       nsIProtocolProxyCallback *callback,
> +                                     nsIEventTarget *mainThreadEventTarget,

both unused??

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2931,5 @@
>    nsresult rv;
>    rv = pps->AsyncResolve(mHttpChannel,
>                           nsIProtocolProxyService::RESOLVE_PREFER_HTTPS_PROXY |
>                           nsIProtocolProxyService::RESOLVE_ALWAYS_TUNNEL,
> +                         this, nullptr, getter_AddRefs(mCancelable));

this is parent process code?

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8881848 [details] [diff] [review]
Part4: Label the timer in PACResolver

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

To be honest, the same question here.

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Comment on attachment 8881816 [details] [diff] [review]
> Part3: Label runnables in nsPACMan
> 
> Review of attachment 8881816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is this for when proxy resolution to my knowledge only executes on the
> parent thread?
> 

I only see one place that calls asyncResolve in child process is at [1] and there is no assertion like "MOZ_ASSERT(XRE_IsParentProcess())" in our code. That's why I still tried to label runnables in nsPACMan.

[1] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#374


> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +370,5 @@
> >      return NS_ERROR_FAILURE;
> >    }
> >  
> > +  nsCOMPtr<nsIEventTarget> target = mParent->GetWindow()
> > +      ? mParent->GetWindow()->EventTargetFor(TaskCategory::Other)
> 
> why not ::Network?
> 

The definition of TaskCategory::Network [2] said this is about data coming from network. If I am right, most of the time that proxy resolution doesn't send data out? Could you correct me if I am wrong? Thanks.

[2] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/xpcom/threads/TaskCategory.h#18-19

> ::: netwerk/base/nsPACMan.cpp
> @@ +442,5 @@
> > +    nsCOMPtr<nsIRunnable> runnable =
> > +      NewRunnableMethod("nsPACMan::StartLoading", this, &nsPACMan::StartLoading);
> > +    nsresult rv = NS_IsMainThread()
> > +      ? Dispatch(runnable.forget())
> > +      : GetCurrentThreadEventTarget()->Dispatch(runnable.forget());
> 
> I more and more see similar constructions (ismain ? maintrg->disp() :
> currect->disp()).  Could you think of some helper for this?  In a followup
> bug, of course :)
> 

Sure. I'll file a follow-up.

> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ +1394,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsProtocolProxyService::AsyncResolve(nsISupports *channelOrURI, uint32_t flags,
> >                                       nsIProtocolProxyCallback *callback,
> > +                                     nsIEventTarget *mainThreadEventTarget,
> 
> both unused??
> 

Sorry about that. This is meant to be passed to "SetupPACThread()".

> ::: netwerk/protocol/websocket/WebSocketChannel.cpp
> @@ +2931,5 @@
> >    nsresult rv;
> >    rv = pps->AsyncResolve(mHttpChannel,
> >                           nsIProtocolProxyService::RESOLVE_PREFER_HTTPS_PROXY |
> >                           nsIProtocolProxyService::RESOLVE_ALWAYS_TUNNEL,
> > +                         this, nullptr, getter_AddRefs(mCancelable));
> 
> this is parent process code?

Yes, it is.
Unfortunately, the interface of AsyncResolve changed, so I have to also do this.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #12)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > Comment on attachment 8881816 [details] [diff] [review]
> > Part3: Label runnables in nsPACMan
> > 
> > Review of attachment 8881816 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > What is this for when proxy resolution to my knowledge only executes on the
> > parent thread?
> > 
> 
> I only see one place that calls asyncResolve in child process is at [1] and
> there is no assertion like "MOZ_ASSERT(XRE_IsParentProcess())" in our code.
> That's why I still tried to label runnables in nsPACMan.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 17ebac68112bd635b458e831670c1e506ebc17ad/media/webrtc/signaling/src/
> peerconnection/PeerConnectionMedia.cpp#374


Ah... webrtc :((((  I'm not sure this even works.  PACman needs access to the profile (preferences)


> 
> 
> > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> > @@ +370,5 @@
> > >      return NS_ERROR_FAILURE;
> > >    }
> > >  
> > > +  nsCOMPtr<nsIEventTarget> target = mParent->GetWindow()
> > > +      ? mParent->GetWindow()->EventTargetFor(TaskCategory::Other)
> > 
> > why not ::Network?
> > 
> 
> The definition of TaskCategory::Network [2] said this is about data coming
> from network. If I am right, most of the time that proxy resolution doesn't
> send data out? Could you correct me if I am wrong? Thanks.
> 
> [2]
> http://searchfox.org/mozilla-central/rev/
> 17ebac68112bd635b458e831670c1e506ebc17ad/xpcom/threads/TaskCategory.h#18-19

Really a detailed definition :D  This hugely blocks data delivery!  IMO it's then part of it.  But I'm not sure how exactly the category is defined and handled.  We should talk to those who invented it (I expect both the definition and the handling will need some adjustments)

> > ::: netwerk/base/nsProtocolProxyService.cpp
> > @@ +1394,5 @@
> > >  
> > >  NS_IMETHODIMP
> > >  nsProtocolProxyService::AsyncResolve(nsISupports *channelOrURI, uint32_t flags,
> > >                                       nsIProtocolProxyCallback *callback,
> > > +                                     nsIEventTarget *mainThreadEventTarget,
> > 
> > both unused??
> > 
> 
> Sorry about that. This is meant to be passed to "SetupPACThread()".
> 
> > ::: netwerk/protocol/websocket/WebSocketChannel.cpp
> > @@ +2931,5 @@
> > >    nsresult rv;
> > >    rv = pps->AsyncResolve(mHttpChannel,
> > >                           nsIProtocolProxyService::RESOLVE_PREFER_HTTPS_PROXY |
> > >                           nsIProtocolProxyService::RESOLVE_ALWAYS_TUNNEL,
> > > +                         this, nullptr, getter_AddRefs(mCancelable));
> > 
> > this is parent process code?
> 
> Yes, it is.
> Unfortunately, the interface of AsyncResolve changed, so I have to also do
> this.

Can you check that webrtc calls to proxy resolution actually works?  Maybe we don't need this at all.

Comment 14

[Kershaw Chang [:kershaw]]

> Can you check that webrtc calls to proxy resolution actually works?  Maybe
> we don't need this at all.

I can confirm that proxy resolution really works in child process.

I think we still need this.

Comment 15

[Kershaw Chang [:kershaw]]

> > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> > > @@ +370,5 @@
> > > >      return NS_ERROR_FAILURE;
> > > >    }
> > > >  
> > > > +  nsCOMPtr<nsIEventTarget> target = mParent->GetWindow()
> > > > +      ? mParent->GetWindow()->EventTargetFor(TaskCategory::Other)
> > > 
> > > why not ::Network?
> > > 
> > 
> > The definition of TaskCategory::Network [2] said this is about data coming
> > from network. If I am right, most of the time that proxy resolution doesn't
> > send data out? Could you correct me if I am wrong? Thanks.
> > 
> > [2]
> > http://searchfox.org/mozilla-central/rev/
> > 17ebac68112bd635b458e831670c1e506ebc17ad/xpcom/threads/TaskCategory.h#18-19
> 
> Really a detailed definition :D  This hugely blocks data delivery!  IMO it's
> then part of it.  But I'm not sure how exactly the category is defined and
> handled.  We should talk to those who invented it (I expect both the
> definition and the handling will need some adjustments)
> 

Hi Bill,

Could you explain more about when should we use TaskCategory::Network [1] or not?
What does "Data from the network" actually mean?
It seems like using TaskCategory::Network at [2] has nothing to do with network?

Thanks.

[1] http://searchfox.org/mozilla-central/source/xpcom/threads/TaskCategory.h#18-19
[2] http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/parser/html/nsHtml5TreeOpExecutor.cpp#246

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kershaw Chang [:kershaw] from comment #15)
> Could you explain more about when should we use TaskCategory::Network [1] or
> not?
> What does "Data from the network" actually mean?
> It seems like using TaskCategory::Network at [2] has nothing to do with
> network?

Eventually, I would like us to be able to put runnables with different TaskCategories in different event queues. Presumably we would have some way of prioritizing one queue over another for performance. For this to work, events with different TaskCategories would have to have no ordering dependencies.

The Network category is roughly meant for runnables where new data is coming in from the network. The most obvious example is for PHttpBackgroundChannel, especially OnTransportAndData (although I guess this will be off the main thread soon and it won't matter). I didn't notice the HTML parser case when I reviewed that patch. It doesn't really seem to fit well.

In any case, if we wanted to actually use a separate queue for a given category, we would have to audit all the runnables in that category. We're not going to just flip a switch without looking. But it does make things a little easier for the future if people do roughly the right thing.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Bill McCloskey (:billm) from comment #16)
> (In reply to Kershaw Chang [:kershaw] from comment #15)
> > Could you explain more about when should we use TaskCategory::Network [1] or
> > not?
> > What does "Data from the network" actually mean?
> > It seems like using TaskCategory::Network at [2] has nothing to do with
> > network?
> 
> Eventually, I would like us to be able to put runnables with different
> TaskCategories in different event queues. Presumably we would have some way
> of prioritizing one queue over another for performance. For this to work,
> events with different TaskCategories would have to have no ordering
> dependencies.
> 
> The Network category is roughly meant for runnables where new data is coming
> in from the network. The most obvious example is for PHttpBackgroundChannel,
> especially OnTransportAndData (although I guess this will be off the main
> thread soon and it won't matter). I didn't notice the HTML parser case when
> I reviewed that patch. It doesn't really seem to fit well.
> 
> In any case, if we wanted to actually use a separate queue for a given
> category, we would have to audit all the runnables in that category. We're
> not going to just flip a switch without looking. But it does make things a
> little easier for the future if people do roughly the right thing.

Thanks Bill for clarification, it makes much more sense now.  I believe the case that triggered this discussion should fall to the ::Network category, since it's part of the chain of operations we need to do to start getting the data.  

If the final goal is to have such runnables on the same queue and give them that way a priority, this one should be there.  There are few main thread loops we do (there is no plan to avoid them in the near future) before we start receiving the data.  These loops usually just push the process a bit further.  Though, one of the reasons why it's main thread is that we may call to various "on-http-*" observer notifications that may be implemented in JS.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8881816 [details] [diff] [review]
Part3: Label runnables in nsPACMan

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

With the fix for nsProtocolProxyService::AsyncResolve arguments, this is r+

Comment 19

[Kershaw Chang [:kershaw]]

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

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Part1: Label runnables in nsBaseChannel, r=mayhemer

Rebase and carry r+.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Part2: Label nsAsyncRedirectVerifyHelper, r=mayhemer

Carry r+.

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: Part3: Label runnables in nsPACMan, r=mayhemer

Carry r+.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Part4: Label the timer in PACResolver, r=mayhemer

Carry r+.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8449df7f3b6c
Label runnables in nsBaseChannel, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d1e0d2b9edc
Part2: Label nsAsyncRedirectVerifyHelper, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/beaf8abcee0b
Part3: Pass labelled event target to nsIProtocolProxyService::AsyncResolve, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/9068daebb346
Part4: Label the timer in PACResolver, r=mayhemer

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8449df7f3b6c
https://hg.mozilla.org/mozilla-central/rev/5d1e0d2b9edc
https://hg.mozilla.org/mozilla-central/rev/beaf8abcee0b
https://hg.mozilla.org/mozilla-central/rev/9068daebb346