Closed Bug 536317 Opened 12 years ago Closed 11 years ago

e10s HTTP: implement Cancel

Categories

(Core :: Networking: HTTP, defect)

Other Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: jduell.mcbugs, Assigned: dwitte)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 13 obsolete files)

23.93 KB, patch
Details | Diff | Splinter Review
13.92 KB, patch
Details | Diff | Splinter Review
4.34 KB, patch
Details | Diff | Splinter Review
We need to get Cancel working.

In the current necko setup, once you've canceled, you won't receive any more OnDataAvailable calls.   In e10s, incoming OnDataAvailable msgs are likely, as they may be sent before the Cancel from the child has time arrive (actually, they may have already been sent by the time the child even decides to cancel).  Any such messages should be dropped (see IPDL's "discard" keyword: it may provide this for free if we have a SendCancel event put us into a state where we discard OnDataAvailable msgs).  We do need to make sure that we deliver the final OnStopRequest msg.

Obviously we need to consider all the different places where a request may need to be canceled, both on the child or parent end.  I've tried to mark these as I've gone along so far.

In the case of the channel being canceled on the parent side, it may be enough to simply send whatever OnStart|StopRequest msgs the channel already will send HttpChannelParent, plus whatever notifications are needed.  

I'm guessing that we don't want to start implementing IPDL state transitions until we've thought through the cancel case fairly thoroughly--it may be good work to do at the same time.  For now I'll mark IPDL states as dependent on this.
Blocks: 536319
Assignee: nobody → michal.novotny
Note:

It's entirely possible--likely even--that a child-side cancel may cross paths (or come after) the parent has send an OnStopRequest with a non-cancel status.  In that case we need to convert the status that we actually deliver to the child channel's listener to the cancelled status.  

There is logic for this already in nsHttpChannel::OnStopRequest, as there's a similar race between the transport thread and the main thread:

line 5207:
    // honor the cancelation status even if the 
    // underlying transaction completed.
    if (mCanceled || NS_FAILED(mStatus))
        status = mStatus;

We should do the same.
Michal, will you be getting to this in the near future?  I have some spare cycles I could probably apply to this bug.
(In reply to comment #2)
> Michal, will you be getting to this in the near future?  I have some spare
> cycles I could probably apply to this bug.

It isn't on the top of my TODO list right now. Maybe I could start working on it at the end of this week. If you want to take it just go ahead...
(In reply to comment #0)
> see IPDL's "discard" keyword: it may provide this for free if we have a
> SendCancel event put us into a state where we discard OnDataAvailable msgs

Please, could you point me to some documentation? I've found only some proposal of discard keyword in an old thread in mozilla.devel mailing list... We still don't use states in PHttpChannel.ipdl, so it is unusable for now, right?
Grabbing from Michal
Assignee: michal.novotny → jduell.mcbugs
Assignee: jduell.mcbugs → webapps
Status: NEW → ASSIGNED
So we've got a least a few different use-cases here:

1) Cancel before we've created an IPDL channel: need to arrange on child-side to call OnStart/Stop with error code

2) Cancel on parent-side, before nsHttpChannel created.  Need to send msg to child and have it do same work as #1, and make sure channel cleaned up

3) Spots in parent where we need to cancel channel; should be fine just to cancel the nsHttpChannel, and it will still call HttpChannelParent with OnStart/Stop, which will propagate to child normally

4) Case where HttpChannelChild sees NS_FAILURE returned from client's OnStart/Data/Stop function.  Need to both send a Cancel to the parent (which may arrive after OnStop is complete there), block any OnDataAvailable msgs which arrive after child has canceled, and convert any OnStop that arrives w/o an error code into the appropriate error code.

I think those are all the use cases.  There's comments with the bug # in all the place we need this--just grep for it.
5) Case where Cancel is called by client code on child side.  Same as case #4.

Ambiguity: in #2, when I said "make sure channel cleaned up", I meant IPDL channel.
Attached patch Patch v1 (obsolete) — Splinter Review
There was one TODO I just removed in HttpParentChannel::OnDataAvailable.  If I understand correctly, there is no need to explicitly Cancel there.
Attachment #452110 - Flags: review?(jduell.mcbugs)
Also, for testing purposes I hacked together a quick fix for no response head.  Marking as a dependent of the real fix.
Depends on: 569044
183	bool
184	HttpChannelParent::RecvCancel(const nsresult& status)
185	{
186	  // May receive cancel before channel has been constructed!
187	  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
188	  if (httpChan) {
189	    nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
190	    httpChan->Cancel(status);
191	  }
192	  return true;
193	}

