1201170 - [e10s] Make possible suspending a channel during Diverting messages from a child to the parent

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

During DivertToParent, the messages are diverted from the child back to the parent and it is not possible to suspend this messages.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1201170_v1.patch

Comment 2

[Patrick McManus [:mcmanus]]

patch in comment 1 needs your attention

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1201170_v1.patch

rebased

Comment 4

[Dragana Damjanovic [:dragana]]

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

Comment 5

[Jason Duell]

Comment on attachment 8726668 [details] [diff] [review]
bug_1201170_v1.patch

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

This looks good.  I only have one issue with naming.

::: netwerk/base/ADivertableParentChannel.h
@@ +32,5 @@
>    virtual nsresult SuspendForDiversion() = 0;
> +
> +  // While messages are diverted back from the child to the parent calls to
> +  // suspend/resume the channel must also suspend/resume the message diversion.
> +  // This two functions will be called by nsHttpChannel and nsFtpChannel

These two...

::: netwerk/base/nsIChannelWithDivertableParentListener.idl
@@ +43,5 @@
> +   * the message diversion. If suspend/resume is received from ChildChannel
> +   * the message diversion should not be suspended/resumed.
> +   */
> +  void SuspendThroughParent();
> +  void ResumeThroughParent();

So I find the name "{Suspend|Resume}ThroughParent()" confusing here.  These functions just contain the body of the original Suspend/Resume functions, and don't do anything "through the parent".  They're the guts of Suspend/Resume, without the part of the new Suspend/Resume that notifies the parent.

I think it's more understandable to rename them something like "SuspendInternal()" (or "SuspendPumps()" or "DoSuspend()": I'll let you pick), and have the description be something like:

 /** 
  * Internal versions of Suspend/Resume that suspend (or resume) the channel
  * but do not suspend the ParentChannel's IPDL message queue.
  */
  void SuspendInternal();

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1364,5 @@
> +nsresult
> +HttpChannelParent::SuspendMessageDiversion()
> +{
> +  LOG(("HttpChannelParent::SuspendMessageDiversion [this=%p]", this));
> +  // This only need to suspend message queue.

needs

@@ +1373,5 @@
> +nsresult
> +HttpChannelParent::ResumeMessageDiversion()
> +{
> +  LOG(("HttpChannelParent::SuspendMessageDiversion [this=%p]", this));
> +  // This only need to resumes message queue.

needs

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4959,5 @@
>  {
> +    nsresult rv = ResumeThroughParent();
> +
> +    nsresult rvParentChannel = NS_OK;
> +    if (mParentChannel) {

Ah, so you guarantee that we only call parent->ResumeMsgDiversion() during diversion by only setting mParentChannel to non-null during the diversion.  That works--I hope we remember this if/when we ever switch to keeping mParentChannel around for longer (if we need it for some other reason).  I guess we can deal with that if it happens.

Comment 6

[Dragana Damjanovic [:dragana]]

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

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1201170_v1.patch

Comment 8

[Jim Mathies [:jimm]]

should this get uplifted to 47?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e109baaf9e01

Comment 10

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

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

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Jim Mathies [:jimm] from comment #8)
> should this get uplifted to 47?

We can uplift it. Let's wait for couple of day...

Comment 12

[Chris Peterson [:cpeterson]]

Dragana, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> Dragana, should this fix be uplifted to Aurora 47 in preparation for our
> e10s experiments on Beta 47?

I would like to, but I would prefer pushing this together with 1259305. 1259305 needs review first.

Comment 14

[Chris Peterson [:cpeterson]]

I'm going to mark status-firefox47=wontfix for now because dependent bug 1259305 is not also ready for uplift. (Bug 1259305 is still waiting for r+.) 47 is moving from Aurora to Beta next week, so the window to uplift riskier changes is quickly closing.