536317 - e10s HTTP: implement Cancel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

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.

Comment 1

[Jason Duell]

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.

Comment 2

[Josh Matthews [:jdm]]

Michal, will you be getting to this in the near future?  I have some spare cycles I could probably apply to this bug.

Comment 3

[Michal Novotny [:michal]]

(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...

Comment 4

[Michal Novotny [:michal]]

(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?

Comment 5

[Jason Duell]

Grabbing from Michal

Comment 6

[Jason Duell]

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.

Comment 7

[Jason Duell]

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.

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Patch v1

There was one TODO I just removed in HttpParentChannel::OnDataAvailable.  If I understand correctly, there is no need to explicitly Cancel there.

Comment 9

[Benjamin Stover (:stechz)]

Also, for testing purposes I hacked together a quick fix for no response head.  Marking as a dependent of the real fix.

Comment 10

[Josh Matthews [:jdm]]

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.

Comment 11

[Benjamin Stover (:stechz)]

> Something really strange is going on here.

Good catch.  We can just get rid of line 189.

Comment 12

[Doug Turner (:dougt)]

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?

Comment 13

[Jason Duell]

Attachment: xpcshell test for HTTP cancel

>-  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.

Comment 14

[Jason Duell]

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.

Comment 15

[Honza Bambas (:mayhemer)]

Nit: flags in HttpBaseChannel are using PRUint8 and not 32, so far.  Maybe completely change it to 32?

Comment 17

[Miika Jarvinen]

Attachment: Patch V2

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.

Comment 18

[Josh Matthews [:jdm]]

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.

Comment 19

[Miika Jarvinen]

Attachment: Patch V3

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?

Comment 20

[Honza Bambas (:mayhemer)]

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 21

[Honza Bambas (:mayhemer)]

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?

Comment 22

[Florian Hänel [:heeen]]

Attachment: Patch V3.1

Comment 23

[Florian Hänel [:heeen]]

Comment on attachment 462996 [details] [diff] [review]
Patch V3.1

Adapted patch to latest codebase but didn't change any functionality.

Comment 24

[Josh Matthews [:jdm]]

This will need to use the same buffering mechanism for RecvOnCancel and RecvCancel that the other receiver functions use.

Comment 25

[Jason Duell]

> 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.

Comment 26

[Miika Jarvinen]

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.

Comment 27

[Florian Hänel [:heeen]]

Attachment: Patch V4 Now buffering of cancel events

Comment 28

[Jason Duell]

Miika/Florian:  don't see the change to test_httpcancel: are you working together, or submitting different patches?

Comment 29

[Miika Jarvinen]

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?

Comment 30

[Miika Jarvinen]

Attachment: Patch v4.1, compined changes

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.

Comment 31

[Miika Jarvinen]

Attachment: wrapper for ipc test and test case for direct call of cancel

Comment 32

[Josh Matthews [:jdm]]

>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.

Comment 33

[Miika Jarvinen]

Is there something what still needs to be done in this bug, while waiting for that one to be fixed?

Comment 34

[[e-mail]]

Comment on attachment 463419 [details] [diff] [review]
Patch v4.1, compined changes

Me me!

Comment 35

[[e-mail]]

Marking dependent on bug 584604, since the buffering changes here will affect this patch, and that patch should land first.

Comment 36

[[e-mail]]

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.

Comment 37

[Josh Matthews [:jdm]]

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.

Comment 38

[Josh Matthews [:jdm]]

Attachment: Updated to new buffering system

Comment 39

[Josh Matthews [:jdm]]

Attachment: Updated to new buffering system

Whoops, missed an indentation.  Also, this is applied over my suspend/resume changes - sorry.

Comment 40

[Florian Hänel [:heeen]]

Attachment: Updated to new buffering system v1.1

removed obsolete comment

Comment 41

[[e-mail]]

Comment on attachment 464486 [details] [diff] [review]
Updated to new buffering system v1.1

I'm unrotting this, new patch upcoming...

Comment 42

[[e-mail]]

Attachment: updated patch

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...

Comment 43

[[e-mail]]

Comment on attachment 464914 [details] [diff] [review]
updated patch

jduell: see previous comment.

Comment 44

[[e-mail]]

Attachment: updated patch

... and here's the actual patch. :/

Comment 45

[[e-mail]]

Attachment: updates

These are changes to the last patch, as discussed with jduell on IRC.

Comment 46

[[e-mail]]

Attachment: updates

Comment 47

[Jason Duell]

Attachment: patch to updates

includes fix for bug 569026

Comment 48

[[e-mail]]

Landed on e10s:

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

Needs merging to m-c.