0

Remove ServiceWorkerRaceNetworkRequestURLLoaderClient::Write()

This methoed could cause a crash issue and currently not used at all.
Removing this and some of crash keys as well.

Bug: 355016923
Change-Id: I4c79adc3113dc782cbee5f45e52c04751e201778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5736871
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Commit-Queue: Shunya Shishido <sisidovski@chromium.org>
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1332172}
This commit is contained in:
Shunya Shishido
2024-07-24 07:02:13 +00:00
committed by Chromium LUCI CQ
parent ad0d146667
commit be13b8f186
3 changed files with 31 additions and 174 deletions

@@ -6253,16 +6253,12 @@ IN_PROC_BROWSER_TEST_F(
}
class ServiceWorkerAutoPreloadBrowserTest
: public ServiceWorkerStaticRouterRaceNetworkAndFetchHandlerSourceBrowserTest,
public testing::WithParamInterface<bool> {
: public ServiceWorkerStaticRouterRaceNetworkAndFetchHandlerSourceBrowserTest {
public:
static constexpr char kSwScriptUrl[] = "/service_worker/auto_preload.js";
ServiceWorkerAutoPreloadBrowserTest() {
feature_list_.InitWithFeaturesAndParameters(
{{features::kServiceWorkerAutoPreload,
{{"use_two_phase_write", GetParam() ? "true" : "false"}}}},
{});
feature_list_.InitWithFeatures({{features::kServiceWorkerAutoPreload}}, {});
RaceNetworkRequestWriteBufferManager::SetDataPipeCapacityBytesForTesting(
1024);
}
@@ -6277,11 +6273,7 @@ class ServiceWorkerAutoPreloadBrowserTest
base::test::ScopedFeatureList feature_list_;
};
INSTANTIATE_TEST_SUITE_P(ALL,
ServiceWorkerAutoPreloadBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
NetworkRequestRepliedFirstButFetchHandlerResultIsUsed) {
// Register the ServiceWorker and navigate to the in scope URL.
SetupAndRegisterServiceWorker();
@@ -6314,7 +6306,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
observer.GetNormalizedResponseHeader("X-Response-From"));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest, PassThrough) {
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest, PassThrough) {
// Register the ServiceWorker and navigate to the in scope URL.
SetupAndRegisterServiceWorker();
// Capture the response head.
@@ -6338,7 +6330,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest, PassThrough) {
EXPECT_EQ(1, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
PassThrough_LargeData) {
SetupAndRegisterServiceWorker();
const std::string relative_url =
@@ -6363,7 +6355,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
observer.GetNormalizedResponseHeader("X-Response-From"));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
NetworkRequest_Wins_FetchHandler_Fallback) {
// Register the ServiceWorker and navigate to the in scope URL.
SetupAndRegisterServiceWorker();
@@ -6385,7 +6377,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
EXPECT_EQ(2, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
FetchHandler_Wins_Fallback) {
// Register the ServiceWorker and navigate to the in scope URL.
SetupAndRegisterServiceWorker();
@@ -6406,7 +6398,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
EXPECT_EQ(2, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(
IN_PROC_BROWSER_TEST_F(
ServiceWorkerAutoPreloadBrowserTest,
Subresource_NetworkRequestRepliedFirstButFetchHandlerResultIsUsed) {
SetupAndRegisterServiceWorker();
@@ -6429,7 +6421,7 @@ IN_PROC_BROWSER_TEST_P(
EXPECT_EQ(1, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
Subresource_PassThrough) {
SetupAndRegisterServiceWorker();
ReloadBlockUntilNavigationsComplete(shell(), 1);
@@ -6450,7 +6442,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
EXPECT_EQ(1, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
Subresource_PassThrough_LargeData) {
SetupAndRegisterServiceWorker();
ReloadBlockUntilNavigationsComplete(shell(), 1);
@@ -6470,7 +6462,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
EXPECT_EQ(1, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
Subresource_NetworkRequest_Wins_FetchHandler_Fallback) {
SetupAndRegisterServiceWorker();
ReloadBlockUntilNavigationsComplete(shell(), 1);
@@ -6488,7 +6480,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
EXPECT_EQ(1, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadBrowserTest,
Subresource_FetchHandler_Wins_Fallback) {
SetupAndRegisterServiceWorker();
ReloadBlockUntilNavigationsComplete(shell(), 1);
@@ -6513,8 +6505,9 @@ class ServiceWorkerAutoPreloadWithBlockedHostsBrowserTest
feature_list_.InitWithFeaturesAndParameters(
{
{features::kServiceWorkerAutoPreload,
{{"blocked_hosts", blocked_host()},
{"use_two_phase_write", GetParam() ? "true" : "false"}}},
{
{"blocked_hosts", blocked_host()},
}},
},
{});
}
@@ -6535,11 +6528,7 @@ class ServiceWorkerAutoPreloadWithBlockedHostsBrowserTest
base::test::ScopedFeatureList feature_list_;
};
INSTANTIATE_TEST_SUITE_P(ALL,
ServiceWorkerAutoPreloadWithBlockedHostsBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadWithBlockedHostsBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadWithBlockedHostsBrowserTest,
BlockedHosts) {
// Register the ServiceWorker and navigate to the in scope URL.
SetupAndRegisterServiceWorker();
@@ -6566,8 +6555,9 @@ class ServiceWorkerAutoPreloadWithEnableSubresourcePreloadBrowserTest
ServiceWorkerAutoPreloadWithEnableSubresourcePreloadBrowserTest() {
feature_list_.InitWithFeaturesAndParameters(
{{features::kServiceWorkerAutoPreload,
{{"enable_subresource_preload", "false"},
{"use_two_phase_write", GetParam() ? "true" : "false"}}}},
{
{"enable_subresource_preload", "false"},
}}},
{});
RaceNetworkRequestWriteBufferManager::SetDataPipeCapacityBytesForTesting(
1024);
@@ -6577,12 +6567,7 @@ class ServiceWorkerAutoPreloadWithEnableSubresourcePreloadBrowserTest
base::test::ScopedFeatureList feature_list_;
};
INSTANTIATE_TEST_SUITE_P(
ALL,
ServiceWorkerAutoPreloadWithEnableSubresourcePreloadBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(
IN_PROC_BROWSER_TEST_F(
ServiceWorkerAutoPreloadWithEnableSubresourcePreloadBrowserTest,
Disabled) {
SetupAndRegisterServiceWorker();
@@ -6627,8 +6612,9 @@ class ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest
ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest() {
feature_list_.InitWithFeaturesAndParameters(
{{features::kServiceWorkerAutoPreload,
{{"enable_only_when_service_worker_not_running", "true"},
{"use_two_phase_write", GetParam() ? "true" : "false"}}}},
{
{"enable_only_when_service_worker_not_running", "true"},
}}},
{});
RaceNetworkRequestWriteBufferManager::SetDataPipeCapacityBytesForTesting(
1024);
@@ -6638,12 +6624,7 @@ class ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest
base::test::ScopedFeatureList feature_list_;
};
INSTANTIATE_TEST_SUITE_P(
ALL,
ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(
IN_PROC_BROWSER_TEST_F(
ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest,
NotRunning) {
// Ensure the ServiceWorker is stopped.
@@ -6670,7 +6651,7 @@ IN_PROC_BROWSER_TEST_P(
observer.GetNormalizedResponseHeader("X-Response-From"));
}
IN_PROC_BROWSER_TEST_P(
IN_PROC_BROWSER_TEST_F(
ServiceWorkerAutoPreloadWithEnableOnlyWhenSWNotRunningBrowserTest,
Running) {
// Ensure the ServiceWorker is running.
@@ -6696,12 +6677,12 @@ IN_PROC_BROWSER_TEST_P(
}
class ServiceWorkerAutoPreloadAllowListBrowserTest
: public ServiceWorkerAutoPreloadBrowserTest {
: public ServiceWorkerAutoPreloadBrowserTest,
public testing::WithParamInterface<bool> {
public:
ServiceWorkerAutoPreloadAllowListBrowserTest() {
feature_list_.InitWithFeaturesAndParameters(
{{features::kServiceWorkerAutoPreload,
{{"use_allowlist", "true"}, {"use_two_phase_write", "false"}}},
{{features::kServiceWorkerAutoPreload, {{"use_allowlist", "true"}}},
{features::kServiceWorkerBypassFetchHandlerHashStrings,
{{"script_checksum_to_bypass",
ShouldUseValidChecksum() ? kValidChecksum : kInvalidChecksum}}}},
@@ -6767,11 +6748,7 @@ class ServiceWorkerAutoPreloadOptOutBrowserTest
base::test::ScopedFeatureList feature_list_;
};
INSTANTIATE_TEST_SUITE_P(ALL,
ServiceWorkerAutoPreloadOptOutBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadOptOutBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadOptOutBrowserTest,
MainResourceFetchHandlerShouldNotRace) {
SetupAndRegisterServiceWorker();
const std::string relative_url = "/service_worker/no_race?sw_slow&sw_respond";
@@ -6798,7 +6775,7 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadOptOutBrowserTest,
EXPECT_EQ(0, GetRequestCount(relative_url));
}
IN_PROC_BROWSER_TEST_P(ServiceWorkerAutoPreloadOptOutBrowserTest,
IN_PROC_BROWSER_TEST_F(ServiceWorkerAutoPreloadOptOutBrowserTest,
SubresourceFetchHandlerShouldNotRace) {
SetupAndRegisterServiceWorker();
WorkerRunningStatusObserver observer(public_context());

@@ -4,7 +4,6 @@
#include "content/common/service_worker/race_network_request_url_loader_client.h"
#include "base/debug/crash_logging.h"
#include "base/feature_list.h"
#include "base/metrics/field_trial_params.h"
#include "base/metrics/histogram_functions.h"
@@ -385,10 +384,7 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::OnDataTransferComplete() {
void ServiceWorkerRaceNetworkRequestURLLoaderClient::WatchDataUpdate() {
auto write_callback =
base::GetFieldTrialParamByFeatureAsBool(
features::kServiceWorkerAutoPreload, "use_two_phase_write", true)
? &ServiceWorkerRaceNetworkRequestURLLoaderClient::TwoPhaseWrite
: &ServiceWorkerRaceNetworkRequestURLLoaderClient::Write;
&ServiceWorkerRaceNetworkRequestURLLoaderClient::TwoPhaseWrite;
CHECK(read_buffer_manager_.has_value());
read_buffer_manager_->Watch(
base::BindRepeating(&ServiceWorkerRaceNetworkRequestURLLoaderClient::Read,
@@ -403,8 +399,6 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::WatchDataUpdate() {
void ServiceWorkerRaceNetworkRequestURLLoaderClient::Read(
MojoResult result,
const mojo::HandleSignalsState& state) {
SCOPED_CRASH_KEY_BOOL("SWRace", "state_readable", state.readable());
SCOPED_CRASH_KEY_BOOL("SWRace", "state_peer_closed", state.peer_closed());
if (!IsReadyToHandleReadWrite(result)) {
return;
}
@@ -416,7 +410,6 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::Read(
return;
}
SCOPED_CRASH_KEY_STRING256("SWRace", "request_url", request_.url.spec());
auto [read_result, read_buffer] = read_buffer_manager_->ReadData();
TRACE_EVENT_WITH_FLOW2("ServiceWorker",
"ServiceWorkerRaceNetworkRequestURLLoaderClient::Read",
@@ -436,7 +429,6 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::Read(
case MOJO_RESULT_SHOULD_WAIT:
return;
default:
SCOPED_CRASH_KEY_NUMBER("SWRace", "read_result", read_result);
NOTREACHED_IN_MIGRATION() << "ReadData result:" << read_result;
return;
}
@@ -561,107 +553,6 @@ void ServiceWorkerRaceNetworkRequestURLLoaderClient::TwoPhaseWrite(
CompleteReadData(num_bytes_to_consume);
}
void ServiceWorkerRaceNetworkRequestURLLoaderClient::Write(
MojoResult result,
const mojo::HandleSignalsState& state) {
if (!IsReadyToHandleReadWrite(result)) {
return;
}
CHECK(read_buffer_manager_.has_value());
if (read_buffer_manager_->BytesRemaining() == 0) {
read_buffer_manager_->ArmOrNotify();
return;
}
base::span<const char> read_buffer = read_buffer_manager_->RemainingBuffer();
size_t num_bytes_to_consume = read_buffer.size();
if (write_buffer_manager_for_race_network_request_.IsWatching() &&
write_buffer_manager_for_fetch_handler_.IsWatching()) {
// If both data pipes are watched, write data to both pipes.
std::pair<size_t, size_t> written_bytes;
std::tie(result, written_bytes.first) =
write_buffer_manager_for_race_network_request_.WriteData(read_buffer);
RecordMojoResultForWrite(result);
switch (result) {
case MOJO_RESULT_OK:
break;
case MOJO_RESULT_FAILED_PRECONDITION:
// The data pipe consumer is aborted.
TransitionState(State::kAborted);
Abort();
return;
case MOJO_RESULT_SHOULD_WAIT:
case MOJO_RESULT_OUT_OF_RANGE:
// The data pipe is not writable yet. We don't consume data from |body_|
// and write any data in this case. And retry it later.
write_buffer_manager_for_race_network_request_.ArmOrNotify();
return;
}
std::tie(result, written_bytes.second) =
write_buffer_manager_for_fetch_handler_.WriteData(read_buffer);
RecordMojoResultForWrite(result);
switch (result) {
case MOJO_RESULT_OK:
break;
case MOJO_RESULT_FAILED_PRECONDITION:
TransitionState(State::kAborted);
Abort();
return;
case MOJO_RESULT_SHOULD_WAIT:
case MOJO_RESULT_OUT_OF_RANGE:
// When the data pipe returns MOJO_RESULT_SHOULD_WAIT, the data pipe is
// not consumed yet but the buffer is full. Stop processing the data
// pipe for the fetch handler side, not to make the data transfer
// process for the race network request side being stuck.
write_buffer_manager_for_fetch_handler_.CancelWatching();
write_buffer_manager_for_race_network_request_.ArmOrNotify();
return;
}
CHECK_EQ(written_bytes.first, written_bytes.second);
num_bytes_to_consume = written_bytes.first;
CHECK_EQ(write_buffer_manager_for_race_network_request_.num_bytes_written(),
write_buffer_manager_for_fetch_handler_.num_bytes_written());
} else if (write_buffer_manager_for_race_network_request_.IsWatching()) {
// If the data pipe for RaceNetworkRequest is the only watcher, don't write
// data to the data pipe for the fetch handler.
std::tie(result, num_bytes_to_consume) =
write_buffer_manager_for_race_network_request_.WriteData(read_buffer);
RecordMojoResultForWrite(result);
switch (result) {
case MOJO_RESULT_OK:
break;
case MOJO_RESULT_FAILED_PRECONDITION:
TransitionState(State::kAborted);
Abort();
return;
case MOJO_RESULT_SHOULD_WAIT:
case MOJO_RESULT_OUT_OF_RANGE:
write_buffer_manager_for_race_network_request_.ArmOrNotify();
return;
}
} else if (write_buffer_manager_for_fetch_handler_.IsWatching()) {
// If the data pipe for the fetch handler is the only watcher, don't write
// data to the data pipe for RaceNetworkRequest.
std::tie(result, num_bytes_to_consume) =
write_buffer_manager_for_fetch_handler_.WriteData(read_buffer);
RecordMojoResultForWrite(result);
switch (result) {
case MOJO_RESULT_OK:
break;
case MOJO_RESULT_FAILED_PRECONDITION:
TransitionState(State::kAborted);
Abort();
return;
case MOJO_RESULT_SHOULD_WAIT:
case MOJO_RESULT_OUT_OF_RANGE:
write_buffer_manager_for_fetch_handler_.ArmOrNotify();
return;
}
}
CompleteReadData(num_bytes_to_consume);
}
bool ServiceWorkerRaceNetworkRequestURLLoaderClient::IsReadyToHandleReadWrite(
MojoResult result) {
if (!owner_) {

@@ -202,17 +202,6 @@ class CONTENT_EXPORT ServiceWorkerRaceNetworkRequestURLLoaderClient
// due to the long fetch handler execution. and test case the mechanism to
// wait for the fetch handler
void TwoPhaseWrite(MojoResult result, const mojo::HandleSignalsState& state);
// Writes data in RaceNetworkRequestReadBufferManager into the data
// pipe producer that handles for both the race network request and the fetch
// handler respectively.
//
// Unlike |TwoPhaseWrite()|, this doesn't use two-phase operations to
// write data into data pipes. However, the result should be the same as
// |TwoPhaseWrite()| because mojo's |WriteData()| is expected to write
// the same amount of data from the given data pipe consumer handle to read.
// also |Write()| has CHECK to guarantee that the actual written sizes
// to data pips are exactly same.
void Write(MojoResult result, const mojo::HandleSignalsState& state);
bool IsReadyToHandleReadWrite(MojoResult result);