1343745 - Label runnables under netwerk/protocol/http

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P1)

Description

[Kershaw Chang [:kershaw]]

This should include files below.
 - netwerk/protocol/http/HttpBaseChannel.cpp
 - netwerk/protocol/http/HttpChannelChild.cpp
 - netwerk/protocol/http/HttpBaseChannel.h

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild

Summary:
 - Add |mDispatcher| in HttpBaseChannel's protected member
 - Dispatch runnables by |mDispatcher| or SystemGroup

Please take a look. Thanks!

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8851960 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild

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

won't be able to do it sooner than next week

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8851960 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild

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

didn't check it's all usage of dispatch to main thread, but overall seems OK.  I'd like SC to take a look, since it touches a code he is changing in bug 1320744.  That bug has landed just today, so this will definitely need a merge and probably another review after that.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +830,5 @@
>  
>  void
>  CopyComplete(void* aClosure, nsresult aStatus) {
>    // Called on the STS thread by NS_AsyncCopy
> +  MOZ_ASSERT(!NS_IsMainThread(), "Should only be called on the STS thread.");

then assert this is the sts thread and not just that it's not the main thread

Comment 4

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

Comment on attachment 8851960 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild

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

Yes, this patch definitely needs to rebase after bug 1320744 landed.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +262,5 @@
> +                          runnable.forget());
> +  } else {
> +    SystemGroup::Dispatch("HttpBaseChannel::ReleaseMainThreadOnlyReferences",
> +                          TaskCategory::Other,
> +                          runnable.forget());

Using SystemGourp to do proxy release should be enough. We should talk to @bkelly while he is in Taipei.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2112,3 @@
>  
> +  gNeckoChild->SetEventTargetForActor(this, eventTarget);
> +  mEventQ->RetargetDeliveryTo(eventTarget);

After bug 1320744, mNeckoTarget is introduced to store the event target for networking event. We can simply use the good old nsIEventTarget without storing another `mDispatcher`.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8851960 [details] [diff] [review]
> Label runnables in HttpBaseChannel and HttpChannelChild
> 
> Review of attachment 8851960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this patch definitely needs to rebase after bug 1320744 landed.
> 

Thanks for the comments.

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +262,5 @@
> > +                          runnable.forget());
> > +  } else {
> > +    SystemGroup::Dispatch("HttpBaseChannel::ReleaseMainThreadOnlyReferences",
> > +                          TaskCategory::Other,
> > +                          runnable.forget());
> 
> Using SystemGourp to do proxy release should be enough. We should talk to
> @bkelly while he is in Taipei.
> 

I thinks this is ok too.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +2112,3 @@
> >  
> > +  gNeckoChild->SetEventTargetForActor(this, eventTarget);
> > +  mEventQ->RetargetDeliveryTo(eventTarget);
> 
> After bug 1320744, mNeckoTarget is introduced to store the event target for
> networking event. We can simply use the good old nsIEventTarget without
> storing another `mDispatcher`.

I'll upload a patch for feedback later. Thanks.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild

Summary:
 - Use |GetNeckoTarget| to get a target for dispatching runnables


Thanks.

Comment 7

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

Comment on attachment 8852793 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild

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

I see `SystemGroup::Dispatch` might be called on parent process in this patch, but I don't know if it is safe to do that. If not, please isolate the code that might touch SystemGroup in HttpChannelChild and add corresponding assertion.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +239,5 @@
> +    new ProxyReleaseRunnable(Move(aArray));
> +  SystemGroup::Dispatch("DoReleaseMainThreadOnlyReferences",
> +                        TaskCategory::Other,
> +                        runnable.forget());
> +}

Just create a utility function `DispatchToSystemGroup(runnable, category)`.

@@ +265,5 @@
>    arrayToRelease.AppendElement(mProxyURI.forget());
>    arrayToRelease.AppendElement(mPrincipal.forget());
>    arrayToRelease.AppendElement(mTopWindowURI.forget());
>  
> +  DoReleaseMainThreadOnlyReferences(Move(arrayToRelease));

