Replace some net::ERR_FAILED generic error codes with more specific codes....

Replace some net::ERR_FAILED generic error codes with more specific codes.
The goal is to end up with more meaningful errors if a page fails to load.

BUG=22623
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27038

[wtc, 2009-09-23 21:07:39]

LGTM.  Thanks!

Please improve the error code names and descriptions.
The other comments are just nits.

http://codereview.chromium.org/222009/diff/1/3
File net/base/net_error_list.h (right):

http://codereview.chromium.org/222009/diff/1/3#newcode276
Line 276: // The URLRequestJob failed to create a network transaction.
These error codes are beginning to expose too much
implementation detail (URLRequestJob).  Please try to
name the error codes so they are more general.

For example, FILTER_FAILED could be renamed
CONTENT_DECODING_FAILED, and it probably belongs in
the -3xx section for HTTP errors.  (The -4xx section
is apparently for cache errors.)

I don't have a good idea for JOB_CANNOT_CREATE_NETWORK_TRANSACTION.
Perhaps we should just have a single
CANNOT_CREATE_NETWORK_TRANSACTION and use
it for both cache and URLRequestJob.

http://codereview.chromium.org/222009/diff/1/2
File net/http/http_transaction_factory.h (right):

http://codereview.chromium.org/222009/diff/1/2#newcode18
Line 18: // Creates a HttpTransaction object. May return NULL on failure.
Change "May return" to "Returns".

http://codereview.chromium.org/222009/diff/1/5
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/222009/diff/1/5#newcode581
Line 581: rv = net::ERR_JOB_CANNOT_CREATE_NETWORK_TRANSACTION;
Another option is to change CreateTransaction to return
an error code on failure.

Is out-of-memory the only reason CreateTransaction may
fail?

[eroman, 2009-09-23 22:13:47]

Please review the new patchset (adds a bunch more files).

http://codereview.chromium.org/222009/diff/1/3
File net/base/net_error_list.h (right):

http://codereview.chromium.org/222009/diff/1/3#newcode276
Line 276: // The URLRequestJob failed to create a network transaction.
On 2009/09/23 21:07:39, wtc wrote:
> These error codes are beginning to expose too much
> implementation detail (URLRequestJob).  Please try to
> name the error codes so they are more general.
> 
> For example, FILTER_FAILED could be renamed
> CONTENT_DECODING_FAILED, and it probably belongs in
> the -3xx section for HTTP errors.  (The -4xx section
> is apparently for cache errors.)

Done.

> I don't have a good idea for JOB_CANNOT_CREATE_NETWORK_TRANSACTION.
> Perhaps we should just have a single
> CANNOT_CREATE_NETWORK_TRANSACTION and use
> it for both cache and URLRequestJob.

http://codereview.chromium.org/222009/diff/1/5
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/222009/diff/1/5#newcode581
Line 581: rv = net::ERR_JOB_CANNOT_CREATE_NETWORK_TRANSACTION;
On 2009/09/23 21:07:39, wtc wrote:
> Another option is to change CreateTransaction to return
> an error code on failure.
> 
> Is out-of-memory the only reason CreateTransaction may
> fail?

CreateTransaction() can fail because of "Suspend".

HttpCache was also checking for this error and returning
ERR_CACHE_CANNOT_CREATE_NETWORK_TRANSACTION (which is what I had modelled the
name after).

I have changed the API of CreateTransaction to instead return an error code.

And as the error code, I have tentatively chosen ERR_NETWORK_LAYER_SUSPENDED.

[wtc, 2009-09-23 23:13:00]

LGTM.  Too bad we need to change so many files.
Please see my comments below and decide how you
want to proceed.

http://codereview.chromium.org/222009/diff/10/4003
File net/base/net_error_list.h (left):

http://codereview.chromium.org/222009/diff/10/4003#oldcode269
Line 269: // The network transaction factory of the cache was not able to create
a new
We should add a comment to note that -402 is available
(or reserved).

http://codereview.chromium.org/222009/diff/10/4003
File net/base/net_error_list.h (right):

http://codereview.chromium.org/222009/diff/10/4003#newcode263
Line 263: // One of the filters attached to the job gave an error.
This comment is the de facto error message for this error
code, so it needs to be clear.  "job" refers to URLRequestJob,
but it's not clear to a user.

I suggest something like
  Content decoding of the response body failed.

http://codereview.chromium.org/222009/diff/10/4003#newcode266
Line 266: // An operation could not be completed, because the network layer is
in a
Same here: "network layer" refers to HttpNetworkLayer,
which is not obvious to the user.

How about this error message and error code name?

  // An operation could not be completed because all network IO
  // is suspended.
  NET_ERROR(NETWORK_IO_SUSPENDED, -331)

We should ask Ricardo if we still need the Suspend method.
He added it to work around the BSOD caused by WinHTTP,
but we later came up with the proper fix, and we're not
using WinHTTP any more.

Do you think the Suspend method is useful for other
purposes?

http://codereview.chromium.org/222009/diff/10/4002
File net/http/http_transaction_factory.h (right):

http://codereview.chromium.org/222009/diff/10/4002#newcode21
Line 21: // transaction to |*trans| and returns OK.
Add: On failure, returns a network error code.

http://codereview.chromium.org/222009/diff/10/4002#newcode22
Line 22: virtual int CreateTransaction(scoped_ptr<HttpTransaction>* trans) = 0;
It is unfortunate that this API change requires changing so
many files.

