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

RESOLVED FIXED in Firefox 49

Status

()

Core
Networking
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jduell, Assigned: dragana)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(e10s+, firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

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

Updated

2 years ago
Duplicate of this bug: 813869
Dragana - I'm marking this active. Knock it back to -next if it's just in your queue.
Whiteboard: [necko-active]
(Assignee)

Comment 5

2 years ago
(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?
Flags: needinfo?(lhansen)

Comment 6

2 years ago
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 :)
Flags: needinfo?(lhansen)

Updated

2 years ago
tracking-e10s: --- → ?

Updated

2 years ago
tracking-e10s: ? → +
Priority: -- → P3
(Assignee)

Comment 11

2 years ago
Created attachment 8755337 [details] [diff] [review]
bug_1271987.patch
Attachment #8755337 - Flags: review?(mcmanus)
Comment on attachment 8755337 [details] [diff] [review]
bug_1271987.patch

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

lgtm, but want a second opinion
Attachment #8755337 - Flags: review?(mcmanus) → review?(honzab.moz)
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.
Attachment #8755337 - Flags: review?(honzab.moz) → review-
(Assignee)

Comment 14

2 years ago
(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?
Flags: needinfo?(honzab.moz)
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.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 16

2 years ago
Created attachment 8758217 [details] [diff] [review]
bug_1271987_v2.patch
Attachment #8755337 - Attachment is obsolete: true
Attachment #8758217 - Flags: review?(honzab.moz)
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
Attachment #8758217 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8758318 [details] [diff] [review]
bug_1271987_v2.patch
Attachment #8758217 - Attachment is obsolete: true
Attachment #8758318 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by kcambridge@mozilla.com:
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
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38f8d1092552
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.