What would happen when this is run on parent process? nsHttpChannel is also derived from HttpBaseChannel.

@@ +921,5 @@
> +
> +    DebugOnly<nsresult> rv =
> +      neckoTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +    return;
> +  }

I'll prefer to create a proxy function to handle the thread-switching and make `EnsureUploadStreamIsCloneableComplete` protected. In addition, you can override the new function in HttpChannelChild so that we can make sure the labeling logic only needs to exist in HttpChannelChild.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +716,5 @@
> +
> +    rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
> +  } else {
> +    rv = NS_DispatchToCurrentThread(event);
> +  }

This is a utility function used by both parent and child process. Maybe need to refine the HttpAsyncAborter to pass the correct event target to this function.

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> Comment on attachment 8852793 [details] [diff] [review]
> Label runnables in HttpBaseChannel and HttpChannelChild
> 
> Review of attachment 8852793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see `SystemGroup::Dispatch` might be called on parent process in this
> patch, but I don't know if it is safe to do that. If not, please isolate the
> code that might touch SystemGroup in HttpChannelChild and add corresponding
> assertion.
> 

I've checked this with bevis, and it is totally safe using SystemGroup::Dispatch in parent. Please see [1] for more detail.

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/xpcom/threads/Dispatcher.cpp#227-231
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +239,5 @@
> > +    new ProxyReleaseRunnable(Move(aArray));
> > +  SystemGroup::Dispatch("DoReleaseMainThreadOnlyReferences",
> > +                        TaskCategory::Other,
> > +                        runnable.forget());
> > +}
> 
> Just create a utility function `DispatchToSystemGroup(runnable, category)`.
> 

I think we also need a name, like `DispatchToSystemGroup(name, runnable, category)`?

> @@ +265,5 @@
> >    arrayToRelease.AppendElement(mProxyURI.forget());
> >    arrayToRelease.AppendElement(mPrincipal.forget());
> >    arrayToRelease.AppendElement(mTopWindowURI.forget());
> >  
> > +  DoReleaseMainThreadOnlyReferences(Move(arrayToRelease));
> 
> What would happen when this is run on parent process? nsHttpChannel is also
> derived from HttpBaseChannel.
> 

As explained above.

> @@ +921,5 @@
> > +
> > +    DebugOnly<nsresult> rv =
> > +      neckoTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);
> > +    return;
> > +  }
> 
> I'll prefer to create a proxy function to handle the thread-switching and
> make `EnsureUploadStreamIsCloneableComplete` protected. In addition, you can
> override the new function in HttpChannelChild so that we can make sure the
> labeling logic only needs to exist in HttpChannelChild.
> 

I'll do it.

> ::: netwerk/protocol/http/HttpBaseChannel.h
> @@ +716,5 @@
> > +
> > +    rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
> > +  } else {
> > +    rv = NS_DispatchToCurrentThread(event);
> > +  }
> 
> This is a utility function used by both parent and child process. Maybe need
> to refine the HttpAsyncAborter to pass the correct event target to this
> function.

I'll see what I can do. Thanks.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild, v3

> > ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > @@ +239,5 @@
> > > +    new ProxyReleaseRunnable(Move(aArray));
> > > +  SystemGroup::Dispatch("DoReleaseMainThreadOnlyReferences",
> > > +                        TaskCategory::Other,
> > > +                        runnable.forget());
> > > +}
> > 
> > Just create a utility function `DispatchToSystemGroup(runnable, category)`.
> > 
> 
> I think we also need a name, like `DispatchToSystemGroup(name, runnable,
> category)`?
> 

Adding another helper function DispatchToSystemGroup which is only responsible for dispatching to system group seems unnecessary to me. Or maybe I completely get you wrong?

I still keep my original design in this patch.

> 
> > ::: netwerk/protocol/http/HttpBaseChannel.h
> > @@ +716,5 @@
> > > +
> > > +    rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
> > > +  } else {
> > > +    rv = NS_DispatchToCurrentThread(event);
> > > +  }
> > 
> > This is a utility function used by both parent and child process. Maybe need
> > to refine the HttpAsyncAborter to pass the correct event target to this
> > function.
> 
> I'll see what I can do. Thanks.

