Add UKM event for Service Worker main resource fetch handler errors [chromium/src : main]
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Commit-Queue | +1 |
2 comments
PTAL.
I have updated the code to record the fetch() API failure in the fetch handler.
Recorded when a Service Worker fetch handler fails to load the main resource
due to a network error.Yoshisato YanagisawaThis sounds that we're collecting metrics when the fetch handler itself fails to load. But what we want to collect is "failed to fetch" error details in `fetch()` inside the fetch handler.
Acknowledged
Related details
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
// If the race-network-and-fetch fetch(event.request) call inside the Service
// Worker fetch handler failed with a network error, this contains net::ERR_...
int32? service_worker_race_fetch_error_code;
// If a regular (non-race) fetch() call inside the Service Worker fetch handler
// failed with a network error, this field contains the net::ERR_... code.
int32? service_worker_regular_fetch_error_code;
I wonder if we could consider adding these error codes to something else in ServiceWorker mojom struct as it's too much SW details and don't want to pollute `Response` struct.
For example:
1. Add a new optional param `ServiceWorkerFetchHandlerMetrics` to `OnResponse`, `OnResponseStream`, and `OnFallback`.
2. Or, simpler: append the optional error codes directly into the existing `ServiceWorkerFetchEventTiming` struct, which is already used to carry telemetry data to the browser process.
This would keep `FetchAPIResponse` fully cleanly decoupled from internal SW metrics, WDYT?
if (active_fetch_respond_with_observers_.size() == 1) {
active_fetch_respond_with_observers_.begin()->value->OnRegularFetchError(
net_error_code);
return;
}
if (request_data) {
for (const auto& entry : active_fetch_respond_with_observers_) {
if (entry.value->request_url() == request_data->Url()) {
entry.value->OnRegularFetchError(net_error_code);
return;
}
}
}I'm concerned that relying on `size() == 1` and URL matching here introduces race conditions resulting in non-deterministic telemetry.
There are two major edge cases:
1. **False Negatives on Fallback**: If a handler does `catch(() => fetch('/offline.html'))` and it fails, `request_data->Url()` won't match `request_url()`. If `size() > 1` (concurrent navigations), the error is discarded. But if `size() == 1`, it's recorded. The same code behaves differently under load.
2. **False Positives on Background Fetches**: If a completely unrelated background fetch (e.g. from `setInterval` or a `message` event) fails, and there happens to be exactly 1 active `FetchEvent`, this will falsely attribute that unrelated error to the main resource navigation.
If threading a context/event ID into `FetchRequestData` is too complex for this CL, I'd suggest removing the `size() == 1` fallback completely and only relying on strict URL matching. While we'd intentionally miss fallback fetch errors, the behavior would at least be deterministic and free from false-positive background errors. WDYT?
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
// If the race-network-and-fetch fetch(event.request) call inside the Service
// Worker fetch handler failed with a network error, this contains net::ERR_...
int32? service_worker_race_fetch_error_code;
// If a regular (non-race) fetch() call inside the Service Worker fetch handler
// failed with a network error, this field contains the net::ERR_... code.
int32? service_worker_regular_fetch_error_code;
I wonder if we could consider adding these error codes to something else in ServiceWorker mojom struct as it's too much SW details and don't want to pollute `Response` struct.
For example:
1. Add a new optional param `ServiceWorkerFetchHandlerMetrics` to `OnResponse`, `OnResponseStream`, and `OnFallback`.
2. Or, simpler: append the optional error codes directly into the existing `ServiceWorkerFetchEventTiming` struct, which is already used to carry telemetry data to the browser process.This would keep `FetchAPIResponse` fully cleanly decoupled from internal SW metrics, WDYT?
tl;dr Option 1 was chosen.
I understand your concern on piggyback FetchAPIResponse. I left the existing structures as-is because they are not made for carrying the error status. Let me create a new one as suggested in Option 1 instead.
if (active_fetch_respond_with_observers_.size() == 1) {
active_fetch_respond_with_observers_.begin()->value->OnRegularFetchError(
net_error_code);
return;
}
if (request_data) {
for (const auto& entry : active_fetch_respond_with_observers_) {
if (entry.value->request_url() == request_data->Url()) {
entry.value->OnRegularFetchError(net_error_code);
return;
}
}
}I'm concerned that relying on `size() == 1` and URL matching here introduces race conditions resulting in non-deterministic telemetry.
There are two major edge cases:
1. **False Negatives on Fallback**: If a handler does `catch(() => fetch('/offline.html'))` and it fails, `request_data->Url()` won't match `request_url()`. If `size() > 1` (concurrent navigations), the error is discarded. But if `size() == 1`, it's recorded. The same code behaves differently under load.
2. **False Positives on Background Fetches**: If a completely unrelated background fetch (e.g. from `setInterval` or a `message` event) fails, and there happens to be exactly 1 active `FetchEvent`, this will falsely attribute that unrelated error to the main resource navigation.If threading a context/event ID into `FetchRequestData` is too complex for this CL, I'd suggest removing the `size() == 1` fallback completely and only relying on strict URL matching. While we'd intentionally miss fallback fetch errors, the behavior would at least be deterministic and free from false-positive background errors. WDYT?
tl;dr removed size() == 1 case.
This aims to track fetch handler failure as much as possible, but at the same time, it brings uncertainty if the fetch() is actually called from the expected fetch handler as you pointed out. Let me remove the size() == 1 to avoid that.
Related details
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted Code-Review+1![]( "Open in Gerrit")
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Reviewed UKM doc is here: https://docs.google.com/document/d/1D7j9uR2oG1nxxSWsG8QmNmc6iR1j_rMOQCC4v1MDWzY/edit?tab=t.0
Will you review this?
Related details
- Chromium IPC Reviews
- Robert Kaplow
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: sa...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): sa...@chromium.org
Reviewer source(s):
sa...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Robert Kaplow
- Sam McNally
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam McNally (Gerrit)
Sam McNally voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sam McNally
| Code-Review | +1 |
2 comments
// This function may be called when an exception is scheduled. Thus, it mustShould this comment stay with `OnResponseRejected()`?
}Please fix this WARNING reported by autoreview issue finding: If `request_data` has a race token but it doesn't match any `active_fetch_respond_with_observers_` (e.g. because it was already destroyed or wasn't tracked), this code will fall through to the second `if (request_data)` block. Since it's a race fetch, `request_data->Url()` will be the main resource URL, so it might accidentally match a regular fetch observer and incorrectly log the error as a `regular_fetch_error_code`.
Consider returning early to prevent this fallback:
```cpp
if (request_data && request_data->ServiceWorkerRaceNetworkRequestToken()) {
base::UnguessableToken token =
request_data->ServiceWorkerRaceNetworkRequestToken();
for (const auto& entry : active_fetch_respond_with_observers_) {
if (entry.value->race_network_request_token() &&
*entry.value->race_network_request_token() == token) {
entry.value->OnRaceFetchError(net_error_code);
return;
}
}
return; // <-- Do not fall through to the regular fetch URL check
}
```
Related details
- Robert Kaplow
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
// This function may be called when an exception is scheduled. Thus, it mustShould this comment stay with `OnResponseRejected()`?
Good catch!
Done.
Please fix this WARNING reported by autoreview issue finding: If `request_data` has a race token but it doesn't match any `active_fetch_respond_with_observers_` (e.g. because it was already destroyed or wasn't tracked), this code will fall through to the second `if (request_data)` block. Since it's a race fetch, `request_data->Url()` will be the main resource URL, so it might accidentally match a regular fetch observer and incorrectly log the error as a `regular_fetch_error_code`.
Consider returning early to prevent this fallback:
```cpp
if (request_data && request_data->ServiceWorkerRaceNetworkRequestToken()) {
base::UnguessableToken token =
request_data->ServiceWorkerRaceNetworkRequestToken();
for (const auto& entry : active_fetch_respond_with_observers_) {
if (entry.value->race_network_request_token() &&
*entry.value->race_network_request_token() == token) {
entry.value->OnRaceFetchError(net_error_code);
return;
}
}
return; // <-- Do not fall through to the regular fetch URL check
}
```
Good catch! Done.
I assume the case can be another way "failed to fetch" happens, but I am not so confident.
Let me leave TODO for that, while applying your suggestion.
Related details
- Robert Kaplow
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |