[DebugUITabStrip] Add SessionRestore metadata to tab-strip-internals [chromium/src : main]
Brijesh Giri (Gerrit)
Brijesh Giri added 2 comments![]( "Open in Gerrit")
void SessionRestore::OnGotSession(Does this method represent a `Primitive Obsession` code-smell?
Would it make sense to group related session restore properties together in a struct (data-class)?
virtual void OnGotSession(Question: Does adding a new overload here break Interface Segregation Principle?
Or does providing a default no-op implementation make it exempt from the anti-pattern?
Related details
- Darryl James
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 1 comment![]( "Open in Gerrit")
SessionRestoreData restored_session;I was able to hook into both `SessionService` (to read session data from disk without invalidating it) and `SessionRestoreObserver` (to observe callbacks when a session is restored). I added fields for both data sources to test how they behave in different scenarios.
From my testing, retrieving data through the observer is more ephemeral i.e. our debug page must already be open when the restore action occurs for it to receive those callbacks. In other words, you need to open the debug page first and then trigger the restore; refreshing the page creates new instances that no longer have access to the previously received callback data, resulting in an empty `restored_session`.
In contrast, the `saved_session` data obtained through SessionService remains valid across refreshes and after restore operations. During testing, I observed that the session data persisted on disk and was not immediately invalidated by a restore. Only closing the browser caused the on-disk session state to be replaced with new data.
Related details
- Darryl James
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. |
Brijesh Giri (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Atharv Maan
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James added 4 comments![]( "Open in Gerrit")
int window_count,This one can be removed since we can get the number of windows from `windows`
void SessionRestore::OnGotSession(Does this method represent a `Primitive Obsession` code-smell?
Would it make sense to group related session restore properties together in a struct (data-class)?
You can definitely group the properties in a struct, however in this case there are few enough parameters where it won't give us too much value (especially when window count gets removed).
virtual void OnGotSession(Question: Does adding a new overload here break Interface Segregation Principle?
Or does providing a default no-op implementation make it exempt from the anti-pattern?
I would update the existing one and remove this overload. `window_count` can be inferred from `windows`. This also allows the callers to be funnelled into one function making it easier to debug (you only have to worry about 1 implementation).
SessionRestoreData restored_session;I was able to hook into both `SessionService` (to read session data from disk without invalidating it) and `SessionRestoreObserver` (to observe callbacks when a session is restored). I added fields for both data sources to test how they behave in different scenarios.
From my testing, retrieving data through the observer is more ephemeral i.e. our debug page must already be open when the restore action occurs for it to receive those callbacks. In other words, you need to open the debug page first and then trigger the restore; refreshing the page creates new instances that no longer have access to the previously received callback data, resulting in an empty `restored_session`.
In contrast, the `saved_session` data obtained through SessionService remains valid across refreshes and after restore operations. During testing, I observed that the session data persisted on disk and was not immediately invalidated by a restore. Only closing the browser caused the on-disk session state to be replaced with new data.
That is surprising to me! Let's keep this behavior for now 👍
Related details
- Atharv Maan
- Brijesh Giri
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 5 comments![]( "Open in Gerrit")
const std::vector<const sessions::SessionWindow*>& windows) {Could this potentially be a dangling reference issue if the caller of this observer method is destroyed while the observer is acting on the callback?
For context, these observer methods are invoked by `SessionRestore` which owns the actual `SessionWindow` objects. The observers only receive a vector of raw pointers to the `SessionWindow` objects.
This one can be removed since we can get the number of windows from `windows`
Done
void SessionRestore::OnGotSession(Darryl JamesDoes this method represent a `Primitive Obsession` code-smell?
Would it make sense to group related session restore properties together in a struct (data-class)?
You can definitely group the properties in a struct, however in this case there are few enough parameters where it won't give us too much value (especially when window count gets removed).
Done
virtual void OnGotSession(Darryl JamesQuestion: Does adding a new overload here break Interface Segregation Principle?
Or does providing a default no-op implementation make it exempt from the anti-pattern?
I would update the existing one and remove this overload. `window_count` can be inferred from `windows`. This also allows the callers to be funnelled into one function making it easier to debug (you only have to worry about 1 implementation).
Done
last_session_windows_.push_back(DeepCopySessionWindow(window));Deep copying as a precaution in-case the caller of this observer method gets destroyed before this callback is fully processed.
`SessionRestore` passes a vector of raw pointers to `SessionWindow` objects to the observers, which could potentially be destroyed before the TabStripInternalsObserver is completed with its callback flow.
Related details
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James added 4 comments![]( "Open in Gerrit")
const std::vector<const sessions::SessionWindow*>& windows) {Could this potentially be a dangling reference issue if the caller of this observer method is destroyed while the observer is acting on the callback?
For context, these observer methods are invoked by `SessionRestore` which owns the actual `SessionWindow` objects. The observers only receive a vector of raw pointers to the `SessionWindow` objects.
Were you seeing this happen by any chance?
I think in the general sense that would be possible but if we look at when the observers are notified [starting from here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/sessions/session_restore.cc;drc=b32663066c4707a90f117b579f1993c7b74a0ad1;l=284) we can see that in the callback we are passing a weak ptr which helps us with the lifetime management if session restore is destroyed.
Going into OnGotSession, we can see that at that point in time we have the windows vector and that the observers are immediately notified of the session we have retrieved. From that perspective, we should not encounter any dangling pointer issues. I guess someone could remove / delete one of the session windows before handing the vector to callers but that would be a separate issue entirely!
So overall, I don't think we have a dangling pointer here. Do let me know if that is not the case!
// `components/sessions/core/session_types.h.h`super nit: remove the additional `.h` 👍
pending_callback_ = std::move(callback);Should we check if there are an already pending_callback_ before doing anything else? Maybe early return?
My worry is that this could get called multiple times and we end up overwriting the data and the wrong callback ends up getting called.
My preference would be to early return if there is an early callback. But depending on how we want to handle things, it would also be okay to store a list of pending callbacks that we process asynchronously / as they are called. But that might be more than we need in this case.
last_session_windows_.push_back(DeepCopySessionWindow(window));Deep copying as a precaution in-case the caller of this observer method gets destroyed before this callback is fully processed.
`SessionRestore` passes a vector of raw pointers to `SessionWindow` objects to the observers, which could potentially be destroyed before the TabStripInternalsObserver is completed with its callback flow.
Let's add a brief comment to why we are deep copying here for future reference 👍
Related details
- Brijesh Giri
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 1 comment![]( "Open in Gerrit")
pending_callback_ = std::move(callback);Should we check if there are an already pending_callback_ before doing anything else? Maybe early return?
My worry is that this could get called multiple times and we end up overwriting the data and the wrong callback ends up getting called.
My preference would be to early return if there is an early callback. But depending on how we want to handle things, it would also be okay to store a list of pending callbacks that we process asynchronously / as they are called. But that might be more than we need in this case.
Good idea. I admit that in-case of multiple calls, we would respond to the latest (incorrect) callback.
I have a chain of thought that I need to validate with you, apologies in advance for the long comment.
Could we also potentially add a `CHECK` to guard against having multiple in-flight requests?
My reasoning here is that we never call this method twice from our renderer process. We only call `GetTabStripData` once during page load.
I assume that on a page refresh, the entire WebUI stack is torn down: the C++ objects (controller and page handler) are destroyed along with any `pending_callback_`, the Mojo pipes are closed, and the JS objects are discarded. A new set of C++ and JS objects are then created, and the renderer issues a fresh `GetTabStripData()` call to the new handler. In summary, a stale pending_callback_ cannot survive a reload.
Similarly, if multiple `chrome://tab-strip-internals` tabs are open, each tab has its own independent C++ and JS WebUI instances. Each one calls GetTabStripData() exactly once on its own handler, so even in this scenario we never end up issuing multiple calls to the same handler instance.
We have below three options, all of which ensure we send response to the correct callback:
- Early return (Drop duplicates)
- Maintain a list of callbacks
- Establish that only 1 in-flight request can be handled from the a single renderer (debug UI tab) at a time by adding a CHECK
I agree with the `early return` suggestion, just needed to confirm my understanding of object lifecycle in a WebUI, hence the comment.
Related details
- Darryl James
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 2 comments![]( "Open in Gerrit")
super nit: remove the additional `.h` 👍
Done. Thanks for catching this!
last_session_windows_.push_back(DeepCopySessionWindow(window));Darryl JamesDeep copying as a precaution in-case the caller of this observer method gets destroyed before this callback is fully processed.
`SessionRestore` passes a vector of raw pointers to `SessionWindow` objects to the observers, which could potentially be destroyed before the TabStripInternalsObserver is completed with its callback flow.
Let's add a brief comment to why we are deep copying here for future reference 👍
Done
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
pending_callback_ = std::move(callback);Brijesh GiriShould we check if there are an already pending_callback_ before doing anything else? Maybe early return?
My worry is that this could get called multiple times and we end up overwriting the data and the wrong callback ends up getting called.
My preference would be to early return if there is an early callback. But depending on how we want to handle things, it would also be okay to store a list of pending callbacks that we process asynchronously / as they are called. But that might be more than we need in this case.
Good idea. I admit that in-case of multiple calls, we would respond to the latest (incorrect) callback.
I have a chain of thought that I need to validate with you, apologies in advance for the long comment.
Could we also potentially add a `CHECK` to guard against having multiple in-flight requests?
My reasoning here is that we never call this method twice from our renderer process. We only call `GetTabStripData` once during page load.
I assume that on a page refresh, the entire WebUI stack is torn down: the C++ objects (controller and page handler) are destroyed along with any `pending_callback_`, the Mojo pipes are closed, and the JS objects are discarded. A new set of C++ and JS objects are then created, and the renderer issues a fresh `GetTabStripData()` call to the new handler. In summary, a stale pending_callback_ cannot survive a reload.
Similarly, if multiple `chrome://tab-strip-internals` tabs are open, each tab has its own independent C++ and JS WebUI instances. Each one calls GetTabStripData() exactly once on its own handler, so even in this scenario we never end up issuing multiple calls to the same handler instance.
We have below three options, all of which ensure we send response to the correct callback:
- Early return (Drop duplicates)
- Maintain a list of callbacks
- Establish that only 1 in-flight request can be handled from the a single renderer (debug UI tab) at a time by adding a CHECK
I agree with the `early return` suggestion, just needed to confirm my understanding of object lifecycle in a WebUI, hence the comment.
Good question, my understanding is that in general everything gets torn down when the page is reloaded / a new render frame host is assigned to the web contents.
I wouldn't be surprised if there were caveats to this rule though. So playing it safe is preferred. If possible I would like to avoid CHECKs on the UI as that would crash the program when it doesn't have to. It's okay to show stale data in this case.
Related details
- Brijesh Giri
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
return;Let's also call `std::move(callback).Run(BuildSnapshot());` in this block. I want to prevent the UI from hanging if this is called multiple times and we have an in flight request. By running the snapshot immediately, stale data will return but that is okay provided the asynchronous data will update the page once it is done. Otherwise the user should be able to refresh the page to get the latest data 👍
Related details
- Brijesh Giri
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 1 comment![]( "Open in Gerrit")
Let's also call `std::move(callback).Run(BuildSnapshot());` in this block. I want to prevent the UI from hanging if this is called multiple times and we have an in flight request. By running the snapshot immediately, stale data will return but that is okay provided the asynchronous data will update the page once it is done. Otherwise the user should be able to refresh the page to get the latest data 👍
Done!
PS: I tested out both the scenarios (hanging the UI as well as sending stale data). The UI lag is not that noticeable because the async call resolves pretty quickly.
Related details
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Atharv Maan (Gerrit)
Atharv Maan added 5 comments![]( "Open in Gerrit")
const std::vector<const sessions::SessionWindow*>& windows) override;forward declare this.
windows_view.reserve(windows_.size());What's the difference in using `windows` vs `windows_`? Is there a reason why we switched to the class member `windows_`?
namespace sessions {
struct SessionWindow;
} // namespace sessionssupernit: remove the newlines inside namespace
std::unique_ptr<sessions::SessionTab> DeepCopySessionTab(IWYU
const std::vector<std::unique_ptr<sessions::SessionWindow>>& windows);include <memory> and <vector>
Related details
- Brijesh Giri
- Darryl James
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 5 comments![]( "Open in Gerrit")
const std::vector<const sessions::SessionWindow*>& windows) override;forward declare this.
It is already forward-declared in the `c/b/sessions/session_restore_observer.h` header that this file depends due to inheritance i.e. `FullRestoreAppLaunchHandler` is a `SessionRestoreObserver` and needs to import it's header.
windows_view.reserve(windows_.size());What's the difference in using `windows` vs `windows_`? Is there a reason why we switched to the class member `windows_`?
In the existing code `windows` gets copied over to `windows_` with the below comment and cleared immediately:
```
// Copy windows into windows_ so that we can combine both app and browser
// windows together before doing a one-pass restore.
std::ranges::move(windows, std::back_inserter(windows_));
SessionRestore::OnGotSession(profile(), for_apps, windows.size());
windows.clear();
```
Both are identical at the point when our newly added code gets executed:
```
// Build a read-only view of the windows for the observers.
std::vector<const sessions::SessionWindow*> windows_view;
windows_view.reserve(windows_.size());
for (const auto& w : windows_) {
windows_view.push_back(w.get());
}
```
There was no specific reason for choosing one over the other, either of them work for our purpose of building a read-only view.
namespace sessions {
struct SessionWindow;
} // namespace sessionssupernit: remove the newlines inside namespace
Done
std::unique_ptr<sessions::SessionTab> DeepCopySessionTab(Brijesh GiriIWYU
Done
const std::vector<std::unique_ptr<sessions::SessionWindow>>& windows);Brijesh Giriinclude <memory> and <vector>
Done
Related details
- Atharv Maan
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
2 comments
// Session data that is saved on disk
// to be restored on next startup after a crash.super nit: Let's wrap at 80 characters here per style guide 👍
Related details
- Atharv Maan
- Brijesh Giri
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 1 comment![]( "Open in Gerrit")
// Session data that is saved on disk
// to be restored on next startup after a crash.super nit: Let's wrap at 80 characters here per style guide 👍
- Atharv Maan
- Darryl James
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. |