Keep browser alive while fetch keepalive URLLoaders exist [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hi @my...@chromium.org i think this solves it, verified it on on all platforms, could you PTAL and re-route if its better fit for other reviewers.
and please let me know if you want me to change anything.
for easier testing here is a sampler with a demo server: https://static.januschka.com/i-408010432/index.html if you want to take a look
Related details
- Ming-Ying Chung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
void ChromeContentBrowserClient::OnFetchKeepAliveRequestCreated(The overall idea looks okay. But I roughly remember one of the goal of the fetch keepalive in-browser migration is to get rid of the usage of `ScopedKeepAlive` (or all its callbacks `OnKeepaliveRequest*` above). Unfortunately the code is not cleaned up after it's launched.
Given that new callbacks to increase `num_fetch_keepalive_loaders_` are added, I think we want to make sure that they don't really depend on all other existing callbacks, i.e. we only want to keep the requests alive in extreme case (browser shutdown), not due to fetch keepalive legacy path.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
void ChromeContentBrowserClient::OnFetchKeepAliveRequestCreated(The overall idea looks okay. But I roughly remember one of the goal of the fetch keepalive in-browser migration is to get rid of the usage of `ScopedKeepAlive` (or all its callbacks `OnKeepaliveRequest*` above). Unfortunately the code is not cleaned up after it's launched.
Given that new callbacks to increase `num_fetch_keepalive_loaders_` are added, I think we want to make sure that they don't really depend on all other existing callbacks, i.e. we only want to keep the requests alive in extreme case (browser shutdown), not due to fetch keepalive legacy path.
thanks for the insights! the new `num_fetch_keepalive_loaders_` path is fully decoupled from the legacy `num_keepalive_requests_`/`keepalive_timer_` one.
for the legacy cleanup i can file a tracking bug, WDYT?
Related details
- Ming-Ying Chung
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
ping, please let me know if i should re-assign to someone else, or if you want me to address anything
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Code-Review | +1 |
5 comments
ping, please let me know if i should re-assign to someone else, or if you want me to address anything
LGTM but you need owner review.
void ChromeContentBrowserClient::OnFetchKeepAliveRequestCreated(Helmut JanuschkaThe overall idea looks okay. But I roughly remember one of the goal of the fetch keepalive in-browser migration is to get rid of the usage of `ScopedKeepAlive` (or all its callbacks `OnKeepaliveRequest*` above). Unfortunately the code is not cleaned up after it's launched.
Given that new callbacks to increase `num_fetch_keepalive_loaders_` are added, I think we want to make sure that they don't really depend on all other existing callbacks, i.e. we only want to keep the requests alive in extreme case (browser shutdown), not due to fetch keepalive legacy path.
thanks for the insights! the new `num_fetch_keepalive_loaders_` path is fully decoupled from the legacy `num_keepalive_requests_`/`keepalive_timer_` one.
for the legacy cleanup i can file a tracking bug, WDYT?
sgtm.
void ChromeContentBrowserClient::OnFetchKeepAliveRequestCreated(Better to have commentes (either here or in the header) explaining why we re-introduce the scoped keepalive usage even with the in-browser migration.
fetch_keepalive_profile_keep_alive_ = ScopedProfileKeepAlive::TryAcquire(
profile, ProfileKeepAliveOrigin::kFetchKeepAlive);How if multiple requests from different profiles exist? It looks like only the last profile will be held.
// Tests for https://crbug.com/408010432.We should have proper test description in addition to links to bug.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
void ChromeContentBrowserClient::OnFetchKeepAliveRequestCreated(Helmut JanuschkaBetter to have commentes (either here or in the header) explaining why we re-introduce the scoped keepalive usage even with the in-browser migration.
Done
fetch_keepalive_profile_keep_alive_ = ScopedProfileKeepAlive::TryAcquire(
profile, ProfileKeepAliveOrigin::kFetchKeepAlive);How if multiple requests from different profiles exist? It looks like only the last profile will be held.
gosh, good catch, now tracking loaders per BrowserContext: OnFetchKeepAliveRequestDestroyed() receives the BrowserContext too, and a ScopedProfileKeepAlive is held for every profile with in-flight loaders (acquired on that profile's first loader, released on its last). The pointer at destroy time may belong to a context mid-destruction, so it is only used as a lookup key and never dereferenced. Added a multi-profile browser test (HoldsEveryProfileWithInFlightLoaders) covering this.
We should have proper test description in addition to links to bug.
- Nicolas Ouellet-Payeur
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |