Evict BFCached pages with SharedWorker when the last client navigates away [chromium/src : main]
Anna Sato (Gerrit)
Anna Sato voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anna Sato
| Commit-Queue | +1 |
1 comment
This CL evicts clients with `kSharedWorker` reason when the worker has no active clients, and also adds/modifies some WPTs. PTAL?
Related details
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 5 comments![]( "Open in Gerrit")
bool EvictBFCachedClientsIfLastActive(Please add a comment describing this method.
if (info.render_frame_host_id == client_rfh_id) {
continue;
}probably don't need this since it will no have `IsInBackForwardCache()`.
bool EvictBFCachedClientsIfLastActive(Please add a comment describing this method.
<!doctype html>Is there any reason for this to be a html test instead of a JS test? If no, please make this and any other new tests JS tests.
<int value="72" label="kSharedWorkerMessage"/>why change this? These are more textual descriptions.
Related details
- Anna Sato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 6 comments![]( "Open in Gerrit")
Thanks fergal for the review! Added yanagisawa-san for shared worker part, PTAL?
Please add a comment describing this method.
Added a comment.
if (info.render_frame_host_id == client_rfh_id) {
continue;
}probably don't need this since it will no have `IsInBackForwardCache()`.
This `continue` is needed to avoid the `else` below. Without it, the function would check the client against itself and return `false` saying there are active clients.
Please add a comment describing this method.
Added a comment.
Is there any reason for this to be a html test instead of a JS test? If no, please make this and any other new tests JS tests.
Made new tests JS.
why change this? These are more textual descriptions.
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 2 comments![]( "Open in Gerrit")
if (info.render_frame_host_id == client_rfh_id) {
continue;
}Anna Satoprobably don't need this since it will no have `IsInBackForwardCache()`.
This `continue` is needed to avoid the `else` below. Without it, the function would check the client against itself and return `false` saying there are active clients.
Sorry, I thought I had deleted that comment after figuring that out myself :)
await assertBFCacheEligibility(rc2, /*shouldRestoreFromBfcache=*/ false);Should this also be doing `assertNotRestoredFromBFCache` with a reason?
Related details
- Anna Sato
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 2 comments![]( "Open in Gerrit")
await assertBFCacheEligibility(rc2, /*shouldRestoreFromBfcache=*/ false);Should this also be doing `assertNotRestoredFromBFCache` with a reason?
That's right. Modified a bit and updated to use `assertNotRestoredFromBFCache` for both pages.
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 6 comments![]( "Open in Gerrit")
Will you leave comments on the design? I feel the implementation is not trivial.
clients are in BFCache. When the last active client of a SharedWorkerThis is for solving the "Worker state when last client enters BFCache." case explained in https://docs.google.com/document/d/1dZxwmUYOlDmZ-PF97mfoXbzMrFHBsqeUT3XxTkpNIb0/edit?tab=t.0, right?
Bug: 406420935 return false;Will you leave a comment for this?
Is this mean active client (i.e. non BFCache'd client) exist?
// the last active client.I guess we lack an explanation that:
We assume that this render frame host (i.e. document) can be a client of multiple SharedWorkers. We would like to list all SharedWorkerHosts and proceed `EvictBFCacheClientsIfLastActive()` to all of them.
for (const auto& entry : shared_worker_client_counts_) {Let me confirm what I have understand from here.
Technically, you can use two approaches:
1. loop with `worker_hosts_` or `shared_worker_hosts_`, and make it ensure if the client is belongs to it. Proceed SharedWorkerHost's `EvictBFCachedClientsIfLastActive()` for that case.
2. loop with `shared_worker_client_counts_`, and select the SharedWorkerHost that has the given client, then run SharedWorkerHost's `EvictBFCachedClientsIfLastActive()`.
I understand you chose the second. Will you leave some comments on why you chose the second, and how it works?
Leaving a mini-design doc is welcomed as well.
Related details
- Anna Sato
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 2 comments![]( "Open in Gerrit")
Thanks for the review! I have a quick question for yanagisawa-san.
for (const auto& entry : shared_worker_client_counts_) {Let me confirm what I have understand from here.
Technically, you can use two approaches:
1. loop with `worker_hosts_` or `shared_worker_hosts_`, and make it ensure if the client is belongs to it. Proceed SharedWorkerHost's `EvictBFCachedClientsIfLastActive()` for that case.
2. loop with `shared_worker_client_counts_`, and select the SharedWorkerHost that has the given client, then run SharedWorkerHost's `EvictBFCachedClientsIfLastActive()`.I understand you chose the second. Will you leave some comments on why you chose the second, and how it works?
Leaving a mini-design doc is welcomed as well.
Thanks for summarizing the approaches, and yes I'm using 2.
1 is better when a client is connected to most of the active workers, while 2 is better when it's connected to only a few, IIUC. I've put the details in this doc:
https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?usp=sharing&resourcekey=0-K6Q2fbF8lAL2QYot2dE73Q
Do you have any thoughts on which approach is better here?
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
for (const auto& entry : shared_worker_client_counts_) {Anna SatoLet me confirm what I have understand from here.
Technically, you can use two approaches:
1. loop with `worker_hosts_` or `shared_worker_hosts_`, and make it ensure if the client is belongs to it. Proceed SharedWorkerHost's `EvictBFCachedClientsIfLastActive()` for that case.
2. loop with `shared_worker_client_counts_`, and select the SharedWorkerHost that has the given client, then run SharedWorkerHost's `EvictBFCachedClientsIfLastActive()`.I understand you chose the second. Will you leave some comments on why you chose the second, and how it works?
Leaving a mini-design doc is welcomed as well.
Thanks for summarizing the approaches, and yes I'm using 2.
1 is better when a client is connected to most of the active workers, while 2 is better when it's connected to only a few, IIUC. I've put the details in this doc:
https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?usp=sharing&resourcekey=0-K6Q2fbF8lAL2QYot2dE73QDo you have any thoughts on which approach is better here?
I feel Option 1 is straight forward because:
a) `worker_hosts_` owns the SharedWorkerHost itself (i.e. no lookup is needed).
b) we can directly write a logic other inside the SharedWorkerHost code,
c) and all necessary information are available in the SharedWorkerHost's field (as you are already using).
I am not sure which case usually happen without seeing a metric.
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');Will you add three more test scenarios?
1. a frame actually has multiple SharedWorker clients with the same SharedWorker instance. (inspired by https://chromium-review.googlesource.com/c/chromium/src/+/1872632, saying a frame can
be added as a client multiple times to the same worker)
2. a frame has multiple SharedWorker clients with different instances.
3. two frames with shared and non-shared SharedWorker. non-shared SharedWorker is destroyed while shared is not. Frame A has SharedWorker a and SharedWorker c. Frame B has SharedWorker b and SharedWorker c. After Frame A unload, Frame A should be evicted on Frame B unload. My concern here is that wrong early return implementation may overlook SharedWorker c here.
Related details
- Anna Sato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato voted and added 8 comments![]( "Open in Gerrit")
Votes added by Anna Sato
| Commit-Queue | +1 |
8 comments
clients are in BFCache. When the last active client of a SharedWorkerThis is for solving the "Worker state when last client enters BFCache." case explained in https://docs.google.com/document/d/1dZxwmUYOlDmZ-PF97mfoXbzMrFHBsqeUT3XxTkpNIb0/edit?tab=t.0, right?
Yes. Updated the desc.
Bug: 406420935I guess it nice also link to https://crbug.com/431875857
Added the bug, on the parent CL as well, thanks!
return false;Will you leave a comment for this?
Is this mean active client (i.e. non BFCache'd client) exist?
Yes. Added a comment here.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Addressed the case where a main frame and its iframe are connected to the same worker, which @fer...@chromium.org also pointed out in [doc](https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?resourcekey=0-K6Q2fbF8lAL2QYot2dE73Q&disco=AAABrySJO4w), by adding a check its main frame to ensure they aren't considered as independent active clients.
Added a test for this senario in `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-iframe-client.tentative.window.js.`
I guess we lack an explanation that:
We assume that this render frame host (i.e. document) can be a client of multiple SharedWorkers. We would like to list all SharedWorkerHosts and proceed `EvictBFCacheClientsIfLastActive()` to all of them.
Yeah right. Added "For all connected workers with `render_frame_host_id`," at the beginning.
for (const auto& entry : shared_worker_client_counts_) {Anna SatoLet me confirm what I have understand from here.
Technically, you can use two approaches:
1. loop with `worker_hosts_` or `shared_worker_hosts_`, and make it ensure if the client is belongs to it. Proceed SharedWorkerHost's `EvictBFCachedClientsIfLastActive()` for that case.
2. loop with `shared_worker_client_counts_`, and select the SharedWorkerHost that has the given client, then run SharedWorkerHost's `EvictBFCachedClientsIfLastActive()`.I understand you chose the second. Will you leave some comments on why you chose the second, and how it works?
Leaving a mini-design doc is welcomed as well.
Yoshisato YanagisawaThanks for summarizing the approaches, and yes I'm using 2.
1 is better when a client is connected to most of the active workers, while 2 is better when it's connected to only a few, IIUC. I've put the details in this doc:
https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?usp=sharing&resourcekey=0-K6Q2fbF8lAL2QYot2dE73QDo you have any thoughts on which approach is better here?
I feel Option 1 is straight forward because:
a) `worker_hosts_` owns the SharedWorkerHost itself (i.e. no lookup is needed).
b) we can directly write a logic other inside the SharedWorkerHost code,
c) and all necessary information are available in the SharedWorkerHost's field (as you are already using).I am not sure which case usually happen without seeing a metric.
Thank you! I now changed to use Option 1 and created a new `ContainsClient()` to avoid checking host that don't have a connection to the client.
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');Will you add three more test scenarios?
1. a frame actually has multiple SharedWorker clients with the same SharedWorker instance. (inspired by https://chromium-review.googlesource.com/c/chromium/src/+/1872632, saying a frame can
be added as a client multiple times to the same worker)
2. a frame has multiple SharedWorker clients with different instances.
3. two frames with shared and non-shared SharedWorker. non-shared SharedWorker is destroyed while shared is not. Frame A has SharedWorker a and SharedWorker c. Frame B has SharedWorker b and SharedWorker c. After Frame A unload, Frame A should be evicted on Frame B unload. My concern here is that wrong early return implementation may overlook SharedWorker c here.
I've added new WPTs for:
1. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-same-instance.tentative.window.js
2. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-different-instances.tentative.window.js
As for case 3, I guess when Frame A unloads, it should be blocked from BFCache immediately, since it's the only client for `worker a`? Please let me know if I've misunderstood, or if you had a different scenario in mind.
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 6 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
6 comments
lgtm w/ comments.
bool SharedWorkerServiceImpl::EvictBFCachedClientsIfLastActive(You might be focus on correctness compared to keeping clients as much as possible, right?
I am considering the scenario like:
Frame 1 has SharedWorker A and SharedWorker B.
Frame 2 has SharedWorker A and SharedWorker B.
Frame 3 only has SharedWorker B.
When Frame 1 got unloaded, Frame 1 will be BFCached.
Then, when Frame 2 got unloaded, Frame 1 will be evicted because Frame 2 is the last client for SharedWorker A.
Technically, Frame 3 and SharedWorker B alive at this time, and Frame 1-2 can technically be BFCached until Frame 3 unload. However, the current implementation should still be correct.
for (const auto& entry : shared_worker_client_counts_) {Anna SatoLet me confirm what I have understand from here.
Technically, you can use two approaches:
1. loop with `worker_hosts_` or `shared_worker_hosts_`, and make it ensure if the client is belongs to it. Proceed SharedWorkerHost's `EvictBFCachedClientsIfLastActive()` for that case.
2. loop with `shared_worker_client_counts_`, and select the SharedWorkerHost that has the given client, then run SharedWorkerHost's `EvictBFCachedClientsIfLastActive()`.I understand you chose the second. Will you leave some comments on why you chose the second, and how it works?
Leaving a mini-design doc is welcomed as well.
Yoshisato YanagisawaThanks for summarizing the approaches, and yes I'm using 2.
1 is better when a client is connected to most of the active workers, while 2 is better when it's connected to only a few, IIUC. I've put the details in this doc:
https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?usp=sharing&resourcekey=0-K6Q2fbF8lAL2QYot2dE73QDo you have any thoughts on which approach is better here?
Anna SatoI feel Option 1 is straight forward because:
a) `worker_hosts_` owns the SharedWorkerHost itself (i.e. no lookup is needed).
b) we can directly write a logic other inside the SharedWorkerHost code,
c) and all necessary information are available in the SharedWorkerHost's field (as you are already using).I am not sure which case usually happen without seeing a metric.
Thank you! I now changed to use Option 1 and created a new `ContainsClient()` to avoid checking host that don't have a connection to the client.
Thank you!
for (const auto& host : worker_hosts_) {Can I ask you to add a trace and Uma to measure elapsed time here?
The code will be executed every time documents with SharedWorker unload, and I just want to know if we need to improve performance or fine to be as-is.
return true;Why is it fine to return here instead of running `EvictBFCachedClientsIfLastActive()` to all `worker_hosts_`?
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');Anna SatoWill you add three more test scenarios?
1. a frame actually has multiple SharedWorker clients with the same SharedWorker instance. (inspired by https://chromium-review.googlesource.com/c/chromium/src/+/1872632, saying a frame can
be added as a client multiple times to the same worker)
2. a frame has multiple SharedWorker clients with different instances.
3. two frames with shared and non-shared SharedWorker. non-shared SharedWorker is destroyed while shared is not. Frame A has SharedWorker a and SharedWorker c. Frame B has SharedWorker b and SharedWorker c. After Frame A unload, Frame A should be evicted on Frame B unload. My concern here is that wrong early return implementation may overlook SharedWorker c here.
I've added new WPTs for:
1. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-same-instance.tentative.window.js
2. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-different-instances.tentative.window.jsAs for case 3, I guess when Frame A unloads, it should be blocked from BFCache immediately, since it's the only client for `worker a`? Please let me know if I've misunderstood, or if you had a different scenario in mind.
Ah, good point. maybe Frame B should also have `worker a` to realize the scenario? i.e. Frame A has `Worker a` and `Worker b`, and Frame B has `worker a`, `worker b`, and `worker c`. Then, it clarifies my question in content/browser/worker_host/shared_worker_service_impl.cc
Related details
- Anna Sato
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 5 comments![]( "Open in Gerrit")
bool SharedWorkerServiceImpl::EvictBFCachedClientsIfLastActive(You might be focus on correctness compared to keeping clients as much as possible, right?
I am considering the scenario like:
Frame 1 has SharedWorker A and SharedWorker B.
Frame 2 has SharedWorker A and SharedWorker B.
Frame 3 only has SharedWorker B.When Frame 1 got unloaded, Frame 1 will be BFCached.
Then, when Frame 2 got unloaded, Frame 1 will be evicted because Frame 2 is the last client for SharedWorker A.
Technically, Frame 3 and SharedWorker B alive at this time, and Frame 1-2 can technically be BFCached until Frame 3 unload. However, the current implementation should still be correct.
Frames 1 and 2 could technically remain as worker B's client in BFCache while Frame 3 alive, but yeah I prefer the current design because of its simplicity, given that this is a temporary workaround until worker freezing is implemented.
Can I ask you to add a trace and Uma to measure elapsed time here?
The code will be executed every time documents with SharedWorker unload, and I just want to know if we need to improve performance or fine to be as-is.
Added `TRACE` and UMA to record the execution time.
Why is it fine to return here instead of running `EvictBFCachedClientsIfLastActive()` to all `worker_hosts_`?
Ah you're right, the early return was incorrect. Fixed to iterate through all workers.
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');Anna SatoWill you add three more test scenarios?
1. a frame actually has multiple SharedWorker clients with the same SharedWorker instance. (inspired by https://chromium-review.googlesource.com/c/chromium/src/+/1872632, saying a frame can
be added as a client multiple times to the same worker)
2. a frame has multiple SharedWorker clients with different instances.
3. two frames with shared and non-shared SharedWorker. non-shared SharedWorker is destroyed while shared is not. Frame A has SharedWorker a and SharedWorker c. Frame B has SharedWorker b and SharedWorker c. After Frame A unload, Frame A should be evicted on Frame B unload. My concern here is that wrong early return implementation may overlook SharedWorker c here.
Yoshisato YanagisawaI've added new WPTs for:
1. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-same-instance.tentative.window.js
2. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-different-instances.tentative.window.jsAs for case 3, I guess when Frame A unloads, it should be blocked from BFCache immediately, since it's the only client for `worker a`? Please let me know if I've misunderstood, or if you had a different scenario in mind.
Ah, good point. maybe Frame B should also have `worker a` to realize the scenario? i.e. Frame A has `Worker a` and `Worker b`, and Frame B has `worker a`, `worker b`, and `worker c`. Then, it clarifies my question in content/browser/worker_host/shared_worker_service_impl.cc
Added a new WPT at third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.tentative.window.js. (Sorry for the multiple updates...)
- Frame 1 connects to workers A and B.
- Frame 2 connects to workers A and C.
- Frame 3 connects to worker B.
The test verifies that when Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is correctly evicted. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
I am still not completely sure if I understood the exact scenario you had in mind, please let me know if this new test covers your concern!
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Message from Anna Sato![]( "Open in Gerrit")
- Anna Sato
- Fergal Daly
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 3 comments![]( "Open in Gerrit")
bool SharedWorkerServiceImpl::EvictBFCachedClientsIfLastActive(Anna SatoYou might be focus on correctness compared to keeping clients as much as possible, right?
I am considering the scenario like:
Frame 1 has SharedWorker A and SharedWorker B.
Frame 2 has SharedWorker A and SharedWorker B.
Frame 3 only has SharedWorker B.When Frame 1 got unloaded, Frame 1 will be BFCached.
Then, when Frame 2 got unloaded, Frame 1 will be evicted because Frame 2 is the last client for SharedWorker A.
Technically, Frame 3 and SharedWorker B alive at this time, and Frame 1-2 can technically be BFCached until Frame 3 unload. However, the current implementation should still be correct.
Frames 1 and 2 could technically remain as worker B's client in BFCache while Frame 3 alive, but yeah I prefer the current design because of its simplicity, given that this is a temporary workaround until worker freezing is implemented.
Acknowledged
UMA_HISTOGRAM_TIMES("SharedWorker.BFCache.LastClientCheckTime",
timer.Elapsed());Thank you for adding a trace and uma. Let me ask something more for uma.
1. if it is not in a critical path, a function is preferred to a macro (https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#coding-emitting-to-histograms)
2. please update histograms.xml (maybe tools/metrics/histograms/metadata/content/histograms.xml?) for this. Unfortunately, SharedWorker is not a top level metrics, I suggest to use the "Content.SharedWorker" prefix instead.
3. if you got stuck on writing a metric description, I suggest you to ask gemini to write it. Note that the description should be understandable without reading the code.
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');Anna SatoWill you add three more test scenarios?
1. a frame actually has multiple SharedWorker clients with the same SharedWorker instance. (inspired by https://chromium-review.googlesource.com/c/chromium/src/+/1872632, saying a frame can
be added as a client multiple times to the same worker)
2. a frame has multiple SharedWorker clients with different instances.
3. two frames with shared and non-shared SharedWorker. non-shared SharedWorker is destroyed while shared is not. Frame A has SharedWorker a and SharedWorker c. Frame B has SharedWorker b and SharedWorker c. After Frame A unload, Frame A should be evicted on Frame B unload. My concern here is that wrong early return implementation may overlook SharedWorker c here.
Yoshisato YanagisawaI've added new WPTs for:
1. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-same-instance.tentative.window.js
2. third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-different-instances.tentative.window.jsAs for case 3, I guess when Frame A unloads, it should be blocked from BFCache immediately, since it's the only client for `worker a`? Please let me know if I've misunderstood, or if you had a different scenario in mind.
Anna SatoAh, good point. maybe Frame B should also have `worker a` to realize the scenario? i.e. Frame A has `Worker a` and `Worker b`, and Frame B has `worker a`, `worker b`, and `worker c`. Then, it clarifies my question in content/browser/worker_host/shared_worker_service_impl.cc
Added a new WPT at third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.tentative.window.js. (Sorry for the multiple updates...)
- Frame 1 connects to workers A and B.
- Frame 2 connects to workers A and C.
- Frame 3 connects to worker B.
The test verifies that when Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is correctly evicted. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
I am still not completely sure if I understood the exact scenario you had in mind, please let me know if this new test covers your concern!
Sorry for going back and forward. My original motivation was creating a test case that won't work with `SharedWorkerServiceImpl::EvictBFCachedClientsIfLastActive()` to return at L542. I assume the case is covered by the case you wrote.
Then, I just started to come up with the question how RFH work if the eviction happens twice. Assume Frame 1 connects to worker A, B, and C (and others are connected to what you wrote). Frame 1 goes to BFCache, then Frame 2 navigate away.
Then, Frame 1 eviction might happen for Worker A and C. Will the second eviction just be ignored?
Related details
- Anna Sato
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
1 comment
lgtm w/ comments.
Anna Sato (Gerrit)
Anna Sato voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anna Sato
| Commit-Queue | +1 |
2 comments
UMA_HISTOGRAM_TIMES("SharedWorker.BFCache.LastClientCheckTime",
timer.Elapsed());Thank you for adding a trace and uma. Let me ask something more for uma.
1. if it is not in a critical path, a function is preferred to a macro (https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#coding-emitting-to-histograms)
2. please update histograms.xml (maybe tools/metrics/histograms/metadata/content/histograms.xml?) for this. Unfortunately, SharedWorker is not a top level metrics, I suggest to use the "Content.SharedWorker" prefix instead.
3. if you got stuck on writing a metric description, I suggest you to ask gemini to write it. Note that the description should be understandable without reading the code.
Totally forgot to add this, thanks. Added in tools/metrics/histograms/metadata/content/histograms.xml.
The second (and later) eviction should be skipped at https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=8770-8771;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18?q=EvictFromBackForwardCacheWithReason
I also confirmed it with adding some logging.
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
lgtm
'/wpt_internal/shared-worker/back-forward-cache/resources/worker-empty.js');- Anna Sato
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 4 comments![]( "Open in Gerrit")
// Neither should be restored from BFCache, sincfe a SharedWorker cannot besince
// Neither should be restored from BFCache, sincfe a SharedWorker cannot besince
// META: title=BFCache is blocked for a page with an iframe that is also a SharedWorker client.I think you need one more test which involves 2 frames in the same frame tree connecting to the same worker. Can be 2 iframes or main frame and iframe.
await rc1.executeScript((workerUrl) => {I think you can simplify this with
```
const iframe = rc1.addIframe(...);
iframe.executeScript(...);
```
Related details
- Anna Sato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 6 comments![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(blink::features::kBFCacheWithSharedWorker)) {Why in PopulateStickyReasonsForDocument? It's not a sticky reason.
// If this is the last active client of a Shared Worker, block this frameI would suggest writing this as
If this frame is a client of a Shared Worker and all other clients are in this frame's frame-tree, block this frame.
if (!rfh->IsInBackForwardCache()) {why do we need this?
const GlobalRenderFrameHostId& client_rfh_id) {Why not pass `RenderFrameHostImpl&` instead of converting to/from IDs?
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {This is called in a loop, so please capture the value outside the loop.
Since this method itself is called in a loop, I think we can capture it once in the outer caller.
Even that outer one is called traversing the frame tree. I wonder if we should be passing in the main frame all the way down the tree.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {I was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
still lgtm w/ minor comment.
RenderFrameHostImpl* client_render_frame_host_id);nit but I suggest to omit `_id`.
Related details
- Anna Sato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Anna Sato (Gerrit)
Anna Sato voted and added 12 comments![]( "Open in Gerrit")
Votes added by Anna Sato
| Commit-Queue | +1 |
12 comments
if (base::FeatureList::IsEnabled(blink::features::kBFCacheWithSharedWorker)) {Why in PopulateStickyReasonsForDocument? It's not a sticky reason.
That's right. Moved to `PopulateNonStickyReasonsForDocument`.
// If this is the last active client of a Shared Worker, block this frameI would suggest writing this as
If this frame is a client of a Shared Worker and all other clients are in this frame's frame-tree, block this frame.
Updated the comment.
if (!rfh->IsInBackForwardCache()) {why do we need this?
IIUC this code is called when pages are going into BFCache and being restored from it.
During a restore, there should be one more active clients, which would cause `EvictBFCachedClientsIfLastActive()` to return false. So the code would be WAI even without the `IsInBackForwardCache` check, but we can keep the check to avoid the cost of calling `EvictBFCachedClientsIfLastActive()`? (The new UMA metric "Content.SharedWorker.Service.LastClientCheckTime" will tell us exactly how much cost we're saving.)
RenderFrameHostImpl* client_render_frame_host_id);nit but I suggest to omit `_id`.
Changed to `render_frame_host` align with the .cc file, thanks!
Why not pass `RenderFrameHostImpl&` instead of converting to/from IDs?
Fixed.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {I was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Switched to use GetOutermostMainFrame instead of GetMainFrame.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {This is called in a loop, so please capture the value outside the loop.
Since this method itself is called in a loop, I think we can capture it once in the outer caller.
Even that outer one is called traversing the frame tree. I wonder if we should be passing in the main frame all the way down the tree.
Moved it outside the loop, and now checking only if the two frames share the same main frame.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {I was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
As we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
// Neither should be restored from BFCache, sincfe a SharedWorker cannot beAnna Satosince
Fixed.
// Neither should be restored from BFCache, sincfe a SharedWorker cannot beAnna Satosince
Fixed.
I think you can simplify this with
```
const iframe = rc1.addIframe(...);
iframe.executeScript(...);
```
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 1 comment![]( "Open in Gerrit")
// META: title=BFCache is blocked for a page with an iframe that is also a SharedWorker client.Anna SatoI think you need one more test which involves 2 frames in the same frame tree connecting to the same worker. Can be 2 iframes or main frame and iframe.
third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-iframe-client.tentative.window.js
This test tests it with the main frame and its iframe.
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
2 comments
still lgtm.
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
As we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Pardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
Related details
- Anna Sato
- Fergal Daly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 1 comment![]( "Open in Gerrit")
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato YanagisawaAs we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Pardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
I quickly checked locally and confirmed when only a fenced frame connects a SharedWorker and its main page navigates away, the page would be evicted with `masked` reason instead of `sharedworker-with-no-active-client`, meaning that workers are terminated correctly. (I'm not very sure if the reason `masked` is correct here, I couldn't find any test for Fenced Frames + BFCache.)
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 1 comment![]( "Open in Gerrit")
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato YanagisawaAs we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Anna SatoPardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
I quickly checked locally and confirmed when only a fenced frame connects a SharedWorker and its main page navigates away, the page would be evicted with `masked` reason instead of `sharedworker-with-no-active-client`, meaning that workers are terminated correctly. (I'm not very sure if the reason `masked` is correct here, I couldn't find any test for Fenced Frames + BFCache.)
Thanks to fergal, I was finally able to create the scenario we were concerned about by creating two fenced frames in a frame tree, and connect to the same worker. The worker would be terminated on its main page unload.
Added the test at `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-fenced-frame-client.tentative.https.window.js`
Sorry for adding many tests and making the CL larger but I believe it's worth to include them to make sure it's WAI.
Related details
- Fergal Daly
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fergal Daly (Gerrit)
Fergal Daly added 3 comments![]( "Open in Gerrit")
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato YanagisawaAs we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Anna SatoPardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
I quickly checked locally and confirmed when only a fenced frame connects a SharedWorker and its main page navigates away, the page would be evicted with `masked` reason instead of `sharedworker-with-no-active-client`, meaning that workers are terminated correctly. (I'm not very sure if the reason `masked` is correct here, I couldn't find any test for Fenced Frames + BFCache.)
I think a ShW can be shared between inside and outside the fenced frame. If the frame tree looks like
``` a.com
then I think both b.coms are the same storage partition and so will share workers.
Even if that's not correct, a frame tree like
```
a.com
- fenced b.com
- c.com
- c.com
```
with both c.com using ShW, we need to ensure that if these are the only clients of that ShW that we will evict.
For that to work reliably, we need to use `GetOutermostMainFrame` everywhere.
RenderFrameHostImpl::FromID(client_render_frame_host_id);I'd like to calculature the main frame here...
host->EvictBFCachedClientsIfLastActive(render_frame_host)) {and pass it in here, to avoid recalculating it every time in this loop too.
Related details
- Anna Sato
- Yoshisato Yanagisawa
Fergal Daly (Gerrit)
Fergal Daly added 6 comments![]( "Open in Gerrit")
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Addressed the case where a main frame and its iframe are connected to the same worker, which @fer...@chromium.org also pointed out in [doc](https://docs.google.com/document/d/19ZMaAaRUnMwB-EyQLjt4YZNSjKlGRTbFbjJ57H2ZIag/edit?resourcekey=0-K6Q2fbF8lAL2QYot2dE73Q&disco=AAABrySJO4w), by adding a check its main frame to ensure they aren't considered as independent active clients.
Added a test for this senario in `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-iframe-client.tentative.window.js.`
Acknowledged
let worker_id = null;Please add a comment like
// This worker sets a unique ID when first connected and posts that ID on when sent the "get_id" message.
// TODO(crbug.com/406420935): Remove this client once SharedWorker freezing isCan't we remove the whole test? I assume there are public WPTs that test the case of the document with a shared worker going into BFCache.
Same for the following tests?
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.No need for tentative in internal WPTs (in test name and in file name).
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.This title doesn't explain what scenario this is testing.
If it's too long, put it in a comment.
I can figure out what you're doing but it's better to document what you intend the test to do, then I can
- review your intention.
- review your code to see if it matches your intention.
const iframe = document.createElement('iframe');RCH has `addIframe`. I think you can remove all of the iframe_ready stuff.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/browsers/browsing-the-web/remote-context-helper/resources/remote-context-helper.js;l=396
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 10 comments![]( "Open in Gerrit")
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato YanagisawaAs we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Anna SatoPardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
Fergal DalyI quickly checked locally and confirmed when only a fenced frame connects a SharedWorker and its main page navigates away, the page would be evicted with `masked` reason instead of `sharedworker-with-no-active-client`, meaning that workers are terminated correctly. (I'm not very sure if the reason `masked` is correct here, I couldn't find any test for Fenced Frames + BFCache.)
I think a ShW can be shared between inside and outside the fenced frame. If the frame tree looks like
``` a.com
then I think both b.coms are the same storage partition and so will share workers.
Even if that's not correct, a frame tree like
```
a.com
- fenced b.com
- c.com
- c.com
```
with both c.com using ShW, we need to ensure that if these are the only clients of that ShW that we will evict.For that to work reliably, we need to use `GetOutermostMainFrame` everywhere.
Acknowledged and the pages will be evicted as expected. Do you want to me to add these tests as well?
RenderFrameHostImpl::FromID(client_render_frame_host_id);I'd like to calculature the main frame here...
Done
host->EvictBFCachedClientsIfLastActive(render_frame_host)) {and pass it in here, to avoid recalculating it every time in this loop too.
Changed to pass GetOutermostMainFrame() to SharedWorkerHost::EvictBFCachedClientsIfLastActive. Thanks!
let worker_id = null;Please add a comment like
// This worker sets a unique ID when first connected and posts that ID on when sent the "get_id" message.
Added.
// TODO(crbug.com/406420935): Remove this client once SharedWorker freezing isCan't we remove the whole test? I assume there are public WPTs that test the case of the document with a shared worker going into BFCache.
Same for the following tests?
Removed this test but leave other tests since they check if pages are evicted with the correct reason, unlike external WPTs.
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.No need for tentative in internal WPTs (in test name and in file name).
Removed "tentative" from the title and file name.
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.This title doesn't explain what scenario this is testing.
If it's too long, put it in a comment.
I can figure out what you're doing but it's better to document what you intend the test to do, then I can
- review your intention.
- review your code to see if it matches your intention.
Sorry for not being clear. Updated the test titles and added one comment at `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.window.js` on this CL.
// META: title=BFCache is blocked and SharedWorkers are terminated correctly when all clients are navigated away.The test verifies that when:
- Frame 1 connects to workers A and B.
- Frame 2 connects to workers A and C.
- Frame 3 connects to worker B.
and Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is evicted and Frame 2 is blocked. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
const iframe = document.createElement('iframe');RCH has `addIframe`. I think you can remove all of the iframe_ready stuff.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/browsers/browsing-the-web/remote-context-helper/resources/remote-context-helper.js;l=396
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Fergal Daly added 2 comments![]( "Open in Gerrit")
Why are you moving many of these from tentative? It's making it show these as a delete/add and I can't look at the diff. I'd rather remove tentative in a separate CL
if (!render_frame_host) {When does this happen? The caller passes an ID but it gets the ID from an RFH. Why not pass the RFH itself?
Related details
- Anna Sato
Anna Sato (Gerrit)
Anna Sato added 5 comments![]( "Open in Gerrit")
Why are you moving many of these from tentative? It's making it show these as a delete/add and I can't look at the diff. I'd rather remove tentative in a separate CL
Reverted. Moved them back to .tentative sorry.
You can view the diff between Patch Set 31 and 37. Thanks!
When does this happen? The caller passes an ID but it gets the ID from an RFH. Why not pass the RFH itself?
Ah, that makes sense now. Changed it to pass `RenderFrameHostImpl` and get the ID inside `ContainsClient()` where the ID is needed.
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.Anna SatoThis title doesn't explain what scenario this is testing.
If it's too long, put it in a comment.
I can figure out what you're doing but it's better to document what you intend the test to do, then I can
- review your intention.
- review your code to see if it matches your intention.
Sorry for not being clear. Updated the test titles and added one comment at `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.window.js` on this CL.
As disccussed in https://chromium-review.googlesource.com/c/chromium/src/+/6914636/comment/c05f6c5c_6b637bbe/,
The test verifies that when:
Frame 1 connects to workers A and B.
Frame 2 connects to workers A and C.
Frame 3 connects to worker B.
and Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is evicted and Frame 2 is blocked. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
// META: title=BFCache is blocked and SharedWorkers are terminated correctly when all clients are navigated away.As disccussed in https://chromium-review.googlesource.com/c/chromium/src/+/6914636/comment/c05f6c5c_6b637bbe/,
The test verifies that when:
- Frame 1 connects to workers A and B.
- Frame 2 connects to workers A and C.
- Frame 3 connects to worker B.
and Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is evicted and Frame 2 is blocked. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
Closing as this has moved to https://chromium-review.googlesource.com/c/chromium/src/+/6914636/comment/6c940fd1_28938646/.
Related details
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Fergal Daly added 4 comments![]( "Open in Gerrit")
if (!rfh->IsInBackForwardCache()) {Anna Satowhy do we need this?
IIUC this code is called when pages are going into BFCache and being restored from it.
During a restore, there should be one more active clients, which would cause `EvictBFCachedClientsIfLastActive()` to return false. So the code would be WAI even without the `IsInBackForwardCache` check, but we can keep the check to avoid the cost of calling `EvictBFCachedClientsIfLastActive()`? (The new UMA metric "Content.SharedWorker.Service.LastClientCheckTime" will tell us exactly how much cost we're saving.)
This code isn't called during a restore. Only when entering BFCache or when being evicted.
if (!rfh->IsInBackForwardCache()) {Based on the discussion above (which not attached to a line of code anymore) I think you should add a comment here like
// If `rfh` is already in BFCache then we are evicting it and there is no need to check if it's the last active client.
if (!other_rfh->IsInBackForwardCache()) {We need to be careful here because `!other_rfh->IsInBackForwardCache()` is different to `other_rfh->IsActive()`.
In particular, `!rfh->IsInBackForwardCache()` would include pre-rendering clients and also clients that are in the `kPendingDeletion` state.
Certainly I think pre-rendering RFHs shouldn't count and I think we have to also make sure that we cancel prerendering if any of the RFHs are pre-rendering.
Sorry, this is going on forever.
Related details
- Anna Sato
Anna Sato (Gerrit)
Anna Sato added 4 comments![]( "Open in Gerrit")
if (!rfh->IsInBackForwardCache()) {Anna Satowhy do we need this?
Fergal DalyIIUC this code is called when pages are going into BFCache and being restored from it.
During a restore, there should be one more active clients, which would cause `EvictBFCachedClientsIfLastActive()` to return false. So the code would be WAI even without the `IsInBackForwardCache` check, but we can keep the check to avoid the cost of calling `EvictBFCachedClientsIfLastActive()`? (The new UMA metric "Content.SharedWorker.Service.LastClientCheckTime" will tell us exactly how much cost we're saving.)
This code isn't called during a restore. Only when entering BFCache or when being evicted.
Got it, understood. I've left the check and added a comment as https://chromium-review.googlesource.com/c/chromium/src/+/6914636/comment/a539d476_0d5afc2a/
Based on the discussion above (which not attached to a line of code anymore) I think you should add a comment here like
// If `rfh` is already in BFCache then we are evicting it and there is no need to check if it's the last active client.
Thanks! Added the comment.
We need to be careful here because `!other_rfh->IsInBackForwardCache()` is different to `other_rfh->IsActive()`.
In particular, `!rfh->IsInBackForwardCache()` would include pre-rendering clients and also clients that are in the `kPendingDeletion` state.
Certainly I think pre-rendering RFHs shouldn't count and I think we have to also make sure that we cancel prerendering if any of the RFHs are pre-rendering.
Sorry, this is going on forever.
Thanks for catching this important edge case! Since the fix is non-trivial, I've added a TODO and will address this in a follow-up CL.
Do you really need this `\`?
- Fergal Daly
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. |
Anna Sato (Gerrit)
Anna Sato voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anna Sato
| Commit-Queue | +1 |
1 comment
if (!other_rfh->IsInBackForwardCache()) {Anna SatoWe need to be careful here because `!other_rfh->IsInBackForwardCache()` is different to `other_rfh->IsActive()`.
In particular, `!rfh->IsInBackForwardCache()` would include pre-rendering clients and also clients that are in the `kPendingDeletion` state.
Certainly I think pre-rendering RFHs shouldn't count and I think we have to also make sure that we cancel prerendering if any of the RFHs are pre-rendering.
Sorry, this is going on forever.
Thanks for catching this important edge case! Since the fix is non-trivial, I've added a TODO and will address this in a follow-up CL.
Created https://crrev.com/c/7024414/ for it. While writing tests for these cases, I noticed that prerendered pages do not become SharedWorker clients until after the page is activated.
There are WPTs for pre-render + SharedWorker:
- [Shared workers should be loaded in suspended state until activated](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=39-64)
- [Existing shared workers should be accessible before activation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=66-94)
The second case is what we need to care about here, as it says "The prerendered page could be a client of the existing worker". But this test is expected to fail in [TestExpectations](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/TestExpectations;l=2806) and found the description in https://crrev.com/c/3874273, saying this is the known current behavior in Chrome. (I confirmed locally that it fails at [L90](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=90). )
Updated the TODO comment to only cover the kPendingDeletion case. Does that seem right to you?
Related details
- Fergal Daly
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. |
Fergal Daly (Gerrit)
Fergal Daly voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fergal Daly
| Code-Review | +1 |
1 comment
if (!other_rfh->IsInBackForwardCache()) {Let's make this `IsActive()` and it will cover that. I don't think you need to write tests for the pending deletion case, it's impossible to write a WPT for it and not having a browser test will be OK, I think
Related details
- Anna Sato
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. |
Anna Sato (Gerrit)
Anna Sato added 5 comments![]( "Open in Gerrit")
Adding andruud@ for Virtualtestsuite, and guiperez@ for histograms.xml. The other parts of the CL have already been LGTM'd. Thanks!
other_rfh->GetMainFrame() == client_rfh->GetMainFrame()) {Anna SatoI was chatting with rakina and I think you want to use `GetOutermostMainFrame` for these to account for fenced frames.
Ideally have a test for that too.
Yoshisato YanagisawaAs we chatted, if fenced frames cannot share a SharedWorker with its main frame, using `GetMainFrame()` here is enough...? but I changed it to `GetOutermostMainFrame()` anyway as it seems like more robust option.
Anna SatoPardon the question from a layman, but I'm trying to understand a specific lifecycle case. Given that a SharedWorker within a Fenced Frame cannot be shared with the embedder, what is the expected behavior when the embedding document is stored in the BFCache? It seems the worker would be left with no active clients, and I'm wondering how that state is managed upon restoration.
Fergal DalyI quickly checked locally and confirmed when only a fenced frame connects a SharedWorker and its main page navigates away, the page would be evicted with `masked` reason instead of `sharedworker-with-no-active-client`, meaning that workers are terminated correctly. (I'm not very sure if the reason `masked` is correct here, I couldn't find any test for Fenced Frames + BFCache.)
Anna SatoI think a ShW can be shared between inside and outside the fenced frame. If the frame tree looks like
``` a.com
then I think both b.coms are the same storage partition and so will share workers.
Even if that's not correct, a frame tree like
```
a.com
- fenced b.com
- c.com
- c.com
```
with both c.com using ShW, we need to ensure that if these are the only clients of that ShW that we will evict.For that to work reliably, we need to use `GetOutermostMainFrame` everywhere.
Acknowledged and the pages will be evicted as expected. Do you want to me to add these tests as well?
Done
Anna SatoWe need to be careful here because `!other_rfh->IsInBackForwardCache()` is different to `other_rfh->IsActive()`.
In particular, `!rfh->IsInBackForwardCache()` would include pre-rendering clients and also clients that are in the `kPendingDeletion` state.
Certainly I think pre-rendering RFHs shouldn't count and I think we have to also make sure that we cancel prerendering if any of the RFHs are pre-rendering.
Sorry, this is going on forever.
Anna SatoThanks for catching this important edge case! Since the fix is non-trivial, I've added a TODO and will address this in a follow-up CL.
Created https://crrev.com/c/7024414/ for it. While writing tests for these cases, I noticed that prerendered pages do not become SharedWorker clients until after the page is activated.
There are WPTs for pre-render + SharedWorker:
- [Shared workers should be loaded in suspended state until activated](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=39-64)
- [Existing shared workers should be accessible before activation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=66-94)
The second case is what we need to care about here, as it says "The prerendered page could be a client of the existing worker". But this test is expected to fail in [TestExpectations](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/TestExpectations;l=2806) and found the description in https://crrev.com/c/3874273, saying this is the known current behavior in Chrome. (I confirmed locally that it fails at [L90](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prerender/workers.https.html;l=90). )
Updated the TODO comment to only cover the kPendingDeletion case. Does that seem right to you?
Done
Let's make this `IsActive()` and it will cover that. I don't think you need to write tests for the pending deletion case, it's impossible to write a WPT for it and not having a browser test will be OK, I think
Thanks! Switched from `!rfh->IsInBackForwardCache()` to `rfh-> IsActive()`.
// META: title=[Tentative] BFCache is blocked/evicted and SharedWorkers are terminated correctly.Anna SatoThis title doesn't explain what scenario this is testing.
If it's too long, put it in a comment.
I can figure out what you're doing but it's better to document what you intend the test to do, then I can
- review your intention.
- review your code to see if it matches your intention.
Anna SatoSorry for not being clear. Updated the test titles and added one comment at `third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.window.js` on this CL.
As disccussed in https://chromium-review.googlesource.com/c/chromium/src/+/6914636/comment/c05f6c5c_6b637bbe/,
The test verifies that when:
Frame 1 connects to workers A and B.
Frame 2 connects to workers A and C.
Frame 3 connects to worker B.
and Frame 1 is in BFCache and Frame 2 navigates away, Frame 1 is evicted and Frame 2 is blocked. It also verifies the worker state, ensuring A and C are terminated while B is kept alive.
- Anders Hartvoll Ruud
- Guillem Perez
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
1 comment
- Anna Sato
- Guillem Perez
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. |
Guillem Perez (Gerrit)
Guillem Perez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Guillem Perez
| Code-Review | +1 |
1 comment
LGTM Histograms
Related details
- Anna Sato
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. |
Anna Sato (Gerrit)
Anna Sato voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anna Sato
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Thanks for your review!
// TODO(crbug.com/406420935): Remove this client once SharedWorker freezing isAnna SatoCan't we remove the whole test? I assume there are public WPTs that test the case of the document with a shared worker going into BFCache.
Same for the following tests?
Removed this test but leave other tests since they check if pages are evicted with the correct reason, unlike external WPTs.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55410.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Evict BFCached pages with SharedWorker when the last client navigates away
This CL prevents a SharedWorker from remaining active when all of its
clients are in BFCache. When the last active client of a SharedWorker
navigates away:
- The navigating client is blocked from entering BFCache.
- Any other clients of the same SharedWorker that are already in BFCache are evicted.
- As a result, the SharedWorker is left with no clients and is
subsequently terminated.
This change is the minimum behavior required for the SharedWorkerBFCache
launch as described at "Worker state when last client enters BFCache" in
[1]. The better long-term solution is to freeze the worker instead of
evicting clients, which is noted as TODOs.
[1]
https://docs.google.com/document/d/1dZxwmUYOlDmZ-PF97mfoXbzMrFHBsqeUT3XxTkpNIb0/edit?usp=sharing
- M content/browser/renderer_host/back_forward_cache_impl.cc
- M content/browser/worker_host/shared_worker_host.cc
- M content/browser/worker_host/shared_worker_host.h
- M content/browser/worker_host/shared_worker_service_impl.cc
- M content/browser/worker_host/shared_worker_service_impl.h
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/external/wpt/html/browsers/browsing-the-web/back-forward-cache/eligibility/shared-worker-active-client.window.js
- A third_party/blink/web_tests/external/wpt/html/browsers/browsing-the-web/back-forward-cache/eligibility/shared-worker-expected.txt
- A third_party/blink/web_tests/external/wpt/web-locks/bfcache/abort.tentative.https-expected.txt
- D third_party/blink/web_tests/external/wpt/web-locks/bfcache/held.tentative.https-expected.txt
- A third_party/blink/web_tests/external/wpt/web-locks/bfcache/release.tentative.https-expected.txt
- D third_party/blink/web_tests/external/wpt/web-locks/bfcache/sharedworker-multiple.tentative.https-expected.txt
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/resources/stateful-worker.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-all-clients-bfcached.tentative.window.js
- D third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-allows-bfcache.window.js
- M third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-client-postmessage-transferred-port.window.js
- M third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-client-postmessage.window.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-mixed-clients-eviction.tentative.window.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-different-instances.tentative.window.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-multiple-clients-same-instance.tentative.window.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-fenced-frame-client.tentative.https.window.js
- A third_party/blink/web_tests/wpt_internal/shared-worker/back-forward-cache/shared-worker-with-iframe-client.tentative.window.js
- M tools/metrics/histograms/metadata/content/histograms.xml
Code-Review: +1 by Guillem Perez, +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/55410
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |