1614569 - Look into error codes that are returned form OnStart/OnStop/OnData or set by Cancel(), they should not be hard network errors

Status: NEW

(Core :: Networking: HTTP, enhancement, P3)

Description

[Dragana Damjanovic [:dragana]]

Currently only this defines what are soft and what are hard error codes.
Hard error codes will cause a Http2Session to be closed immediately and all started request will be aborted. We should be careful when we set a hard network error code. For example bug 1613943 was caused by BeaconStreamListener::OnStartRequest returning an hard error code.

OnStart/OnStop/OnData should not return an hard network error code (probably they should not return a network error code at all, but that will need to be investigated for each individual occurance). The behavior should be similar for error codes set by a HttpChannel's Cancel() call.

We should document which error codes are ok in nsIStreamListener IDL and nsIRequest IDL with explanation when to use which one.

Also adding a assertion (if possible) to OnStart/OnStop/OnData will be good. I am not sure if it is possible for Cancel().

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

I took a peek at this today and I have some questions:

1. Do we already have evidence that there are actors other than BeaconStreamListener returning hard error codes?
2. The main challenge here seems to be the task of exhaustively identifying all occurrences of this misbehavior. Is that right?
3. Where exactly is the code that causes the Http2Session to be closed immediately? From my digging, it appeared that it's not all in one place, but I wanted to be sure that I didn't miss something.
4. What should we assert in OnStart/Stop/Data? My guess would be that in the cases when these methods are returning an error code, it would be useful to have an assertion if the error is due to some anomaly that we might need/want to debug in the future. Let me know if I'm on the right track.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Forgot to needinfo?

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #1)

I took a peek at this today and I have some questions:

1. Do we already have evidence that there are actors other than BeaconStreamListener returning hard error codes?

No. This one has been notice only because the server had a problem for a longer time (a couple of days). I expect we do not have similar situation elsewhere. The main think I want to do in this bug is to add a diagnostic assertions in OnStart/OnData/OnStop.

2. The main challenge here seems to be the task of exhaustively identifying all occurrences of this misbehavior. Is that right?

yes. there is a lot of OnStart/OnData/OnStop. We could also add diagnostic assertions and wait for bugs to come. Have a look how much code is there to audit.

3. Where exactly is the code that causes the Http2Session to be closed immediately? From my digging, it appeared that it's not all in one place, but I wanted to be sure that I didn't miss something.

https://searchfox.org/mozilla-central/rev/2c06b16a0c15ae340a0532e319cbf89ef9d21b68/netwerk/protocol/http/ASpdySession.h#66

no-soft errors is what we want to avoid.

I notice that some listeners use NS_ERROR_FAILURE, we could change that to NS_ERROR_ABORT. It looks like that error is used when something goes really wrong, so it is not important.

4. What should we assert in OnStart/Stop/Data? My guess would be that in the cases when these methods are returning an error code, it would be useful to have an assertion if the error is due to some anomaly that we might need/want to debug in the future. Let me know if I'm on the right track.

yes.