In this patch, I make AsyncCall be a virtual function and let HttpChannelChild override it. I think maybe it's better than adding another extra parameter.

Comment 10

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

Comment on attachment 8853324 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild, v3

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

overall lgtm!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +238,5 @@
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new ProxyReleaseRunnable(Move(aArray));
> +  SystemGroup::Dispatch("DoReleaseMainThreadOnlyReferences",
> +                        TaskCategory::Other,
> +                        runnable.forget());

Okay, we should directly use SystemGroup::Dispatch to replace the NS_DispatchToMainThread in the place you want to modify. Creating a specific function for ReleaseMainThreadOnlyReferences doesn't make sense to me.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +353,5 @@
>      DoApplyContentConversions(nsIStreamListener *aNextListener,
>                                nsIStreamListener **aNewNextListener);
>  
> +    // Proxy function to call EnsureUploadStreamIsCloneableComplete
> +    virtual void ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus);

How about `OnCopyComplete`?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1370,5 @@
>    }
>  
>    // The IPDL connection was torn down by a interception logic in
>    // CompleteRedirectSetup, and we need to call FinishInterceptedRedirect.
> +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();

add `MOZ_ASSERT(neckoTarget);` here

@@ +1951,5 @@
>  
>      RefPtr<InterceptStreamListener> streamListener =
>          new InterceptStreamListener(redirectedChannel, mListenerContext);
>  
> +    nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();

add `MOZ_ASSERT(neckoTarget);` here

@@ +2245,5 @@
> +  nsCOMPtr<nsIEventTarget> target = nullptr;
> +  if (dispatcher) {
> +    target = dispatcher->EventTargetFor(TaskCategory::Network);
> +  } else {
> +    target = SystemGroup::EventTargetFor(TaskCategory::Network);

no need to store event target for SystemGroup here based on the logic of GetNeckoTarget. I'll suggest to keep SetEventTarget as it is.

@@ +3114,5 @@
>      Unused << PHttpChannelChild::SendDeletingChannel();
>      return;
>    }
>  
> +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();

add `MOZ_ASSERT(neckoTarget);` here

@@ +3127,5 @@
> +HttpChannelChild::ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus)
> +{
> +  nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod<nsresult>(
> +    this, &HttpChannelChild::EnsureUploadStreamIsCloneableComplete, aStatus);
> +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();

add `MOZ_ASSERT(neckoTarget);` here

::: netwerk/protocol/http/HttpChannelChild.h
@@ +171,5 @@
> +  void ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus) override;
> +
> +  nsresult
> +  AsyncCall(void (HttpChannelChild::*funcPtr)(),
> +            nsRunnableMethod<HttpChannelChild> **retval = nullptr) override

Can we move the implementation to .cpp?

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #10)
> Comment on attachment 8853324 [details] [diff] [review]
> Label runnables in HttpBaseChannel and HttpChannelChild, v3
> 
> Review of attachment 8853324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall lgtm!
> 

Thanks for the feedback. I'll upload another patch later.

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +238,5 @@
> > +  nsCOMPtr<nsIRunnable> runnable =
> > +    new ProxyReleaseRunnable(Move(aArray));
> > +  SystemGroup::Dispatch("DoReleaseMainThreadOnlyReferences",
> > +                        TaskCategory::Other,
> > +                        runnable.forget());
> 
> Okay, we should directly use SystemGroup::Dispatch to replace the
> NS_DispatchToMainThread in the place you want to modify. Creating a specific
> function for ReleaseMainThreadOnlyReferences doesn't make sense to me.
> 

Done.

> ::: netwerk/protocol/http/HttpBaseChannel.h
> @@ +353,5 @@
> >      DoApplyContentConversions(nsIStreamListener *aNextListener,
> >                                nsIStreamListener **aNewNextListener);
> >  
> > +    // Proxy function to call EnsureUploadStreamIsCloneableComplete
> > +    virtual void ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus);
> 
> How about `OnCopyComplete`?
> 

