Introduce ERR_NETWORK_CHANGED and allow URLFetcher to automatically retry on that error.

Introduce ERR_NETWORK_CHANGED and allow URLFetcher to automatically retry on that error.

BUG=164363
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173227

[Joao da Silva, 2012-12-07 18:51:22]

Hi Paul, can you have a first look at this before asking a wider audience for a
review?

ERR_NETWORK_CHANGED is triggered by host_resolver_impl.cc and
client_socket_pool_base.cc when their jobs are aborted due to network changes.
This means that fetchers in the resolving or connecting stage can automatically
retry. The new unit_tests verify the expectations of the retry behavior.

I've also verified that enabling SetAutomaticallyRetryOnNetworkChanges() fixes
the other bugs that are causing by the early notifications, but left that out of
this CL.

Let me know what you think. Thanks!

[pauljensen, 2012-12-07 23:00:05]

I took a first look.  It seems on track with what we discussed prior.  I haven't
dealt much with the URLFetcher so I can't give too much good feedback on the API
change.  szym@ should definitely have a look at the HostResolver stuff.  You
didn't add the initial connectivity check I mentioned (i.e. if offline, wait
until online before issuing request); if this fixes the issues you were seeing,
I wonder if the fetchers are always attempted and then aborted and then retried?
 I don't really see any changes to users of ERR_ABORTED; have you reviewed them
much?

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:617: if (NetworkChangeNotifier::IsOffline())
{
I think it should be:
  if (type == NetworkChangeNotifier::CONNECTION_NONE)

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:883: max_retries_on_network_changes_--;
I notice the code above uses a counter and a maximum value, rather than your
method of decreasing the maximum.  I don't know if one way is better than the
other.  If there were some way to restart a fetch, then having a separate
maximum might be nice because it could live across reuses.  Your call.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:912:
NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
Would be nice if we didn't always have to do this (i.e. just when we did
register ourselves).  The ObserverList is rather slow (its thread safe so it
locks and it's really a list so it's linear time).

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.h:395: int max_retries_;
Might be nice to comment this more explicitly, ditto with SetMaxRetries() which
doesn't have any comment.  Just so it doesn't get confused with your retry
counter.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.h:400: // Defaults to 0.
nit: Maybe change to "0 by default." to match two lines above.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:78: class TestNetworkChangeNotifier
: public NetworkChangeNotifier {
Using GMOCK for this might be a few lines shorter, for an example see
chrome/browser/chromeos/drive/drive_sync_client_unittest.cc

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:132:
network_change_notifier_.reset(new TestNetworkChangeNotifier);
Could |disable_default_notifier_| and |network_change_notifier_| just be simple
members rather than scoped_ptr's?  Then UseTestNetworkChangeNotifier() could be
eliminated.

[Joao da Silva, 2012-12-10 15:32:41]

+szym: please have a look.

Also, please +cc whoever you think should have a look at this! :-)

