NavigationMojoResponse: two test regressed. |
||
Issue descriptionThe two tests are no more working with NavigationMojoResponse. * DNSErrorPageTest.StaleCacheStatus * ErrorPageNavigationCorrectionsFailTest.StaleCacheStatusFailedCorrections For the second one, it was working at least until commit 71cf20d31324 and is no more working now (commit 9a36904547f5). They regressed the last couple of days. I will start a bisection.
,
Mar 15 2018
I reverted the CL here: https://chromium-review.googlesource.com/c/chromium/src/+/964461
,
Mar 15 2018
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb2025ae3516f5f7442401a8ce84f3c099de7de9 commit bb2025ae3516f5f7442401a8ce84f3c099de7de9 Author: Conley Owens <cco3@chromium.org> Date: Fri Mar 23 23:34:00 2018 Report was_cached in SubresourceLoadInfo This change adds a boolean `was_cached` to the SubresourceLoadInfo struct. This lets consumers know whether or not the response was fetched from the network cache. Notably, this is important for recording page load metrics when the Network Service is enabled. This reverts commit 0b2b1a92072144890a713700be568c9fc3e520c4. Issues in the previous commit are fixed by retaining the exists_in_cache field. In a many cases when NavigationURLLoaderDelegate::OnRequestFailed() is called, NavigationURLLoaderNetworkService::OnResponseStarted() has not been called. In these cases, we cannot rely on using a stored was_cached value, since it may not be properly updated. BUG=816684, 822237 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I0beb72b825ae0a12122170cec2e4f4358dda58af Reviewed-on: https://chromium-review.googlesource.com/966792 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Conley Owens <cco3@chromium.org> Cr-Commit-Position: refs/heads/master@{#545626} [modify] https://crrev.com/bb2025ae3516f5f7442401a8ce84f3c099de7de9/content/browser/web_contents/web_contents_impl_browsertest.cc [modify] https://crrev.com/bb2025ae3516f5f7442401a8ce84f3c099de7de9/content/public/common/subresource_load_info.mojom [modify] https://crrev.com/bb2025ae3516f5f7442401a8ce84f3c099de7de9/content/renderer/loader/resource_dispatcher.cc |
||
►
Sign in to add a comment |
||
Comment 1 by arthurso...@chromium.org
, Mar 15 2018The regression come from: Report was_cached in SubresourceLoadInfo This change adds a boolean `was_cached` to the SubresourceLoadInfo struct. This lets consumers know whether or not the response was fetched from the network cache. Notably, this is important for recording page load metrics when the Network Service is enabled. https://chromium-review.googlesource.com/c/chromium/src/+/938650 +CC author of the patch. I think the issue is in: --------------------------------------------------------------------------------- void NavigationURLLoaderNetworkService::OnComplete( const network::URLLoaderCompletionStatus& status) { if (status.error_code == net::OK) return; TRACE_EVENT_ASYNC_END2("navigation", "Navigation timeToResponseStarted", this, "&NavigationURLLoaderNetworkService", this, "success", false); + delegate_->OnRequestFailed(status.exists_in_cache, status.error_code, - delegate_->OnRequestFailed(response_was_cached_, status.error_code, status.ssl_info); } --------------------------------------------------------------------------------- Note: Ironically NavigationURLLoaderNetworkService is also used without the NetworkService :-) We are going to replace the NavigationURLLoader by the NavigationURLLoaderNetworkService soon. The two regressing tests are already disabled with the NetworkService. I don't really know if there is something wrong with this patch or if it requires a different behaviour whether the NetworkService is enabled or not.