We already have CopyComplete in HttpBaseChannel.cpp, so I prefer to stick at 'ProxyEnsureUploadStreamIsCloneableComplete'.
Please let me know if you still want to change the name.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1370,5 @@
> >    }
> >  
> >    // The IPDL connection was torn down by a interception logic in
> >    // CompleteRedirectSetup, and we need to call FinishInterceptedRedirect.
> > +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> 
> add `MOZ_ASSERT(neckoTarget);` here
> 

Done.

> @@ +1951,5 @@
> >  
> >      RefPtr<InterceptStreamListener> streamListener =
> >          new InterceptStreamListener(redirectedChannel, mListenerContext);
> >  
> > +    nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> 
> add `MOZ_ASSERT(neckoTarget);` here
> 

Done.

> @@ +2245,5 @@
> > +  nsCOMPtr<nsIEventTarget> target = nullptr;
> > +  if (dispatcher) {
> > +    target = dispatcher->EventTargetFor(TaskCategory::Network);
> > +  } else {
> > +    target = SystemGroup::EventTargetFor(TaskCategory::Network);
> 
> no need to store event target for SystemGroup here based on the logic of
> GetNeckoTarget. I'll suggest to keep SetEventTarget as it is.
> 

I'll do it as you suggested.
However, if we are not able to get a dispatcher here, that means we cannot set the correct event target before sending IPC constructor message.

I've tried to use SystemGroup's event target when failed to get a one from the load info, but I encountered an assertion (see [1]) at [2]. Anyway, I'll ask billm about this.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=88745083&repo=try&lineNumber=13995
[2] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/threads/Dispatcher.h#83

> @@ +3114,5 @@
> >      Unused << PHttpChannelChild::SendDeletingChannel();
> >      return;
> >    }
> >  
> > +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> 
> add `MOZ_ASSERT(neckoTarget);` here
> 

Done.

