Fix encoded_body_size for the race-network-and-fetch-handler
This is the bug fix CL to address the issue that the `encodedBodySize` in the resource timing API has `0` even if it actually has body data. This is because the response head lacks `was_fetched_via_service_worker` flag when the matched router source is `race-network-and-fetch-handler` and the network wins the race. Without `was_fetched_via_service_worker`, the encoded body size is not calculated in `DocumentLoader` [1]. This is a short-term fix, eventually we should change the code to handle the response from the network as if the response came from the fetch event for the case of `race-network-and-fetch-handler` for the consistnecy. [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/document_loader.cc;l=1378-1384 Bug: 404577046 Change-Id: I400a2563fc957a97053b98e64b28048b63cdcbf4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6432968 Commit-Queue: Shunya Shishido <sisidovski@chromium.org> Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org> Reviewed-by: Keita Suzuki <suzukikeita@chromium.org> Reviewed-by: Shunya Shishido <sisidovski@chromium.org> Cr-Commit-Position: refs/heads/main@{#1445641}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
f95d6bb383
commit
ce7760e7ed
content
browser
service_worker
common
service_worker
third_party/blink/web_tests/external/wpt/service-workers/service-worker/tentative/static-router
@ -40,6 +40,7 @@
|
||||
#include "services/network/public/cpp/single_request_url_loader_factory.h"
|
||||
#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
|
||||
#include "services/network/public/mojom/fetch_api.mojom.h"
|
||||
#include "services/network/public/mojom/service_worker_router_info.mojom-shared.h"
|
||||
#include "services/network/public/mojom/url_response_head.mojom.h"
|
||||
#include "services/network/test/test_url_loader_client.h"
|
||||
#include "storage/browser/blob/blob_data_builder.h"
|
||||
@ -736,21 +737,43 @@ class ServiceWorkerMainResourceLoaderTest : public testing::Test {
|
||||
const network::mojom::URLResponseHead& expected_info) {
|
||||
bool expect_service_worker_timing =
|
||||
expected_info.was_fetched_via_service_worker;
|
||||
if (expected_info.service_worker_router_info &&
|
||||
(expected_info.service_worker_router_info->actual_source_type ==
|
||||
network::mojom::ServiceWorkerRouterSourceType::kCache)) {
|
||||
expect_service_worker_timing = false;
|
||||
network::mojom::IPAddressSpace expected_client_address_space =
|
||||
expected_info.was_fetched_via_service_worker
|
||||
? network::mojom::IPAddressSpace::kPrivate
|
||||
: network::mojom::IPAddressSpace::kUnknown;
|
||||
if (expected_info.service_worker_router_info) {
|
||||
if (expected_info.service_worker_router_info->actual_source_type ==
|
||||
network::mojom::ServiceWorkerRouterSourceType::kCache) {
|
||||
// If the actual router source is Cache, we don't expect service worker
|
||||
// timing info.
|
||||
expect_service_worker_timing = false;
|
||||
}
|
||||
if (expected_info.service_worker_router_info->matched_source_type ==
|
||||
network::mojom::ServiceWorkerRouterSourceType::
|
||||
kRaceNetworkAndFetchEvent &&
|
||||
expected_info.service_worker_router_info->actual_source_type ==
|
||||
network::mojom::ServiceWorkerRouterSourceType::kNetwork) {
|
||||
// If the matched router source is race and the actual source is
|
||||
// network, we don't expect service worker timing info.
|
||||
// `expected_client_address_space` is `kUnknown` as well since this is
|
||||
// not override in the ServiceWorkerMainResourceLoader for now.
|
||||
//
|
||||
// TODO(crbug.com/408309960): Update this to handle the response as if
|
||||
// it comes from the fetch event so that every information is propagated
|
||||
// correctly.
|
||||
expect_service_worker_timing = false;
|
||||
expected_client_address_space =
|
||||
network::mojom::IPAddressSpace::kUnknown;
|
||||
}
|
||||
}
|
||||
|
||||
EXPECT_EQ(expected_info.was_fetched_via_service_worker,
|
||||
info.was_fetched_via_service_worker);
|
||||
EXPECT_EQ(expected_info.url_list_via_service_worker,
|
||||
info.url_list_via_service_worker);
|
||||
EXPECT_EQ(expected_info.response_type, info.response_type);
|
||||
EXPECT_EQ(expected_info.response_time, info.response_time);
|
||||
EXPECT_EQ(info.client_address_space,
|
||||
expected_info.was_fetched_via_service_worker
|
||||
? network::mojom::IPAddressSpace::kPrivate
|
||||
: network::mojom::IPAddressSpace::kUnknown);
|
||||
EXPECT_EQ(info.client_address_space, expected_client_address_space);
|
||||
EXPECT_EQ(!info.load_timing.service_worker_start_time.is_null(),
|
||||
expect_service_worker_timing);
|
||||
EXPECT_EQ(!info.load_timing.service_worker_ready_time.is_null(),
|
||||
@ -1563,7 +1586,7 @@ TEST_F(ServiceWorkerMainResourceLoaderTest, StaticRoutingRaceNetworkWin) {
|
||||
EXPECT_FALSE(info->load_timing.receive_headers_start.is_null());
|
||||
EXPECT_FALSE(info->load_timing.receive_headers_end.is_null());
|
||||
auto expected_info = CreateResponseInfoFromServiceWorker();
|
||||
expected_info->was_fetched_via_service_worker = false;
|
||||
expected_info->was_fetched_via_service_worker = true;
|
||||
auto expected_router_info = CreateExpectedMatchingServiceWorkerRouterInfo(
|
||||
network::mojom::ServiceWorkerRouterSourceType::kRaceNetworkAndFetchEvent);
|
||||
expected_router_info->actual_source_type =
|
||||
|
@ -133,6 +133,16 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::OnReceiveResponse(
|
||||
head_->load_timing.receive_headers_end =
|
||||
head_->load_timing.receive_headers_start;
|
||||
cached_metadata_ = std::move(cached_metadata);
|
||||
// TODO(crbug.com/408309960): Update to call `owner_`'s
|
||||
// DidDispatchFetchEvent() or StartResponse() to handle the response as if
|
||||
// it comes from the fetch event so that every information is propagated
|
||||
// correctly.
|
||||
//
|
||||
// For now, set was_fetched_via_service_worker true here to address
|
||||
// crbug.com/404577046. Since this field is normally set by
|
||||
// blink::ServiceWorkerLoaderHelpers::SaveResponseInfo(). But currently
|
||||
// this is called only when the response is returned from the fetch event.
|
||||
head_->was_fetched_via_service_worker = true;
|
||||
if (base::FeatureList::IsEnabled(
|
||||
features::
|
||||
kServiceWorkerStaticRouterRaceNetworkRequestPerformanceImprovement) &&
|
||||
|
@ -56,10 +56,12 @@ function test_resource_timing(options) {
|
||||
assert_equals(entry.workerStart, 0, description);
|
||||
assert_equals(entry.workerCacheLookupStart, 0, description);
|
||||
assert_less_than_equal(entry.workerRouterEvaluationStart, entry.fetchStart, description);
|
||||
assert_greater_than(entry.encodedBodySize, 0, description);
|
||||
break;
|
||||
case 'cache':
|
||||
assert_equals(entry.workerStart, 0, description);
|
||||
assert_greater_than_equal(entry.workerCacheLookupStart, entry.workerRouterEvaluationStart, description);
|
||||
assert_greater_than(entry.encodedBodySize, 0, description);
|
||||
if (entry.workerFinalSourceType === 'cache') {
|
||||
assert_equals(entry.fetchStart, entry.responseStart, description);
|
||||
assert_less_than_equal(entry.workerCacheLookupStart, entry.responseStart, description);
|
||||
@ -70,6 +72,7 @@ function test_resource_timing(options) {
|
||||
break;
|
||||
case 'race-network-and-fetch-handler':
|
||||
assert_equals(entry.workerCacheLookupStart, 0, description);
|
||||
assert_greater_than(entry.encodedBodySize, 0, description);
|
||||
if (entry.workerFinalSourceType === 'network') {
|
||||
assert_equals(entry.workerStart, 0, description);
|
||||
assert_less_than_equal(entry.workerRouterEvaluationStart, entry.fetchStart, description);
|
||||
@ -79,6 +82,8 @@ function test_resource_timing(options) {
|
||||
}
|
||||
break;
|
||||
case 'fetch-event':
|
||||
assert_greater_than(entry.encodedBodySize, 0, description);
|
||||
break;
|
||||
case '': // i.e. no matching rules
|
||||
assert_equals(entry.workerCacheLookupStart, 0, description);
|
||||
assert_greater_than_equal(entry.workerStart, entry.workerRouterEvaluationStart, description);
|
||||
|
Reference in New Issue
Block a user