Addressed Paul's comments and added another test. This is now checking if
IsOffline() before issuing the first request. The fetchers that I tested were
aborted while at the resolving stage; with this change they still make it to the
resolving stage (the device is not offline), but an OnIPAddressChanged
notification kills this job which then succeeds on the retry. I've also seen
spdy connections being aborted the same way; see comments #28 and #29 at
http://crosbug.com/16114 for a netlog capture showing a device policy fetch
being interrupted by OnIPAddressChange
(https://code.google.com/p/chromium-os/issues/detail?id=16114#c29).

Regarding current users of ERR_ABORTED: grep shows that it's used in 73 files
(22 of which are tests). Most of them are triggering errors with ERR_ABORTED;
only 14 files verify if something else returned an ERR_ABORTED. Here's a
breakdown of those:

These should be OK:

1. android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
  (ERR_ABORTED generated by reasons that shouldn't propagate to embedder)
2. content/browser/download/download_resource_handler.cc
  (maps ERR_ABORTED to DOWNLOAD_INTERRUPT_REASON_USER_CANCELED)
3. content/browser/web_contents/web_contents_impl.cc
  ("EVIL HACK ALERT!" from 2008 to avoid crashes in interstitials)
4. content/renderer/render_view_impl.cc
  (skips error pages if the error is ERR_ABORTED)
5. net/http/http_pipelined_connection_impl.cc
  (ERR_ABORTED used to handle errors not caused by the server.
AddedERR_NETWORK_CHANGED as well)
6. webkit/appcache/appcache_quota_client.cc
  (converts to QuotaStatusCode)
7. webkit/glue/weburlloader_impl.cc
  (flags isCancellation on ERR_ABORTED)
8. webkit/plugins/npapi/webplugin_impl.cc
  (flags an error reason as NPRES_USER_BREAK if ERR_ABORTED, NPRES_NETWORK_ERR
otherwise)
9. webkit/plugins/ppapi/ppb_url_loader_impl.cc
  (converts ERR_ABORTED to PP_ERROR_ABORTED)
10. webkit/support/webkit_support.cc
  (converts ERR_ABORTED to NSURLErrorCancelled)
11. webkit/tools/test_shell/test_webview_delegate.cc
  (doesn't show an error page if the load was cancelled (ERR_ABORTED))

These I'm not sure:

12. chrome/browser/captive_portal/captive_portal_tab_helper.cc
  (prevents captive portal checks on ERR_ABORTED; not sure if it should also
catch ERR_NETWORK_CHANGED)

13. chrome/renderer/chrome_render_process_observer.cc
  (no idea why it handles ERR_ABORTED in a special way; maybe related to error
page displaying? (see render_view_impl.cc))

14. webkit/appcache/appcache_storage_impl.cc
  (appends a response ID to a deleted_response_ids_ vector if err !=
ERR_ABORTED. Not sure if this needs modifications)

If net/ people find this change agreeable then I'd land it and let it stay for a
week before using the new API, and see if anything breaks. I'd also shoot an
email to chromium-dev mentioning this change, and pinging the owners of at least
those 3 files to double check what to do on ERR_NETWORK_CHANGED.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:617: if (NetworkChangeNotifier::IsOffline())
{
On 2012/12/07 23:00:05, pauljensen wrote:
> I think it should be:
>   if (type == NetworkChangeNotifier::CONNECTION_NONE)

Done.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:883: max_retries_on_network_changes_--;
On 2012/12/07 23:00:05, pauljensen wrote:
> I notice the code above uses a counter and a maximum value, rather than your
> method of decreasing the maximum.  I don't know if one way is better than the
> other.  If there were some way to restart a fetch, then having a separate
> maximum might be nice because it could live across reuses.  Your call.

Done. Having a separate count also allows calling RemoveConnectionTypeObserver()
in ReleaseRequest() if there was a previous retry.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:912:
NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
On 2012/12/07 23:00:05, pauljensen wrote:
> Would be nice if we didn't always have to do this (i.e. just when we did
> register ourselves).  The ObserverList is rather slow (its thread safe so it
> locks and it's really a list so it's linear time).

Checking that |num_retries_on_network_changes > 0| is not a perfect signal, but
removes the need to do this from the majority of fetchers. WDYT?

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.h:395: int max_retries_;
On 2012/12/07 23:00:05, pauljensen wrote:
> Might be nice to comment this more explicitly, ditto with SetMaxRetries()
which
> doesn't have any comment.  Just so it doesn't get confused with your retry
> counter.

Done. Also renamed these fields and the setter/getter to make them clearer.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.h:400: // Defaults to 0.
On 2012/12/07 23:00:05, pauljensen wrote:
> nit: Maybe change to "0 by default." to match two lines above.

Done.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:78: class TestNetworkChangeNotifier
: public NetworkChangeNotifier {
On 2012/12/07 23:00:05, pauljensen wrote:
> Using GMOCK for this might be a few lines shorter, for an example see
> chrome/browser/chromeos/drive/drive_sync_client_unittest.cc

These tests still need to invoke NotifyObserversOfConnectionTypeChange(), which
is protected in the superclass. So the mock would be very similar to this, to
make that method public.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:132:
network_change_notifier_.reset(new TestNetworkChangeNotifier);
On 2012/12/07 23:00:05, pauljensen wrote:
> Could |disable_default_notifier_| and |network_change_notifier_| just be
simple
> members rather than scoped_ptr's?  Then UseTestNetworkChangeNotifier() could
be
> eliminated.

Done.

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:132:
network_change_notifier_.reset(new TestNetworkChangeNotifier);
On 2012/12/07 23:00:05, pauljensen wrote:
> Could |disable_default_notifier_| and |network_change_notifier_| just be
simple
> members rather than scoped_ptr's?  Then UseTestNetworkChangeNotifier() could
be
> eliminated.

Done.

[pauljensen, 2012-12-10 16:09:14]

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:912:
NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
On 2012/12/10 15:32:42, Joao da Silva wrote:
> On 2012/12/07 23:00:05, pauljensen wrote:
> > Would be nice if we didn't always have to do this (i.e. just when we did
> > register ourselves).  The ObserverList is rather slow (its thread safe so it
> > locks and it's really a list so it's linear time).
> 
> Checking that |num_retries_on_network_changes > 0| is not a perfect signal,
but
> removes the need to do this from the majority of fetchers. WDYT?
That seems fine.

[szym, 2012-12-10 18:36:31]

I have not looked at url_fetcher_impl_unittest.cc yet.

HostResolver and ClientSocketPool both have the property that they only return
ERR_ABORTED when:
A) they are "flushed" due to network change.
B) they are "aborted", i.e., they are destroyed.

I am not clear if in situation (B) they should be invoking the callbacks at all.

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1274: CompleteRequestsWithError(error);
The only changes needed in host_resolver_impl.{h,cc} are to
(1) change ERR_ABORTED to ERR_NETWORK_CHANGED here.

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1535: (entry.error != ERR_NETWORK_CHANGED) &&
(2) change ERR_ABORTED to ERR_NETWORK_CHANGED here.

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.h:212: void AbortAllInProgressJobs(int error);
This is only called on network changes, so only with ERR_NETWORK_CHANGED. Do not
introduce |int error| argument. Perhaps add comment that it notifies their
requests with ERR_NETWORK_CHANGED.

In general, HostResolver shall never pass "user cancelled" back to the caller
since the only way this can be done is using HostResolver::CancelRequest.

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolver.h
File net/base/mock_host_resolver.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolve...
net/base/mock_host_resolver.h:68: // Asynchronous requests are automatically
resolved when this is set to true,
Just as |set_asynchronous_mode| makes an exception to the normal behavior, I
think this should be an exception, so I suggest |set_resolve_ondemand_only| or
|ondemand_mode|. This way the reader can quickly notice that
|host_resolver_.set_resolve_ondemand_only(true)| introduces an exception.

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolve...
net/base/mock_host_resolver.h:88: // Resolves all pending requests. It is only
valid to invoked this if
nit: invoke

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
net/socket/client_socket_pool_base.h:246: void Flush(int error);
The documentation is now inconsistent, since ClientSocketPool::Flush does not
take |int error| argument.

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
net/socket/client_socket_pool_base.h:711: void Flush() {
helper_.Flush(ERR_ABORTED); }
We don't flush sockets on user request (aside from chrome://net-internals).

Outside of tests this is only called (indirectly) by:
ClientSocketPoolManagerImpl::OnCertAdded/OnCertTrustChanged, 
ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler. Assuming only
the first one is available in user sessions, this should probably be
ERR_NETWORK_CHANGED.

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:283: CloseCurrentSessions(ERR_ABORTED);
Probably should be ERR_NETWORK_CHANGED. This is called from
SSLConfigServiceManagerPref::OnPreferenceChanged, and even though it's a
user-induced change, I would consider this situation similar to a user meddling
with the network configuration rather than the user explicitly cancelling the
request.

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:350: CloseCurrentSessions(ERR_ABORTED);
Probably should be ERR_NETWORK_CHANGED.

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:358: CloseCurrentSessions(ERR_ABORTED);
Probably should be ERR_NETWORK_CHANGED.

https://codereview.chromium.org/11464028/diff/9001/net/url_request/test_url_f...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/test_url_f...
net/url_request/test_url_fetcher_factory.h:109: virtual void
SetMaxRetriesOn5xx(int max_retries) OVERRIDE;
It seems the only non-test place that cares about this is CloudPrintURLFetcher
and it does not use the URLFetcher's retry logic, so I'm not sure there's the
need to separate the maximums for 5xx retries and NetworkChange retries.

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:894: num_retries_on_network_changes_++;
nit: pre-increment

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.h:43: public
NetworkChangeNotifier::ConnectionTypeObserver {
Shouldn't it use NetworkChangeObserver?

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
Not necessary.

[Joao da Silva, 2012-12-11 13:36:42]

+Will: please review this too, in particular the changes to client_socket_pool
and url_fetcher_core.

@Paul, Szymon: please have another look.

Thanks!

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1274: CompleteRequestsWithError(error);
On 2012/12/10 18:36:31, szym wrote:
> The only changes needed in host_resolver_impl.{h,cc} are to
> (1) change ERR_ABORTED to ERR_NETWORK_CHANGED here.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1535: (entry.error != ERR_NETWORK_CHANGED) &&
On 2012/12/10 18:36:31, szym wrote:
> (2) change ERR_ABORTED to ERR_NETWORK_CHANGED here.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/host_resolver_imp...
net/base/host_resolver_impl.h:212: void AbortAllInProgressJobs(int error);
On 2012/12/10 18:36:31, szym wrote:
> This is only called on network changes, so only with ERR_NETWORK_CHANGED. Do
not
> introduce |int error| argument. Perhaps add comment that it notifies their
> requests with ERR_NETWORK_CHANGED.
> 
> In general, HostResolver shall never pass "user cancelled" back to the caller
> since the only way this can be done is using HostResolver::CancelRequest.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolver.h
File net/base/mock_host_resolver.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolve...
net/base/mock_host_resolver.h:68: // Asynchronous requests are automatically
resolved when this is set to true,
On 2012/12/10 18:36:31, szym wrote:
> Just as |set_asynchronous_mode| makes an exception to the normal behavior, I
> think this should be an exception, so I suggest |set_resolve_ondemand_only| or
> |ondemand_mode|. This way the reader can quickly notice that
> |host_resolver_.set_resolve_ondemand_only(true)| introduces an exception.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/base/mock_host_resolve...
net/base/mock_host_resolver.h:88: // Resolves all pending requests. It is only
valid to invoked this if
On 2012/12/10 18:36:31, szym wrote:
> nit: invoke

Done.

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
net/socket/client_socket_pool_base.h:246: void Flush(int error);
On 2012/12/10 18:36:31, szym wrote:
> The documentation is now inconsistent, since ClientSocketPool::Flush does not
> take |int error| argument.

Documented the |error| arg.

https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p...
net/socket/client_socket_pool_base.h:711: void Flush() {
helper_.Flush(ERR_ABORTED); }
On 2012/12/10 18:36:31, szym wrote:
> We don't flush sockets on user request (aside from chrome://net-internals).
> 
> Outside of tests this is only called (indirectly) by:
> ClientSocketPoolManagerImpl::OnCertAdded/OnCertTrustChanged, 
> ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler. Assuming only
> the first one is available in user sessions, this should probably be
> ERR_NETWORK_CHANGED.

Changed to ERR_NETWORK_CHANGED.

@Will: does this make sense to you as well?

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:283: CloseCurrentSessions(ERR_ABORTED);
On 2012/12/10 18:36:31, szym wrote:
> Probably should be ERR_NETWORK_CHANGED. This is called from
> SSLConfigServiceManagerPref::OnPreferenceChanged, and even though it's a
> user-induced change, I would consider this situation similar to a user
meddling
> with the network configuration rather than the user explicitly cancelling the
> request.
> 

Agreed, done.

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:350: CloseCurrentSessions(ERR_ABORTED);
On 2012/12/10 18:36:31, szym wrote:
> Probably should be ERR_NETWORK_CHANGED.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:358: CloseCurrentSessions(ERR_ABORTED);
On 2012/12/10 18:36:31, szym wrote:
> Probably should be ERR_NETWORK_CHANGED.

Done.

https://codereview.chromium.org/11464028/diff/9001/net/url_request/test_url_f...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/test_url_f...
net/url_request/test_url_fetcher_factory.h:109: virtual void
SetMaxRetriesOn5xx(int max_retries) OVERRIDE;
On 2012/12/10 18:36:31, szym wrote:
> It seems the only non-test place that cares about this is CloudPrintURLFetcher
> and it does not use the URLFetcher's retry logic, so I'm not sure there's the
> need to separate the maximums for 5xx retries and NetworkChange retries.

grep finds a couple more users:

chrome/browser/chrome_to_mobile_service.cc
chrome/browser/google/google_url_tracker.cc
chrome/browser/metrics/variations/variations_service.cc
chrome/browser/translate/translate_manager.cc
chrome/service/cloud_print/cloud_print_url_fetcher.cc
google_apis/gaia/gaia_oauth_client.cc

It's not clear to me if you're suggesting to merge the maximums for the
TestURLFetcher or for the URLFetcherCore. I think they can be merged in the
core; can you clarify this?

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:894: num_retries_on_network_changes_++;
On 2012/12/10 18:36:31, szym wrote:
> nit: pre-increment

Done.

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.h:43: public
NetworkChangeNotifier::ConnectionTypeObserver {
On 2012/12/10 18:36:31, szym wrote:
> Shouldn't it use NetworkChangeObserver?

This is only used to determine if IsOffline() changed, which only depends on the
connection type.

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/9001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
On 2012/12/10 18:36:31, szym wrote:
> Not necessary.

Done.

[pauljensen, 2012-12-11 16:46:29]

https://codereview.chromium.org/11464028/diff/26001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/26001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:310: num_retries_on_network_changes_ <
max_retries_on_network_changes_) {
I think you should swap these two conditions as IsOffline() can be slow, and at
least to start with, few people will be setting max_retries_on_network_changes_.

[willchan no longer on Chromium, 2012-12-11 17:39:33]

General nit for all the ClientSocketPool stuff. Please rename the member
functions. Flush()=>FlushWithError(),
AbortAllRequests()=>CancelAllRequestsWithError()

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:712: void Flush() {
helper_.Flush(ERR_NETWORK_CHANGED); }
It's non-obvious that Flush() should always mean ERR_NETWORK_CHANGED. Who calls
this? Isn't ClientSocketPoolBaseHelper already listening to the appropriate
signals and sending out the ERR_NETWORK_CHANGED cancellations?

[Joao da Silva, 2012-12-11 17:52:57]

Thanks for the comments.

@Will, Szymon: please see inline. Szymon's earlier comment makes sense, but
maybe Will knows better? :-)

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:712: void Flush() {
helper_.Flush(ERR_NETWORK_CHANGED); }
On 2012/12/11 17:39:33, willchan wrote:
> It's non-obvious that Flush() should always mean ERR_NETWORK_CHANGED. Who
calls
> this? Isn't ClientSocketPoolBaseHelper already listening to the appropriate
> signals and sending out the ERR_NETWORK_CHANGED cancellations?

The previous patch sets had ERR_ABORTED here, but Szymon clarified that this is
never triggered by a user action at
https://codereview.chromium.org/11464028/diff/9001/net/socket/client_socket_p....
WDYT?

https://codereview.chromium.org/11464028/diff/26001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11464028/diff/26001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:310: num_retries_on_network_changes_ <
max_retries_on_network_changes_) {
On 2012/12/11 16:46:30, pauljensen wrote:
> I think you should swap these two conditions as IsOffline() can be slow, and
at
> least to start with, few people will be setting
max_retries_on_network_changes_.

Good catch, done.

[szym, 2012-12-11 17:54:45]

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:712: void Flush() {
helper_.Flush(ERR_NETWORK_CHANGED); }
On 2012/12/11 17:39:33, willchan wrote:
> It's non-obvious that Flush() should always mean ERR_NETWORK_CHANGED. Who
calls
> this? Isn't ClientSocketPoolBaseHelper already listening to the appropriate
> signals and sending out the ERR_NETWORK_CHANGED cancellations?

I agree it's not obvious. I followed the call graph up and concluded that Flush
is only called (indirectly) by:
ClientSocketPoolManagerImpl::OnCertAdded/OnCertTrustChanged, 
ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler.

None of them are due to user action wanting to abort any requests (except the
net-internals WebUI). I compare those changes to the user changing the network
configuration (e.g., switching DNS servers), so ERR_NETWORK_CHANGED seems more
fitting than ERR_ABORTED.

The same logic applies to the call-sites of
SpdySessionPool::CloseCurrentSessions() which are OnCert* and
SSLConfigServiceManagerPref::OnPreferenceChanged.

[willchan no longer on Chromium, 2012-12-11 18:06:14]

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:712: void Flush() {
helper_.Flush(ERR_NETWORK_CHANGED); }
On 2012/12/11 17:54:45, szym wrote:
> On 2012/12/11 17:39:33, willchan wrote:
> > It's non-obvious that Flush() should always mean ERR_NETWORK_CHANGED. Who
> calls
> > this? Isn't ClientSocketPoolBaseHelper already listening to the appropriate
> > signals and sending out the ERR_NETWORK_CHANGED cancellations?
> 
> I agree it's not obvious. I followed the call graph up and concluded that
Flush
> is only called (indirectly) by:
> ClientSocketPoolManagerImpl::OnCertAdded/OnCertTrustChanged, 
> ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler.
> 
> None of them are due to user action wanting to abort any requests (except the
> net-internals WebUI). I compare those changes to the user changing the network
> configuration (e.g., switching DNS servers), so ERR_NETWORK_CHANGED seems more
> fitting than ERR_ABORTED.
> 
> The same logic applies to the call-sites of
> SpdySessionPool::CloseCurrentSessions() which are OnCert* and
> SSLConfigServiceManagerPref::OnPreferenceChanged.

Assuming ERR_NETWORK_CHANGED sounds wrong here then. I'd rather also do
FlushWithError() or something. ChromeBenchmarkingMessageFilter is actually
triggered by user action, since when you start the benchmark, it tries to flush
all network state so it can have a "clean" run.

[Joao da Silva, 2012-12-11 21:14:55]

Please have another look.

Besides the renames, the last patch also adds an error param to
FlushSocketPools, so that it's invoked with ERR_ABORTED when invoked from the
chrome/ message filters, and with ERR_NETWORK_CHANGED when invoked from the
OnCert* callbacks.

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/11464028/diff/26001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:712: void Flush() {
helper_.Flush(ERR_NETWORK_CHANGED); }
On 2012/12/11 18:06:14, willchan wrote:
> On 2012/12/11 17:54:45, szym wrote:
> > On 2012/12/11 17:39:33, willchan wrote:
> > > It's non-obvious that Flush() should always mean ERR_NETWORK_CHANGED. Who
> > calls
> > > this? Isn't ClientSocketPoolBaseHelper already listening to the
appropriate
> > > signals and sending out the ERR_NETWORK_CHANGED cancellations?
> > 
> > I agree it's not obvious. I followed the call graph up and concluded that
> Flush
> > is only called (indirectly) by:
> > ClientSocketPoolManagerImpl::OnCertAdded/OnCertTrustChanged, 
> > ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler.
> > 
> > None of them are due to user action wanting to abort any requests (except
the
> > net-internals WebUI). I compare those changes to the user changing the
network
> > configuration (e.g., switching DNS servers), so ERR_NETWORK_CHANGED seems
more
> > fitting than ERR_ABORTED.
> > 
> > The same logic applies to the call-sites of
> > SpdySessionPool::CloseCurrentSessions() which are OnCert* and
> > SSLConfigServiceManagerPref::OnPreferenceChanged.
> 
> Assuming ERR_NETWORK_CHANGED sounds wrong here then. I'd rather also do
> FlushWithError() or something. ChromeBenchmarkingMessageFilter is actually
> triggered by user action, since when you start the benchmark, it tries to
flush
> all network state so it can have a "clean" run.

Flush() has been changed to FlushWithError(int) as suggested.

I've also added the error parameter to FlushSocketPools (as
FlushSocketPoolsWithError), so that HttpNetworkSession::CloseAllConnections()
(which is what ChromeBenchmarkingMessageFilter and NetInternalsMessageHandler
end up calling) invokes it with ERR_ABORTED, and the certs-changes callbacks
invoke it with ERR_NETWORK_CHANGED.

[szym, 2012-12-12 19:50:46]

LGTM with minor remarks.

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
net/url_request/url_fetcher.h:200: // attempted once the network connection is
back.
Not immediately obvious here is that if |max_retries > 0| and
|NetworkChangeNotifier::IsOffline()| then it won't actually start until the next
change to online.

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:162: class URLFetcherMockDNSTest :
public URLFetcherTest {
nit: Suggest 'Dns' (like 'Http' and most code in net/dns/).

[willchan no longer on Chromium, 2012-12-12 19:53:04]

Just to be clear, I've already sent all my comments, and trust that
szym/pauljensen/whoever takes them into consideration in their review. I'm
ducking out of the rest of this, so no need to wait for anything more for me as
there's nothing more forthcoming :)

[Joao da Silva, 2012-12-12 20:17:20]

Cool, thanks for the reviews :-)

@Paul: please have one last look at the current patch.

+joi and +jochen to TBR for the mechanical changes to chrome/ and google_apis/.
Feel free to comment as well!

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
net/url_request/url_fetcher.h:200: // attempted once the network connection is
back.
On 2012/12/12 19:50:46, szym wrote:
> Not immediately obvious here is that if |max_retries > 0| and
> |NetworkChangeNotifier::IsOffline()| then it won't actually start until the
next
> change to online.

Clarified that in the comment.

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11464028/diff/28016/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:162: class URLFetcherMockDNSTest :
public URLFetcherTest {
On 2012/12/12 19:50:46, szym wrote:
> nit: Suggest 'Dns' (like 'Http' and most code in net/dns/).

Done.

[Jói, 2012-12-12 21:16:12]

LGTM

On Mon, Dec 10, 2012 at 3:32 PM,  <joaodasilva@chromium.org> wrote:
> +szym: please have a look.
>
> Also, please +cc whoever you think should have a look at this! :-)
>
> Addressed Paul's comments and added another test. This is now checking if
> IsOffline() before issuing the first request. The fetchers that I tested
> were
> aborted while at the resolving stage; with this change they still make it to
> the
> resolving stage (the device is not offline), but an OnIPAddressChanged
> notification kills this job which then succeeds on the retry. I've also seen
> spdy connections being aborted the same way; see comments #28 and #29 at
> http://crosbug.com/16114 for a netlog capture showing a device policy fetch
> being interrupted by OnIPAddressChange
> (https://code.google.com/p/chromium-os/issues/detail?id=16114#c29).
>
> Regarding current users of ERR_ABORTED: grep shows that it's used in 73
> files
> (22 of which are tests). Most of them are triggering errors with
> ERR_ABORTED;
> only 14 files verify if something else returned an ERR_ABORTED. Here's a
> breakdown of those:
>
> These should be OK:
>
> 1.
> android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
>   (ERR_ABORTED generated by reasons that shouldn't propagate to embedder)
> 2. content/browser/download/download_resource_handler.cc
>   (maps ERR_ABORTED to DOWNLOAD_INTERRUPT_REASON_USER_CANCELED)
> 3. content/browser/web_contents/web_contents_impl.cc
>   ("EVIL HACK ALERT!" from 2008 to avoid crashes in interstitials)
> 4. content/renderer/render_view_impl.cc
>   (skips error pages if the error is ERR_ABORTED)
> 5. net/http/http_pipelined_connection_impl.cc
>   (ERR_ABORTED used to handle errors not caused by the server.
> AddedERR_NETWORK_CHANGED as well)
> 6. webkit/appcache/appcache_quota_client.cc
>   (converts to QuotaStatusCode)
> 7. webkit/glue/weburlloader_impl.cc
>   (flags isCancellation on ERR_ABORTED)
> 8. webkit/plugins/npapi/webplugin_impl.cc
>   (flags an error reason as NPRES_USER_BREAK if ERR_ABORTED,
> NPRES_NETWORK_ERR
> otherwise)
> 9. webkit/plugins/ppapi/ppb_url_loader_impl.cc
>   (converts ERR_ABORTED to PP_ERROR_ABORTED)
> 10. webkit/support/webkit_support.cc
>   (converts ERR_ABORTED to NSURLErrorCancelled)
> 11. webkit/tools/test_shell/test_webview_delegate.cc
>   (doesn't show an error page if the load was cancelled (ERR_ABORTED))
>
> These I'm not sure:
>
> 12. chrome/browser/captive_portal/captive_portal_tab_helper.cc
>   (prevents captive portal checks on ERR_ABORTED; not sure if it should also
> catch ERR_NETWORK_CHANGED)
>
> 13. chrome/renderer/chrome_render_process_observer.cc
>   (no idea why it handles ERR_ABORTED in a special way; maybe related to
> error
> page displaying? (see render_view_impl.cc))
>
> 14. webkit/appcache/appcache_storage_impl.cc
>   (appends a response ID to a deleted_response_ids_ vector if err !=
> ERR_ABORTED. Not sure if this needs modifications)
>
> If net/ people find this change agreeable then I'd land it and let it stay
> for a
> week before using the new API, and see if anything breaks. I'd also shoot an
> email to chromium-dev mentioning this change, and pinging the owners of at
> least
> those 3 files to double check what to do on ERR_NETWORK_CHANGED.
>
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> File net/url_request/url_fetcher_core.cc (right):
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.cc:617: if
> (NetworkChangeNotifier::IsOffline()) {
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> I think it should be:
>>    if (type == NetworkChangeNotifier::CONNECTION_NONE)
>
>
> Done.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.cc:883:
> max_retries_on_network_changes_--;
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> I notice the code above uses a counter and a maximum value, rather
>
> than your
>>
>> method of decreasing the maximum.  I don't know if one way is better
>
> than the
>>
>> other.  If there were some way to restart a fetch, then having a
>
> separate
>>
>> maximum might be nice because it could live across reuses.  Your call.
>
>
> Done. Having a separate count also allows calling
> RemoveConnectionTypeObserver() in ReleaseRequest() if there was a
> previous retry.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.cc:912:
> NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> Would be nice if we didn't always have to do this (i.e. just when we
>
> did
>>
>> register ourselves).  The ObserverList is rather slow (its thread safe
>
> so it
>>
>> locks and it's really a list so it's linear time).
>
>
> Checking that |num_retries_on_network_changes > 0| is not a perfect
> signal, but removes the need to do this from the majority of fetchers.
> WDYT?
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> File net/url_request/url_fetcher_core.h (right):
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.h:395: int max_retries_;
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> Might be nice to comment this more explicitly, ditto with
>
> SetMaxRetries() which
>>
>> doesn't have any comment.  Just so it doesn't get confused with your
>
> retry
>>
>> counter.
>
>
> Done. Also renamed these fields and the setter/getter to make them
> clearer.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.h:400: // Defaults to 0.
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> nit: Maybe change to "0 by default." to match two lines above.
>
>
> Done.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
> File net/url_request/url_fetcher_impl_unittest.cc (right):
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
> net/url_request/url_fetcher_impl_unittest.cc:78: class
> TestNetworkChangeNotifier : public NetworkChangeNotifier {
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> Using GMOCK for this might be a few lines shorter, for an example see
>> chrome/browser/chromeos/drive/drive_sync_client_unittest.cc
>
>
> These tests still need to invoke
> NotifyObserversOfConnectionTypeChange(), which is protected in the
> superclass. So the mock would be very similar to this, to make that
> method public.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
> net/url_request/url_fetcher_impl_unittest.cc:132:
> network_change_notifier_.reset(new TestNetworkChangeNotifier);
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> Could |disable_default_notifier_| and |network_change_notifier_| just
>
> be simple
>>
>> members rather than scoped_ptr's?  Then UseTestNetworkChangeNotifier()
>
> could be
>>
>> eliminated.
>
>
> Done.
>
>
>
https://codereview.chromium.org/11464028/diff/1/net/url_request/url_fetcher_i...
> net/url_request/url_fetcher_impl_unittest.cc:132:
> network_change_notifier_.reset(new TestNetworkChangeNotifier);
> On 2012/12/07 23:00:05, pauljensen wrote:
>>
>> Could |disable_default_notifier_| and |network_change_notifier_| just
>
> be simple
>>
>> members rather than scoped_ptr's?  Then UseTestNetworkChangeNotifier()
>
> could be
>>
>> eliminated.
>
>
> Done.
>
> https://codereview.chromium.org/11464028/

[pauljensen, 2012-12-12 23:41:33]

lgtm but I'd check with owners of those three files checking ERR_ABORTED before
landing.  I'd also email chromium-dev a few days prior to landing.

[jochen (gone - plz use gerrit), 2012-12-13 10:04:01]

lgtm

[Joao da Silva, 2012-12-13 11:03:27]

Last checks before landing, adding +mmenke, +jam and +michaeln (see below for
what to look at).

This change makes network changes (ip address change, dns change, etc) cause
ERR_NETWORK_CHANGED instead of ERR_ABORTED. The following 3 files test for
ERR_ABORTED and I'd like your opinion on whether they're fine as is or whether
they should also test for ERR_NETWORK_CHANGED now:

1. chrome/browser/captive_portal/captive_portal_tab_helper.cc

@mmenke: DidFailProvisionalLoad() invokes OnLoadAborted() on ERR_ABORTED. I
think that's OK as is, do you think it should also do that on
ERR_NETWORK_CHANGED?

2. chrome/renderer/chrome_render_process_observer.cc

@jam: there's no comment about why RendererResourceDelegate::OnRequestComplete
quits earlier on ERR_ABORTED, can you clarify if this is still correct or if
ERR_NETWORK_CHANGED should also quit? I'm not sure if this is related to
RenderViewImpl::cancelledError() which sets the cancellation error to
ERR_ABORTED; it later checks for this error in didFailProvisionalLoad().

3. webkit/appcache/appcache_storage_impl.cc

@michaeln: AppCacheStorageImpl::OnDeletedOneResponse special-cases ERR_ABORTED,
do you think it should do the same on ERR_NETWORK_CHANGED? It looks like this
error comes from AppCacheDiskCache::DoomEntry(), which returns ERR_ABORTED when
|is_disabled_|, so I think the code should stay as is.

Thanks!

[jam, 2012-12-13 16:48:35]

On 2012/12/13 11:03:27, Joao da Silva wrote:
> 2. chrome/renderer/chrome_render_process_observer.cc
> 
> @jam: there's no comment about why RendererResourceDelegate::OnRequestComplete
> quits earlier on ERR_ABORTED, can you clarify if this is still correct or if
> ERR_NETWORK_CHANGED should also quit? I'm not sure if this is related to
> RenderViewImpl::cancelledError() which sets the cancellation error to
> ERR_ABORTED; it later checks for this error in didFailProvisionalLoad().

I'm not familiar with the details of this code. I had refactored it, but if you
trace this all through viewvc, you'll see that it was like this in initial
commit

[mmenke, 2012-12-13 16:53:17]

CaptivePortalHelper differentiates OnLoadAborted from other errors because it
doesn't result in an error page - ERR_NETWORK_CHANGED will, so it's fine as-is. 
I do appreciate your asking me about it, though.

Speaking of error pages...  I think this error may be common enough that we want
to make a custom error page for it.  The code is in common/localized_error.  I'm
thinking we want to suggest reloading and checking connection, with a brief
summary and details (details is just so that we don't show "Unknown error", and
should be largely redundant with the summary).

If we're landing this before branch point, I think we want the error page in
there, too.  It's unclear how common this error is, but the generic error page
is just bad.

On 2012/12/13 11:03:27, Joao da Silva wrote:
> Last checks before landing, adding +mmenke, +jam and +michaeln (see below for
> what to look at).
> 
> This change makes network changes (ip address change, dns change, etc) cause
> ERR_NETWORK_CHANGED instead of ERR_ABORTED. The following 3 files test for
> ERR_ABORTED and I'd like your opinion on whether they're fine as is or whether
> they should also test for ERR_NETWORK_CHANGED now:
> 
> 1. chrome/browser/captive_portal/captive_portal_tab_helper.cc
> 
> @mmenke: DidFailProvisionalLoad() invokes OnLoadAborted() on ERR_ABORTED. I
> think that's OK as is, do you think it should also do that on
> ERR_NETWORK_CHANGED?
> 
> 2. chrome/renderer/chrome_render_process_observer.cc
> 
> @jam: there's no comment about why RendererResourceDelegate::OnRequestComplete
> quits earlier on ERR_ABORTED, can you clarify if this is still correct or if
> ERR_NETWORK_CHANGED should also quit? I'm not sure if this is related to
> RenderViewImpl::cancelledError() which sets the cancellation error to
> ERR_ABORTED; it later checks for this error in didFailProvisionalLoad().
> 
> 3. webkit/appcache/appcache_storage_impl.cc
> 
> @michaeln: AppCacheStorageImpl::OnDeletedOneResponse special-cases
ERR_ABORTED,
> do you think it should do the same on ERR_NETWORK_CHANGED? It looks like this
> error comes from AppCacheDiskCache::DoomEntry(), which returns ERR_ABORTED
when
> |is_disabled_|, so I think the code should stay as is.
> 
> Thanks!

[mmenke, 2012-12-13 16:55:14]

On 2012/12/13 16:53:17, Matt Menke wrote:
> CaptivePortalHelper differentiates OnLoadAborted from other errors because it
> doesn't result in an error page - ERR_NETWORK_CHANGED will, so it's fine
as-is. 
> I do appreciate your asking me about it, though.
> 
> Speaking of error pages...  I think this error may be common enough that we
want
> to make a custom error page for it.  The code is in common/localized_error. 
I'm
> thinking we want to suggest reloading and checking connection, with a brief
> summary and details (details is just so that we don't show "Unknown error",
and
> should be largely redundant with the summary).
> 
> If we're landing this before branch point, I think we want the error page in
> there, too.  It's unclear how common this error is, but the generic error page
> is just bad.

Error page before branch, too, rather.  I'm happy to help with the error page,
if you have questions.

[michaeln, 2012-12-13 21:22:24]

> 3. webkit/appcache/appcache_storage_impl.cc

Yup, the source of that error is the disk_cache so not changing
appcache_storage_impl.cc lgtm.

Just took a look at the bug, another place to automatically retry based on the
new error code could be inside of AppCacheUpdateJob::URLFetcher.


> 
> @michaeln: AppCacheStorageImpl::OnDeletedOneResponse special-cases
ERR_ABORTED,
> do you think it should do the same on ERR_NETWORK_CHANGED? It looks like this
> error comes from AppCacheDiskCache::DoomEntry(), which returns ERR_ABORTED
when
> |is_disabled_|, so I think the code should stay as is.

[Joao da Silva, 2012-12-14 01:23:08]

Thanks all for your comments.

As John mentioned, the code in RendererResourceDelegate::OnRequestComplete comes
from the initial import:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/resource_dispat...

ERR_ABORTED was a catch-all meaning "non-specific error". I'll leave this code
as is.

@mmenke: I'd like to land this for 25 if possible, to fix the bugs blocked by
this. I don't think we'll have an error page ready in time, WDYT? Since it
probably introduces new strings I don't think it can be merged back later. Are
you OK with the error page for 26 only?

If so then I'll land this Friday morning (CET time), when the tree is slower. If
it doesn't break anything then I'll fix the other bugs, and file a bug for the
error page.

[mmenke, 2012-12-14 01:41:09]

On 2012/12/14 01:23:08, Joao da Silva wrote:
> Thanks all for your comments.
> 
> As John mentioned, the code in RendererResourceDelegate::OnRequestComplete
comes
> from the initial import:
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/resource_dispat...
> 
> ERR_ABORTED was a catch-all meaning "non-specific error". I'll leave this code
> as is.
> 
> @mmenke: I'd like to land this for 25 if possible, to fix the bugs blocked by
> this. I don't think we'll have an error page ready in time, WDYT? Since it
> probably introduces new strings I don't think it can be merged back later. Are
> you OK with the error page for 26 only?
> 
> If so then I'll land this Friday morning (CET time), when the tree is slower.
If
> it doesn't break anything then I'll fix the other bugs, and file a bug for the
> error page.

You're right that M25 error strings should not be updated after the branch.  All
you need to do is add 2 or 3 strings, so not a huge time sink, but if there are
other things you need to get in before the branch...

The problem is just that the default error page isn't too useful, and it's weird
to give an error page that only gives the actual problem in a way that English
speakers can understand it.  I'm not a big fan of that, but 50+% of our error
pages used to do that (And some rarer errors still do), so it's not exactly new
problem.  In other words, I don't think it's a great idea, but I can live with
it.

[commit-bot: I haz the power, 2012-12-14 12:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11464028/39003

[commit-bot: I haz the power, 2012-12-14 15:39:03]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-14 16:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11464028/39003

[commit-bot: I haz the power, 2012-12-14 20:40:46]

Retried try job too often on win_rel for step(s) browser_tests