1271987 - Revert AsyncOpen failures to be async (OnStart/onStop) instead of synchronous

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Jason Duell]

In bug 1170197 we changed a lot of AsyncOpen codepaths to return synchronous failure (instead of asynchronously failing via OnStart/OnStop).

There are some issues with this:

1) Many of these codepaths will fail async in the e10s case (example: we don't call on-modify-request observers in the parent until asyncOpen has returned in the child), but now fail sync in single-process.  In general, it's preferable for the code to work the same way in single process and e10s, so we might want to revert back to the AsyncAbort approach?  That would be my guess.

2) The new code looks like it will never remove a channel cancelled in on-modify-request (or any other error that causes a sync error) from the loadGroup.

3) There are still some AsyncAbort calls in BeginConnect--those seem to be in error at this point (well, if we keep failing synchronously)

4) We still have a comment here about only failing async after we're added to the LoadGroup:

   https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?from=nsHttpChannel.cpp#5206

I'm not actually sure if that comment was just stuck there randomly, or if there's actually something about having been added to a loadGroup that requires us to fail async after that.

Oh, actually, now I see bug 813869 comment 3--I talked to bz about this a long time ago.  The reason why we need to fail async after we called on-XXX-request and/or adding to the loadGroup is that those observers may expect to be able to add a reference to the channel and expect to get OnStopRequest so they know when to release it.   So I think we do want to change this code back to async failures (and we should move the existing comment about needing to fail async "from this point on" from the loadGroup add to the earlier on-opening-request call.  Maybe we could make the comment better too, something like

  // After we notify any observers (on-opening-request, loadGroup, etc) we must return NS_OK and return any errors asynchronously via OnStart/OnStopRequest.  Observers may add a reference to the channel and expect to get OnStopRequest so they know when to drop the reference, etc.

At this point my patch from bug 813869 is probably best just done as part of this work--make sure to double-check the comment/patch there and incorporate it.

While I'm making comment suggestions, let's modify 

    // the only time we would already know the proxy information at this
    // point would be if we were proxying a non-http protocol like ftp

to 

    // The common case for HTTP channels is to begin proxy resolution and return 
    // at this point. The only time we know mProxyInfo already is if we're
    // proxying a non-http protocol like ftp.

Comment 2

[u408661]

Dragana - I'm marking this active. Knock it back to -next if it's just in your queue.

Comment 3

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/233a3b2d3b36257042f3a7f5fdff87464e5fe346
Bug 1271987 - Correct heap size in bullet.js test case. r=luke

Comment 4

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d49d4613b029

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Lars T Hansen [:lth] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 233a3b2d3b36257042f3a7f5fdff87464e5fe346
> Bug 1271987 - Correct heap size in bullet.js test case. r=luke

This is a wrong bug :)
The bug message has a wrong bug number in it. Can you find the correct one?

Comment 6

[Lars T Hansen [:lth]]

Yes, sorry about that.  The correct bug was 1271985.  The sheriff corrected it on m-i, so things should be OK and you should just ignore the noise :)

Comment 7

[Dragana Damjanovic [:dragana]]

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

Comment 8

[Dragana Damjanovic [:dragana]]

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

Comment 9

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5373d36ea39d

Comment 10

[Dragana Damjanovic [:dragana]]

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

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1271987.patch

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8755337 [details] [diff] [review]
bug_1271987.patch

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

lgtm, but want a second opinion

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8755337 [details] [diff] [review]
bug_1271987.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5280,5 @@
>      // Remember we have Authorization header set here.  We need to check on it
>      // just once and early, AsyncOpen is the best place.
>      mCustomAuthHeader = mRequestHead.HasHeader(nsHttp::Authorization);
>  
> +    // The common case for HTTP channels is to begin proxy resolution and return 

nit: trailing ws

@@ +5287,2 @@
>      if (!mProxyInfo && NS_SUCCEEDED(ResolveProxy()))
>          return NS_OK;

nit: maybe add { }