> @@ +3127,5 @@
> > +HttpChannelChild::ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus)
> > +{
> > +  nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod<nsresult>(
> > +    this, &HttpChannelChild::EnsureUploadStreamIsCloneableComplete, aStatus);
> > +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> 
> add `MOZ_ASSERT(neckoTarget);` here
> 

Done.

> ::: netwerk/protocol/http/HttpChannelChild.h
> @@ +171,5 @@
> > +  void ProxyEnsureUploadStreamIsCloneableComplete(nsresult aStatus) override;
> > +
> > +  nsresult
> > +  AsyncCall(void (HttpChannelChild::*funcPtr)(),
> > +            nsRunnableMethod<HttpChannelChild> **retval = nullptr) override
> 
> Can we move the implementation to .cpp?

Done.
Yes, we can.

Comment 12

[Kershaw Chang [:kershaw]]

Hi Bill,

Please take a look at [0].
When it is not able to get a dispatcher from load info, I try to assign SystemGroup's event target to HttpChannelChild. But, I hit the assertion [1] at [2].
I think using SystemGroup's event target here should be totally fine? Is there anything I missed? 

Thanks.


[0] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/netwerk/protocol/http/HttpChannelChild.cpp#2236
[1] https://treeherder.mozilla.org/logviewer.html#?job_id=88745083&repo=try&lineNumber=13995
[2] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/threads/Dispatcher.h#83

Comment 13

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

Hi Kershaw,
When you use SystemGroup, you're not allowed to touch a page's DOM or run content JS. Looking at the stack you posted in [1], it looks like a ScrolledAreaEvent is being dispatched inside a runnable labeled with SystemGroup.

If you want to use SystemGroup there, I think you'll need to figure why a load that can affect a tab is returning null from GetDispatcherByLoadInfo.

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to Bill McCloskey (:billm) from comment #13)
> Hi Kershaw,
> When you use SystemGroup, you're not allowed to touch a page's DOM or run
> content JS. Looking at the stack you posted in [1], it looks like a
> ScrolledAreaEvent is being dispatched inside a runnable labeled with
> SystemGroup.
> 
> If you want to use SystemGroup there, I think you'll need to figure why a
> load that can affect a tab is returning null from GetDispatcherByLoadInfo.

Thanks for your feedback.

After digging more deeply, I found the reason that GetDispatcherByLoadInfo returns a null dispatcher. Took a look at [1], and I found it is possible that a document could return a null DocGroup. In this case [2], this document is created at [3] and has no window so that I cannot get a dispatcher from it.

About the reason that runnable labeled with SystemGroup touches content, it's because this channel is in a load group with an observer pointing to a doc loader. When a channel's |OnStopRequest| is called, the doc loader could touch content and trigger some state change events.

What do you think we can do in this case?


[1] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/base/nsContentUtils.cpp#10173
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=88745083&repo=try&lineNumber=13995
[3] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/xbl/nsXBLService.cpp#1047

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild, v4

Summary:
 - Address the previous comments
 - Keep |HttpChannelChild::SetEventTarget| and |HttpChannelChild::GetNeckoTarget| untouched

It looks like we cannot label runnables with SystemGroup when we fail to get a dispatcher from load info. I think we should land this patch first and file another bug to address the SystemGroup issue.

What do you think?

Comment 16

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

Comment on attachment 8856799 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild, v4

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

r+ with some comments.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +830,5 @@
> +  nsCOMPtr<nsIEventTarget> sts =
> +    do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> +  bool result = false;
> +  sts->IsOnCurrentThread(&result);
> +  MOZ_ASSERT(result, "Should only be called on the STS thread.");

I'm thinking about creating a static function in nsStreamTransportService.h for checking current thread, so that we can easily put the assertion in OnCopyComplete as well.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1964,5 @@
> +    DebugOnly<nsresult> rv =
> +      neckoTarget->Dispatch(
> +        new OverrideRunnable(this, redirectedChannel, streamListener,
> +                             mSynthesizedInput, mResponseHead),
> +        NS_DISPATCH_NORMAL);

nit: Add corresponding MOZ_ASSERT for checking rv, or use `Unused` if you don't care about the return value.

@@ +3147,5 @@
> +    this, &HttpChannelChild::EnsureUploadStreamIsCloneableComplete, aStatus);
> +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> +  MOZ_ASSERT(neckoTarget);
> +
> +  DebugOnly<nsresult> rv = neckoTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);

nit: Add corresponding MOZ_ASSERT for checking rv, or use `Unused` if you don't care about the return value.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +167,5 @@
>    virtual void DoNotifyListenerCleanup() override;
>  
>    NS_IMETHOD GetResponseSynthesized(bool* aSynthesized) override;
>  
> +  void OnCopyComplete(nsresult aStatus) override;

OnCopyComplete is in public in HttpBaseChannel, any particular reason we change it to protected in sub class?

Comment 17

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #16)
> Comment on attachment 8856799 [details] [diff] [review]
> Label runnables in HttpBaseChannel and HttpChannelChild, v4
> 
> Review of attachment 8856799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with some comments.
> 

Thanks for the review.

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +830,5 @@
> > +  nsCOMPtr<nsIEventTarget> sts =
> > +    do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> > +  bool result = false;
> > +  sts->IsOnCurrentThread(&result);
> > +  MOZ_ASSERT(result, "Should only be called on the STS thread.");
> 
> I'm thinking about creating a static function in nsStreamTransportService.h
> for checking current thread, so that we can easily put the assertion in
> OnCopyComplete as well.
> 

Great!

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1964,5 @@
> > +    DebugOnly<nsresult> rv =
> > +      neckoTarget->Dispatch(
> > +        new OverrideRunnable(this, redirectedChannel, streamListener,
> > +                             mSynthesizedInput, mResponseHead),
> > +        NS_DISPATCH_NORMAL);
> 
> nit: Add corresponding MOZ_ASSERT for checking rv, or use `Unused` if you
> don't care about the return value.
> 

Will do.

> @@ +3147,5 @@
> > +    this, &HttpChannelChild::EnsureUploadStreamIsCloneableComplete, aStatus);
> > +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> > +  MOZ_ASSERT(neckoTarget);
> > +
> > +  DebugOnly<nsresult> rv = neckoTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);
> 
> nit: Add corresponding MOZ_ASSERT for checking rv, or use `Unused` if you
> don't care about the return value.
> 