If ERR_NETWORK_LAYER_SUSPENDED (other than out-of-memory,
which we ignore throughout the code base) is the only
possible error code, why don't we just set that error
code manually when a CreateTransaction call fails?

[eroman, 2009-09-24 00:10:44]

http://codereview.chromium.org/222009/diff/10/4003
File net/base/net_error_list.h (left):

http://codereview.chromium.org/222009/diff/10/4003#oldcode269
Line 269: // The network transaction factory of the cache was not able to create
a new
On 2009/09/23 23:13:00, wtc wrote:
> We should add a comment to note that -402 is available
> (or reserved).

Done.

http://codereview.chromium.org/222009/diff/10/4003
File net/base/net_error_list.h (right):

http://codereview.chromium.org/222009/diff/10/4003#newcode263
Line 263: // One of the filters attached to the job gave an error.
On 2009/09/23 23:13:00, wtc wrote:
> This comment is the de facto error message for this error
> code, so it needs to be clear.  "job" refers to URLRequestJob,
> but it's not clear to a user.
> 
> I suggest something like
>   Content decoding of the response body failed.

Done.

http://codereview.chromium.org/222009/diff/10/4003#newcode266
Line 266: // An operation could not be completed, because the network layer is
in a
On 2009/09/23 23:13:00, wtc wrote:
> Same here: "network layer" refers to HttpNetworkLayer,
> which is not obvious to the user.
> 
> How about this error message and error code name?
> 
>   // An operation could not be completed because all network IO
>   // is suspended.
>   NET_ERROR(NETWORK_IO_SUSPENDED, -331)
> 
> We should ask Ricardo if we still need the Suspend method.
> He added it to work around the BSOD caused by WinHTTP,
> but we later came up with the proper fix, and we're not
> using WinHTTP any more.
> 
> Do you think the Suspend method is useful for other
> purposes?

Done.

I will ask Ricardo with regards to if it is still necessary.

[rvargas (doing something else), 2009-09-24 01:14:53]

> > 
> > Do you think the Suspend method is useful for other
> > purposes?
> 
> Done.
> 
> I will ask Ricardo with regards to if it is still necessary.

In theory we don't need it anymore but it may be helping to reduce the amount of
unneeded work on system resume (or maybe not). In any case, it's better to do
that in another CL (if we remove it).

[wtc, 2009-09-24 17:06:45]

Eric, Ricardo,

It's unfortunate that we had to change the prototype of
CreateTransaction to return an error code.  Now
CreateTransaction looks different from all the other
factory methods in our source tree.

I have a better idea now.  Let's restore the original
prototype of CreateTransaction but change the way we
suspend network IO.  Rather than suppressing the
creation of network transactions, we can suppress the
Start method instead.  In other words, we still create
a network transaction, but its Start method returns
ERR_NETWORK_IO_SUSPENDED if the suspended_ state is
true.

This will require some changes to allow a network
transaction to access the suspended_ state variable,
but I think it's more in keeping with the design of
our HttpTransaction interface.

What do you think?

[eroman, 2009-09-24 20:14:28]

sounds fine.

in addition to Start(), this should probably also apply for redirects
too (which create a job, which may create a transaction).

On Thu, Sep 24, 2009 at 10:06 AM,  <wtc@chromium.org> wrote:
> Eric, Ricardo,
>
> It's unfortunate that we had to change the prototype of
> CreateTransaction to return an error code. =A0Now
> CreateTransaction looks different from all the other
> factory methods in our source tree.
>
> I have a better idea now. =A0Let's restore the original
> prototype of CreateTransaction but change the way we
> suspend network IO. =A0Rather than suppressing the
> creation of network transactions, we can suppress the
> Start method instead. =A0In other words, we still create
> a network transaction, but its Start method returns
> ERR_NETWORK_IO_SUSPENDED if the suspended_ state is
> true.
>
> This will require some changes to allow a network
> transaction to access the suspended_ state variable,
> but I think it's more in keeping with the design of
> our HttpTransaction interface.
>
> What do you think?
>
> http://codereview.chromium.org/222009
>

[rvargas (doing something else), 2009-09-24 23:27:25]

We can do that. However, I would prefer something like the first version of this
CL, returning CANNOT_CREATE_NETWORK_TRANSACTION at the two places where we check
that the transaction was actually created. I know it's not the cleanest solution
but I think it's the simplest one.

It looks like there is no undesirable side effect to create the transaction when
the system is going to sleep, but doing nothing seems better.

On 2009/09/24 20:14:28, eroman wrote:
> sounds fine.
> 
> in addition to Start(), this should probably also apply for redirects
> too (which create a job, which may create a transaction).
> 
> On Thu, Sep 24, 2009 at 10:06 AM,  <mailto:wtc@chromium.org> wrote:
> > Eric, Ricardo,
> >
> > It's unfortunate that we had to change the prototype of
> > CreateTransaction to return an error code. =A0Now
> > CreateTransaction looks different from all the other
> > factory methods in our source tree.
> >
> > I have a better idea now. =A0Let's restore the original
> > prototype of CreateTransaction but change the way we
> > suspend network IO. =A0Rather than suppressing the
> > creation of network transactions, we can suppress the
> > Start method instead. =A0In other words, we still create
> > a network transaction, but its Start method returns
> > ERR_NETWORK_IO_SUSPENDED if the suspended_ state is
> > true.
> >
> > This will require some changes to allow a network
> > transaction to access the suspended_ state variable,
> > but I think it's more in keeping with the design of
> > our HttpTransaction interface.
> >
> > What do you think?
> >
> > http://codereview.chromium.org/222009
> >