@@ +5288,5 @@
>          return NS_OK;
>  
> +    if (NS_FAILED(BeginConnect())) {
> +        CloseCacheEntry(true);
> +        AsyncAbort(rv);

ehm.. and what is the rv value here?  didn't you want to:

rv = BeginConnect();
if (NS_FAILED(rv)) {
  CloseCacheEntry(true);
  AsyncAbort(rv);
}

return NS_OK;

?

@@ -5480,1 @@
>          }

the missing return rv was a bug here, right? :)

@@ +5746,1 @@
>          AsyncAbort(rv);

why are you closing the cache entry?  comment needed.
and, why are you closing it before you call asyncAbort?  the (true) arg - doomOnFailure - makes only sense when mStatus on the channel is set to a failure, but that's something AsyncAbort does.  Hence, the entry won't be doomed (and I don't understand why it should be!) when mStatus != failure.

I don't understand what you are doing here, is it what you just copy from ContinueBeginConnect()?

Introduced in https://hg.mozilla.org/mozilla-central/rev/25d9ba8ceb7a, never went through my review, so I don't know what's going on.

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8755337 [details] [diff] [review]
> bug_1271987.patch
> 
> Review of attachment 8755337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5280,5 @@
> >      // Remember we have Authorization header set here.  We need to check on it
> >      // just once and early, AsyncOpen is the best place.
> >      mCustomAuthHeader = mRequestHead.HasHeader(nsHttp::Authorization);
> >  
> > +    // The common case for HTTP channels is to begin proxy resolution and return 
> 
> nit: trailing ws
> 
> @@ +5287,2 @@
> >      if (!mProxyInfo && NS_SUCCEEDED(ResolveProxy()))
> >          return NS_OK;
> 
> nit: maybe add { }
> 
> @@ +5288,5 @@
> >          return NS_OK;
> >  
> > +    if (NS_FAILED(BeginConnect())) {
> > +        CloseCacheEntry(true);
> > +        AsyncAbort(rv);
> 
> ehm.. and what is the rv value here?  didn't you want to:
> 
> rv = BeginConnect();
> if (NS_FAILED(rv)) {
>   CloseCacheEntry(true);
>   AsyncAbort(rv);
> }
> 
> return NS_OK;
> 
> ?

Yes, thanks :)

> 
> @@ -5480,1 @@
> >          }
> 
> the missing return rv was a bug here, right? :)

There is return only couple of lines down so it has not really a bug.

> 
> @@ +5746,1 @@
> >          AsyncAbort(rv);
> 
> why are you closing the cache entry?  comment needed.
> and, why are you closing it before you call asyncAbort?  the (true) arg -
> doomOnFailure - makes only sense when mStatus on the channel is set to a
> failure, but that's something AsyncAbort does.  Hence, the entry won't be
> doomed (and I don't understand why it should be!) when mStatus != failure.
> 
> I don't understand what you are doing here, is it what you just copy from
> ContinueBeginConnect()?
> 
> Introduced in https://hg.mozilla.org/mozilla-central/rev/25d9ba8ceb7a, never
> went through my review, so I don't know what's going on.


I copied it from ContinueBeginConnect(). I wanted to make it consistent :)
Change https://hg.mozilla.org/mozilla-central/rev/25d9ba8ceb7a just copied it from somewhere else.

What should it be?

Should i remove it from both places?

Comment 15

[Honza Bambas (:mayhemer)]

So, I rather made an audit of what the arg should be and filed bug 1276673.  Hope my understanding is correct.

Based on that, I think you want to call it with false.

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1271987_v2.patch

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8758217 [details] [diff] [review]
bug_1271987_v2.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5763,5 @@
>          rv = BeginConnect();
>      }
>  
>      if (NS_FAILED(rv)) {
> +        CloseCacheEntry(true);

false here as well please

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1271987_v2.patch

Comment 19

[Dragana Damjanovic [:dragana]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f8d1092552
After on-***-request observers and loadGroup::AddRequest are called, on a failure AsyncOpen() must return async.r=mcmanus, r=mayhemer

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38f8d1092552