Something really strange is going on here.
> Something really strange is going on here.

Good catch.  We can just get rid of line 189.
Comment on attachment 452110 [details] [diff] [review]
Patch v1


>   if (NS_FAILED(rv)) {
>-    // TODO: Cancel request: (bug 536317)
>-    //  - Send Cancel msg to parent 
>-    //  - drop any in flight OnDataAvail msgs we receive
>-    //  - make sure we do call OnStopRequest eventually
>-    //  - return true here, not false
>-    return false;  
>+    Cancel(rv);
>   }

>+  if (mCanceled) {
>+    return true;
>+  }

On single line if branches, no need to have curly braces.  You made changes in a few places just to change style.  lets not do that. ;)


>   if (NS_FAILED(rv)) {
>-    // TODO: Cancel request: see OnStartRequest. Bug 536317
>-    return false; 
>+    Cancel(rv);
>+    return true;
>   }
>   return true;

no need for the first return true since after that block, you will return true regardless.


> }
> 
> bool 
> HttpChannelChild::RecvOnStopRequest(const nsresult& statusCode)
> {
>   LOG(("HttpChannelChild::RecvOnStopRequest [this=%x status=%u]\n", 
>            this, statusCode));
> 
>-  mState = HCC_ONSTOP;
>+  if (!mCanceled) {
>+    mState = HCC_ONSTOP;
>+    mStatus = statusCode;
>+  }
> 
>   mIsPending = PR_FALSE;
>-  mStatus = statusCode;

Don't you want to set mStatus to statusCode regardless of the state of mCanceled?

>-  if (mLoadGroup)
>+  if (mLoadGroup) {
>     mLoadGroup->RemoveRequest(this, nsnull, statusCode);
>+  }


^^^ style change for no reason

>+    mStatus = status;
>+
>+    mIsPending = false;
>+    if (mLoadGroup) {
>+      mLoadGroup->RemoveRequest(this, nsnull, mStatus);
>+    }
>+
>+    mListener->OnStartRequest(this, mListenerContext);
>+    mListener->OnStopRequest(this, mListenerContext, mStatus);
>+    mListener = NULL;
>+    mListenerContext = NULL;

nsnull to match the style above.  or vis versa.

Are there tests?
Attachment #452110 - Flags: review?(jduell.mcbugs) → review-
Attached patch xpcshell test for HTTP cancel (obsolete) — Splinter Review
>-  mState = HCC_ONSTOP;
>+  if (!mCanceled) {
>+    mState = HCC_ONSTOP;
>+    mStatus = statusCode;
>+  }
> 
>   mIsPending = PR_FALSE;
>-  mStatus = statusCode;

> Don't you want to set mStatus to statusCode regardless of the state of
> mCanceled?

No.  If the channel has been canceled on the client-side, but the cancel msg to the parent did not arrive before the parent sent SendOnStopRequest, it's possible that the child has a cancel status that it wants to keep, while the OnStop method is passing a non-cancelled status.  

We do want to to set the state to HCC_ONSTOP unconditionally, though.

> test for this?

I've attached an xpcshell test.  Note that "make check-one" is broken right now and always reports success even if the test fails (bug 581750).  So you need to eyeball or grep for TEST-UNEXPECTED-FAIL.

Note that test_httpcancel.js only tests case #5 (see comment 7), i.e when the channel is canceled on the client side.  You should be able to easily add logic to test case #1 (comment 6) and #4--just add them to test_httpcancel.js.  Case
2 and 3 (canceling on parent-side) are going to be hard to do in xpcshell, and I'm less worried about them. 

We also have a case #6:  cancel from a redirect handler.  test_bug455311.js will test this, but it depends on e10s redirects, so we can't use it yet.
stechz,

I haven't tested to see if test_httpcancel.js works with your patch (I have verified that it breaks w/o it).  So update the patch, make sure it works with the test, then add me as reviewer. Thanks.
Nit: flags in HttpBaseChannel are using PRUint8 and not 32, so far.  Maybe completely change it to 32?
Duplicate of this bug: 582477
Blocks: 582477
Attached patch Patch V2 (obsolete) — Splinter Review
I updated patch according to comments, and modified it so that it goes into mozilla central after changes. Test hangs for me, and I'm inspecting what happens.
Attachment #452110 - Attachment is obsolete: true
Please create any further patches using -U 8 -p.

Drive-by!  My biggest question is in regards to cancelling from the parent side.  There are various places that call SendOnCancel to the child, but the channel itself is not actually cancelled.  I guess we just let the parent channel carry on pretending everything's fine while the child silently discards everything, but then we'll hit OnStopRequest, and the child looks like it'll null-deref the listener which was previous cleared in CancelEarly.  Am I correct here? 

+  if (!mCanceled) {
+    // If this cancel occurs before nsHttpChannel has been set up, AsyncOpen
+    // is responsible for cleaning up.
+    mCanceled = true;
+    SendCancel(status);
+  }

Is there a test for this?  If the cancel occurs before AsyncOpen has been called, what happens when we try to send over IPDL?

>+void
>+HttpChannelChild::CancelEarly(nsresult status)
>+{
>+  // The cancel has occurred before nsHttpChannel was created on the parent
>+  // side.
>+
>+  if (!mCanceled) {

Let's invert the condition and just return early and save ourselves some indentation.


> bool
>+HttpChannelParent::RecvCancel(const nsresult& status)
>+{
>+  // May receive cancel before channel has been constructed!
>+  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>+  if (httpChan) {
>+    nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>+    httpChan->Cancel(status);
>+  }
>+  return true;
>+}

This block is still incorrect.

+  // Helpers
+  void CancelEarly(nsresult status);
+
+

Undesirable extra newline.
OS: Linux → All
tracking-fennec: --- → ?
Attached patch Patch V3 (obsolete) — Splinter Review
Fixed findings from Josh and made changes which makes test pass.

Jason: what should be done in ASyncOpen if Cancel is called before it?  

Your analysis seems correct of client/parent working behaviour seems correct to me. Can Jason confirm this?
Attachment #461584 - Attachment is obsolete: true
Mid-air.. so might be a little outdated...

(In reply to comment #18)
> Please create any further patches using -U 8 -p.

Yes, please do and also update your repo to the latest.

> ... Am I correct here? 

This patch has to better cooperate with bug 536317.

If nsHttpChannel::AsyncOpen failed, listener (HttpChannelParent) is not
referenced by that nsHttpChannel.  We don't need any additional clean-up on the
parent side then.  If I'm correct, the child just have to call
PHttpChannelChild::Send__delete__(this) as it does in OnStopRequest to break
the IPC bondage and release the parent that way (missing in the patch at the
moment).

It should be:
HttpChannelChild::CancelEarly()
{
  ...
  if (mIPCOpen)
    PHttpChannelChild::Send__delete__(this)
}

> 
> +  if (!mCanceled) {
> +    // If this cancel occurs before nsHttpChannel has been set up, AsyncOpen
> +    // is responsible for cleaning up.
> +    mCanceled = true;
> +    SendCancel(status);
> +  }
> 
> Is there a test for this?  If the cancel occurs before AsyncOpen has been
> called, what happens when we try to send over IPDL?

There is no IPC bondage at this point, just let the channel child get released
and we are done.  But it's worth to have a test we don't leak.
Comment on attachment 462158 [details] [diff] [review]
Patch V3

>+void
>+HttpChannelChild::CancelEarly(nsresult status)
>+  if (mCanceled && mIPCOpen)
>+    return;

This condition is strange, what if is mCanceled true and mIPCOpen false?

> bool
>+HttpChannelParent::RecvCancel(const nsresult& status)
>+  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>+  if (httpChan) {

You should check mChannel.get() is non-null here, static_cast might make null pointer non-null..

> HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
>+  
>+  if (mIPCClosed) 
>+    return NS_ERROR_UNEXPECTED;

Why exactly this change?
tracking-fennec: ? → 2.0a1+
Attached patch Patch V3.1 (obsolete) — Splinter Review
Comment on attachment 462996 [details] [diff] [review]
Patch V3.1

Adapted patch to latest codebase but didn't change any functionality.
Attachment #462996 - Attachment description: Patch V4 → Patch V3.1
Attachment #462158 - Attachment is obsolete: true
This will need to use the same buffering mechanism for RecvOnCancel and RecvCancel that the other receiver functions use.
Depends on: 584863
> what should be done in ASyncOpen if Cancel is called before it?  

See bug 584863.  Just have HttpChannelChild::AsyncOpen return mStatus immediately if mCanceled.

I'm planning to review this after I look over redirects.  Let me know if you're close to a new patch.
I have new patch almost ready, so please do not review yet. I modified test_httpcancel.js to also test case when cancel is called directly after asyncOpen.
Attachment #462996 - Attachment is obsolete: true
Attachment #463315 - Flags: review?
Miika/Florian:  don't see the change to test_httpcancel: are you working together, or submitting different patches?
We are currently syncinc work. Buffering of events caused test case when cancel is called directly after asyncOpen to fail. 

This happens because buffered events are handled from HttpChannelChild::RecvOnStartRequest after listeners has been notified with OnStartRequest call. This causes listener to see 0 as a context instead of NS_BINDING_ABORTED. How we should handle this?
Attached patch Patch v4.1, compined changes (obsolete) — Splinter Review
I put mine and Florians changes together. 

This patch passes test case, where cancel is called from on-modify-request. Case when cancel is called directly after asyncOpen is still non-working because of request buffering. I'll also attach modifications to test patch. Both cases succeeds when running without parent/child process separation.

On last comment I meant status, not context.
Attachment #463315 - Attachment is obsolete: true
Attachment #463315 - Flags: review?
Attachment #460596 - Attachment is obsolete: true
>This patch passes test case, where cancel is called from on-modify-request.
>Case when cancel is called directly after asyncOpen is still non-working
>because of request buffering. I'll also attach modifications to test patch.
>Both cases succeeds when running without parent/child process separation.

This buffering is actually incorrect - no messages should be buffered before OnStartRequest is received.  This will be corrected by bug 584604.
Is there something what still needs to be done in this bug, while waiting for that one to be fixed?
Attachment #463419 - Flags: review?(jduell.mcbugs)
Comment on attachment 463419 [details] [diff] [review]
Patch v4.1, compined changes

Me me!
Attachment #463419 - Flags: review?(jduell.mcbugs) → review?(dwitte)
Assignee: webapps → nobody
Marking dependent on bug 584604, since the buffering changes here will affect this patch, and that patch should land first.
Depends on: 584604
Comment on attachment 463419 [details] [diff] [review]
Patch v4.1, compined changes

>diff -r 5a12acfab150 netwerk/protocol/http/HttpChannelChild.cpp

>@@ -252,24 +258,29 @@ HttpChannelChild::RecvOnStopRequest(cons
> {
>   StopRequestEvent* event = new StopRequestEvent(this, statusCode);
>   return BufferOrDispatch(event);
> }
> 
> bool 
> HttpChannelChild::OnStopRequest(const nsresult& statusCode)
> {
>-  LOG(("HttpChannelChild::RecvOnStopRequest [this=%x status=%u]\n", 
>+  LOG(("HttpChannelChild::OnStopRequest [this=%x status=%u]\n", 
>            this, statusCode));
> 
>-  mState = HCC_ONSTOP;
>+  mIsPending = PR_FALSE;
> 
>-  mIsPending = PR_FALSE;
>-  mStatus = statusCode;
>-  mListener->OnStopRequest(this, mListenerContext, statusCode);
>+  if (!mCanceled) {
>+    mState = HCC_ONSTOP;
>+    mStatus = statusCode;
>+  }
>+
>+  if (mListener)
>+    mListener->OnStopRequest(this, mListenerContext, statusCode);

Under what conditions could mListener be null here?

>+bool
>+HttpChannelChild::RecvOnCancel(const nsresult& status)
>+{
>+  CancelEvent* event = new CancelEvent(this, status);
>+  return BufferOrDispatch(event);
>+}

This will need revising per the changes in bug 584604. Either you should update it, or perhaps jdm can do it since he's rather familiar with that code now. ;)

>+
>+bool
>+HttpChannelChild::OnCancel(const nsresult& status)

It doesn't look like this needs to return anything, so you can make it void.

>+{
>+  //We need two flags here, because mCanceled would be
>+  //true when we are cancelled by listener in asyncOpen
>+  if (mCanceled)
>+    return true;

This comment isn't accurate -- you set mCanceled to false in AsyncOpen before getting here, so just note that fact here.

>diff -r 5a12acfab150 netwerk/protocol/http/HttpChannelParent.cpp

>@@ -116,21 +116,24 @@ HttpChannelParent::RecvAsyncOpen(const I
>   uri->GetSpec(uriSpec);
>   LOG(("HttpChannelParent RecvAsyncOpen [this=%x uri=%s]\n", 
>        this, uriSpec.get()));
> 
>   nsresult rv;
> 
>   nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv));
>   if (NS_FAILED(rv))
>-    return false;       // TODO: cancel request (bug 536317), return true
>+    return SendOnCancel(rv);
> 
>   rv = NS_NewChannel(getter_AddRefs(mChannel), uri, ios, nsnull, nsnull, loadFlags);
>   if (NS_FAILED(rv))
>-    return false;       // TODO: cancel request (bug 536317), return true
>+    return SendOnCancel(rv);
>+
>+  if (!mChannel.get())
>+    return SendOnCancel(NS_ERROR_NULL_POINTER);

I don't think it's possible for NS_NewChannel() to succeed and return a null pointer... you can remove this check.

> bool
>+HttpChannelParent::RecvCancel(const nsresult& status)
>+{
>+  // May receive cancel before channel has been constructed!
>+  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>+  if (mChannel.get() && httpChan) {

if (mChannel) is sufficient here.

>@@ -198,16 +212,19 @@ HttpChannelParent::RecvSetCacheTokenCach
> NS_IMETHODIMP
> HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
> {
>   LOG(("HttpChannelParent::OnStartRequest [this=%x]\n", this));
> 
>   nsHttpChannel *chan = static_cast<nsHttpChannel *>(aRequest);
>   nsHttpResponseHead *responseHead = chan->GetResponseHead();
> 
>+  if (mIPCClosed)
>+    return NS_ERROR_UNEXPECTED;

Under what conditions would this occur? Do we need the same check in HttpChannelParent::OnStopRequest()?

>@@ -254,17 +271,17 @@ HttpChannelParent::OnDataAvailable(nsIRe
> 
>   nsCString data;
>   data.SetLength(aCount);
>   char * p = data.BeginWriting();
>   PRUint32 bytesRead;
>   rv = aInputStream->Read(p, aCount, &bytesRead);
>   data.EndWriting();
>   if (!NS_SUCCEEDED(rv) || bytesRead != aCount) {
>-    return rv;              // TODO: cancel request locally (bug 536317)
>+    return rv;

Don't we have to cancel locally and eventually send a cancel across to the child?

r- since I'd like to see answers to the above, but otherwise looks good.
Attachment #463419 - Flags: review?(dwitte) → review-
I've been updating this patch to work with the new buffering changes, and I discovered that the tests completely failed with my changes.  Specifically, the mStatus member is never set in HttpChannelChildCancel, so the status code is never propagated forwards.  I'll attach my updated patch, but this needs to be fixed.
Attached patch Updated to new buffering system (obsolete) — Splinter Review
Attachment #463419 - Attachment is obsolete: true
Attached patch Updated to new buffering system (obsolete) — Splinter Review
Whoops, missed an indentation.  Also, this is applied over my suspend/resume changes - sorry.
Attachment #464263 - Attachment is obsolete: true
removed obsolete comment
Attachment #464264 - Attachment is obsolete: true
Attachment #464486 - Flags: review?
Attachment #464486 - Flags: review? → review?(jduell.mcbugs)
Assignee: nobody → florian.haenel
Blocks: 586146
Comment on attachment 464486 [details] [diff] [review]
Updated to new buffering system v1.1

I'm unrotting this, new patch upcoming...
Attachment #464486 - Flags: review?(jduell.mcbugs)
Attached patch updated patch (obsolete) — Splinter Review
Updated patch for my review comments, rot, and redirect cancels.

I *think* I've got the redirect cancel stuff right, but I'd like jduell to look at that bit carefully. I also put an XXXdwitte comment in HttpChannelParent::OnDataAvailable(). The previous patch here removed a comment saying "cancel request locally" without actually doing any cancelling, which seemed wrong to me, but maybe it was intentional.

This also includes the cancel test patch, which passes for me. But I'm sure it doesn't test all the cases. ;) We at least need one for redirect cancel stuff...
Attachment #463420 - Attachment is obsolete: true
Attachment #464486 - Attachment is obsolete: true
Attachment #464914 - Flags: review?
Comment on attachment 464914 [details] [diff] [review]
updated patch

jduell: see previous comment.
Attachment #464914 - Flags: review? → review?(jduell.mcbugs)
Attached patch updated patchSplinter Review
... and here's the actual patch. :/
Attachment #464914 - Attachment is obsolete: true
Attachment #464941 - Flags: review?(jduell.mcbugs)
Attachment #464914 - Flags: review?(jduell.mcbugs)
Attached patch updates (obsolete) — Splinter Review
These are changes to the last patch, as discussed with jduell on IRC.
Assignee: florian.haenel → dwitte
Attachment #465146 - Flags: review?(jduell.mcbugs)
Attachment #465146 - Attachment is obsolete: true
Attachment #465146 - Flags: review?(jduell.mcbugs)
Attached patch updatesSplinter Review
Attachment #465155 - Flags: review?(jduell.mcbugs)
Attached patch patch to updatesSplinter Review
includes fix for bug 569026
Attachment #464941 - Flags: review?(jduell.mcbugs)
Attachment #465155 - Flags: review?(jduell.mcbugs)
Landed on e10s:

http://hg.mozilla.org/projects/electrolysis/rev/6bc7eea143ea

Needs merging to m-c.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 586205
Blocks: 586766
Blocks: 586802
You need to log in before you can comment on or make changes to this bug.