Will do.

> ::: netwerk/protocol/http/HttpChannelChild.h
> @@ +167,5 @@
> >    virtual void DoNotifyListenerCleanup() override;
> >  
> >    NS_IMETHOD GetResponseSynthesized(bool* aSynthesized) override;
> >  
> > +  void OnCopyComplete(nsresult aStatus) override;
> 
> OnCopyComplete is in public in HttpBaseChannel, any particular reason we
> change it to protected in sub class?

Thanks for catching this. It's a mistake.
OnCopyComplete should stay in public in HttpChannelChild.

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild, v5

Fix the previous comment from S.C.

Honza, do you want to take a look? Thanks.

Comment 19

[Kershaw Chang [:kershaw]]

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

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8857807 [details] [diff] [review]
Label runnables in HttpBaseChannel and HttpChannelChild, v5

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

Only problem I can see is the potential call of ~nsLoadGroup on the system group.  Someone more familiar with the event grouping should tell you if its worth the effort and release it (maybe only it) on a document or tab group somehow.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +256,5 @@
>    arrayToRelease.AppendElement(mTopWindowURI.forget());
>  
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new ProxyReleaseRunnable(Move(arrayToRelease));
> +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",

full name?

@@ +258,5 @@
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new ProxyReleaseRunnable(Move(arrayToRelease));
> +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",
> +                        TaskCategory::Other,
> +                        runnable.forget());

nsLoadGroup dtor may call via nsLoadGroup::Cancel on callbacks implemented in JS that may have undetermined effect.  not sure you can use the system group?

mCallbacks dtor is something out of the control too, but I would not bother about that one particular.

@@ +905,5 @@
>  
>  void
> +HttpBaseChannel::OnCopyComplete(nsresult aStatus)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess());

why? (a comment)

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +352,5 @@
>      MOZ_MUST_USE nsresult
>      DoApplyContentConversions(nsIStreamListener *aNextListener,
>                                nsIStreamListener **aNewNextListener);
>  
> +    virtual void OnCopyComplete(nsresult aStatus);

you don't like writing comments, do you? :))

please add some, what this is, what it does, who and when calls it.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +212,5 @@
>    arrayToRelease.AppendElement(mCacheKey.forget());
>  
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new ProxyReleaseRunnable(Move(arrayToRelease));
> +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",

"HttpChannelChild::ReleaseMainThreadOnlyReferences" ?

@@ +1377,5 @@
>    // CompleteRedirectSetup, and we need to call FinishInterceptedRedirect.
> +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> +  MOZ_ASSERT(neckoTarget);
> +
> +  DebugOnly<nsresult> rv =

Unused << ?  because I don't see this used even in debug builds

Comment 21

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

Sorry for the delay. This got lost in my queue somehow. Olli, see comment 14. It looks like we're creating an XBL document that has no docgroup, and it would be nice if it did. Do you think it makes sense to use the DocGroup of aBoundDocument?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

But XBL documents are shared.

XBL loading should probably use system group to ensure we never preempt it, no?

Comment 23

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

