Closed Bug 1201170 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 2 obsolete files)

During DivertToParent, the messages are diverted from the child back to the parent and it is not possible to suspend this messages.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Attached patch bug_1201170_v1.patch (obsolete) — Splinter Review
Attachment #8665090 - Flags: review?(jduell.mcbugs)
patch in comment 1 needs your attention
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-backlog]
Attached patch bug_1201170_v1.patch (obsolete) — Splinter Review
rebased
Attachment #8665090 - Attachment is obsolete: true
Attachment #8665090 - Flags: review?(jduell.mcbugs)
Attachment #8726668 - Flags: review?(jduell.mcbugs)
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.
Attachment #8726668 - Flags: review?(jduell.mcbugs) → review+
Flags: needinfo?(jduell.mcbugs)
Attachment #8726668 - Attachment is obsolete: true
Attachment #8730655 - Flags: review+
Keywords: checkin-needed
should this get uplifted to 47?
https://hg.mozilla.org/mozilla-central/rev/e109baaf9e01
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(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...
Depends on: 1259305
Dragana, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-backlog]
(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.
Flags: needinfo?(dd.mozilla)
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.
You need to log in before you can comment on or make changes to this bug.