(In reply to Olli Pettay [:smaug] from comment #22)
> But XBL documents are shared.

Yeah, I guess you're right.

> XBL loading should probably use system group to ensure we never preempt it,
> no?

Well, using the SystemGroup is how we ended up with the assertion in comment 14. I think what might be happening is that, when the XBL document finishes loading, we fire OnStateChange for the main document, and that can't happen in the SystemGroup. Maybe the XBL document is in the main document's load group or something? I don't know much about how that works.

I suspect we probably don't need to label this at all. In cases when we do use XBL in content, the XBL should be cached. So this should be a pretty rare thing.

Comment 24

[Kershaw Chang [:kershaw]]

(In reply to Bill McCloskey (:billm) from comment #23)
> (In reply to Olli Pettay [:smaug] from comment #22)
> > But XBL documents are shared.
> 
> Yeah, I guess you're right.
> 
> > XBL loading should probably use system group to ensure we never preempt it,
> > no?
> 
> Well, using the SystemGroup is how we ended up with the assertion in comment
> 14. I think what might be happening is that, when the XBL document finishes
> loading, we fire OnStateChange for the main document, and that can't happen
> in the SystemGroup. Maybe the XBL document is in the main document's load
> group or something? I don't know much about how that works.
> 
> I suspect we probably don't need to label this at all. In cases when we do
> use XBL in content, the XBL should be cached. So this should be a pretty
> rare thing.

Thanks. I also think we don't have to label this.

I've filed another bug 1355688 to discuss the case when we can't get an labeled event target in HttpChannelChild.

Comment 25

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> Comment on attachment 8857807 [details] [diff] [review]
> Label runnables in HttpBaseChannel and HttpChannelChild, v5
> 
> Review of attachment 8857807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only problem I can see is the potential call of ~nsLoadGroup on the system
> group.  Someone more familiar with the event grouping should tell you if its
> worth the effort and release it (maybe only it) on a document or tab group
> somehow.
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +256,5 @@
> >    arrayToRelease.AppendElement(mTopWindowURI.forget());
> >  
> > +  nsCOMPtr<nsIRunnable> runnable =
> > +    new ProxyReleaseRunnable(Move(arrayToRelease));
> > +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",
> 
> full name?
> 

Sure.

> @@ +258,5 @@
> > +  nsCOMPtr<nsIRunnable> runnable =
> > +    new ProxyReleaseRunnable(Move(arrayToRelease));
> > +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",
> > +                        TaskCategory::Other,
> > +                        runnable.forget());
> 
> nsLoadGroup dtor may call via nsLoadGroup::Cancel on callbacks implemented
> in JS that may have undetermined effect.  not sure you can use the system
> group?
> 

Thanks for pointing this out. I think the best is to label this runnable if we have a doc/tab group. If not, we don't have to label it at all.

> mCallbacks dtor is something out of the control too, but I would not bother
> about that one particular.
> 
> @@ +905,5 @@
> >  
> >  void
> > +HttpBaseChannel::OnCopyComplete(nsresult aStatus)
> > +{
> > +  MOZ_ASSERT(XRE_IsParentProcess());
> 
> why? (a comment)
> 

I'll add it.

> ::: netwerk/protocol/http/HttpBaseChannel.h
> @@ +352,5 @@
> >      MOZ_MUST_USE nsresult
> >      DoApplyContentConversions(nsIStreamListener *aNextListener,
> >                                nsIStreamListener **aNewNextListener);
> >  
> > +    virtual void OnCopyComplete(nsresult aStatus);
> 
> you don't like writing comments, do you? :))
> 

Sometimes I think the code itself should be clear enough, but apparently I was wrong...

> please add some, what this is, what it does, who and when calls it.
> 

Will do.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +212,5 @@
> >    arrayToRelease.AppendElement(mCacheKey.forget());
> >  
> > +  nsCOMPtr<nsIRunnable> runnable =
> > +    new ProxyReleaseRunnable(Move(arrayToRelease));
> > +  SystemGroup::Dispatch("ReleaseMainThreadOnlyReferences",
> 
> "HttpChannelChild::ReleaseMainThreadOnlyReferences" ?
> 

Will do.

> @@ +1377,5 @@
> >    // CompleteRedirectSetup, and we need to call FinishInterceptedRedirect.
> > +  nsCOMPtr<nsIEventTarget> neckoTarget = GetNeckoTarget();
> > +  MOZ_ASSERT(neckoTarget);
> > +
> > +  DebugOnly<nsresult> rv =
> 
> Unused << ?  because I don't see this used even in debug builds

Will do.

Comment 26

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in HttpBaseChannel and HttpChannelChild, r=honzab

Summary:
 - Fix previous comments
 - Rebase
 - Carry reviewer's name

Comment 27

[Kershaw Chang [:kershaw]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72c391e3729
Label runnables in /http, r=honzab

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/c72c391e3729