[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. |
Darryl James (Gerrit)
Darryl James voted Code-Review+1![]( "Open in Gerrit")
- 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. |
Atharv Maan (Gerrit)
Atharv Maan added 3 comments![]( "Open in Gerrit")
We have zero test coverage for the added functionality. Let's add unit/browsertests for this CL.
const std::vector<const sessions::SessionWindow*>& windows) override;Brijesh Giriforward 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.
Did some research on this - We should avoid transitive dependencies, and aim for each header file to be self-contained.
Lets forward declare the `sessions::SessionWindow` class here.
windows_view.reserve(windows_.size());Brijesh GiriWhat'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.
- 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")
const std::vector<const sessions::SessionWindow*>& windows) override;Brijesh Giriforward declare this.
Atharv MaanIt 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.
Did some research on this - We should avoid transitive dependencies, and aim for each header file to be self-contained.
Lets forward declare the `sessions::SessionWindow` class here.
I agree, however, I had a query.
In this case, a `FullRestoreAppLaunchHandler` IS-A `SessionRestoreObserver` and will always need to import it's header by definition.
Is it okay to rely on the parent class's forward declarations if there is an inheritance between two classes?
Related details
- Atharv Maan
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. |
พันนา แสงทอง (Gerrit)
พันนา แสงทอง added 1 comment![]( "Open in Gerrit")
We have zero test coverage for the added functionality. Let's add unit/browsertests for this CL.
- Atharv Maan
- 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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Atharv Maan
- Darryl James
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 6 comments![]( "Open in Gerrit")
int32 window_id;
int32 tab_id;How about defining `SessionID` to avoid unnecessary general type uses here?
```
// This represents `components/sessions/core/session_id.h`
struct SessionID {
int32 id;
}
```
string session_storage_persistent_id;Can we have a clear comment on this?
Generally, we discourage to use string type in mojom, but in this case, we need to pass the C++ side string as-is? In such a case, can we clarify how we use this string in the remote end?
string guid;Hum... It seems this is std::string also in C++ side, but can we use base/uuid.h and mojo_base.mojom.Uuid for this guid value?
string title;This is very trivial, but it's still nice to have a similar comment to the one for the `session_storage_persistent_id`.
string title;
string workspace;ditto, detailed comments for string uses.
Related details
- Atharv Maan
- 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 1 comment![]( "Open in Gerrit")
const std::vector<const sessions::SessionWindow*>& windows) override;Brijesh Giriforward declare this.
Atharv MaanIt 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.
Brijesh GiriDid some research on this - We should avoid transitive dependencies, and aim for each header file to be self-contained.
Lets forward declare the `sessions::SessionWindow` class here.
I agree, however, I had a query.
In this case, a `FullRestoreAppLaunchHandler` IS-A `SessionRestoreObserver` and will always need to import it's header by definition.
Is it okay to rely on the parent class's forward declarations if there is an inheritance between two classes?
Added the forward declaration as per the comment.
Thank You!
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. |
Brijesh Giri (Gerrit)
Brijesh Giri added 1 comment![]( "Open in Gerrit")
int32 window_id;
int32 tab_id;How about defining `SessionID` to avoid unnecessary general type uses here?
```
// This represents `components/sessions/core/session_id.h`
struct SessionID {
int32 id;
}
```
Just providing some context here, this data always flows from the browser → renderer down the privilege gradient. It is only meant to display debugging information on a debug WebUI.
Our understanding is that data validation with stronger types is required when data flows from renderer → browser, but for the opposite direction (browser → renderer), the renderer is allowed to trust the browser’s data.
Reference: https://bit.ly/3XgRekg
```
When passing objects up a privilege gradient (from less → more privileged), the callee must validate the inputs before acting on them. When passing objects down a privilege gradient, such as from browser → renderer, it is OK for the callee to trust the caller.
```
Related details
- Atharv Maan
- Darryl James
- Takashi Toyoshima
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
lgtm % asks in .mojom and a question in the test
// Session restore pref must be set to LAST to test browser close and
// restore.
SessionStartupPref pref(SessionStartupPref::LAST);
SessionStartupPref::SetStartupPref(profile, pref);Was there a reason this needed to be moved out of SetUpOnMainThread?
Related details
- Atharv Maan
- Brijesh Giri
- Takashi Toyoshima
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 restore pref must be set to LAST to test browser close and
// restore.
SessionStartupPref pref(SessionStartupPref::LAST);
SessionStartupPref::SetStartupPref(profile, pref);Was there a reason this needed to be moved out of SetUpOnMainThread?
This is not required for other tests so I moved it out of the common test fixture setup.
Do let me know if there are any hard requirements about running it in the `SetUpOnMainThread` method.
Thanks!
Related details
- Atharv Maan
- Darryl James
- Takashi Toyoshima
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
// Session restore pref must be set to LAST to test browser close and
// restore.
SessionStartupPref pref(SessionStartupPref::LAST);
SessionStartupPref::SetStartupPref(profile, pref);Brijesh GiriWas there a reason this needed to be moved out of SetUpOnMainThread?
This is not required for other tests so I moved it out of the common test fixture setup.
Do let me know if there are any hard requirements about running it in the `SetUpOnMainThread` method.
Thanks!
Got it - I don't have any problems if all of the tests that need the pref set flow through this function 👍
Thanks!
Related details
- Atharv Maan
- Brijesh Giri
- Takashi Toyoshima
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
int32 window_id;
int32 tab_id;Brijesh GiriHow about defining `SessionID` to avoid unnecessary general type uses here?
```
// This represents `components/sessions/core/session_id.h`
struct SessionID {
int32 id;
}
```
Just providing some context here, this data always flows from the browser → renderer down the privilege gradient. It is only meant to display debugging information on a debug WebUI.
Our understanding is that data validation with stronger types is required when data flows from renderer → browser, but for the opposite direction (browser → renderer), the renderer is allowed to trust the browser’s data.
Reference: https://bit.ly/3XgRekg
```
When passing objects up a privilege gradient (from less → more privileged), the callee must validate the inputs before acting on them. When passing objects down a privilege gradient, such as from browser → renderer, it is OK for the callee to trust the caller.
```
We have a review guideline here.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.zlvrdwxzhuu
So, mojom prefers specific type rather than generic type, such as integers and string, regardless of the direction.
Also, just in case, are these all in this file for debugging purpose?
Otherwise, it's better to split the interface, and give it to the UI renderer only when the debugging is enabled.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.hus2vlat0wvq
This comes from the section, "Scope capabilities and privileges with separate interfaces".
But, IIUC, these all are for chrome://tab-strip-internals/ and this interface is given only to renderers that host it. In such a case, please ignore my second comment above.
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 7 comments![]( "Open in Gerrit")
string node_id;Given the opaque nature of this property would it be okay to store it as a string?
int32 window_id;
int32 tab_id;Brijesh GiriHow about defining `SessionID` to avoid unnecessary general type uses here?
```
// This represents `components/sessions/core/session_id.h`
struct SessionID {
int32 id;
}
```
Takashi ToyoshimaJust providing some context here, this data always flows from the browser → renderer down the privilege gradient. It is only meant to display debugging information on a debug WebUI.
Our understanding is that data validation with stronger types is required when data flows from renderer → browser, but for the opposite direction (browser → renderer), the renderer is allowed to trust the browser’s data.
Reference: https://bit.ly/3XgRekg
```
When passing objects up a privilege gradient (from less → more privileged), the callee must validate the inputs before acting on them. When passing objects down a privilege gradient, such as from browser → renderer, it is OK for the callee to trust the caller.
```
We have a review guideline here.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.zlvrdwxzhuu
So, mojom prefers specific type rather than generic type, such as integers and string, regardless of the direction.Also, just in case, are these all in this file for debugging purpose?
Otherwise, it's better to split the interface, and give it to the UI renderer only when the debugging is enabled.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.hus2vlat0wvqThis comes from the section, "Scope capabilities and privileges with separate interfaces".
But, IIUC, these all are for chrome://tab-strip-internals/ and this interface is given only to renderers that host it. In such a case, please ignore my second comment above.
Regarding point 2 (splitting the interface), yes, I can confirm that all the fields in this mojom file are meant to display read-only debugging information in `chrome://tab-strip-internals`.
The overall goal of this page is to provide quick access to TabStrip and SessionRestore information to help developers debug tab behavior.
PS: I have declared the rqeuired type, please let me know if it's as per expectations.
string session_storage_persistent_id;Can we have a clear comment on this?
Generally, we discourage to use string type in mojom, but in this case, we need to pass the C++ side string as-is? In such a case, can we clarify how we use this string in the remote end?
Please let me know if the provide comment is as per expectations.
string guid;Hum... It seems this is std::string also in C++ side, but can we use base/uuid.h and mojo_base.mojom.Uuid for this guid value?
Need a little help to get more insights into what this field represents and why we store this field as a string in `components/sessions/core/session_types.h` instead of a `base/uuid.h`
Is this `guid` unique across sessions?
This is very trivial, but it's still nice to have a similar comment to the one for the `session_storage_persistent_id`.
Done
int32 window_id;Brijesh Giriditto, SessionId.
Done
string title;
string workspace;ditto, detailed comments for string uses.
Please let me know if the provide comment is as per expectations.
Related details
- Atharv Maan
- Darryl James
- Takashi Toyoshima
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 2 comments![]( "Open in Gerrit")
string session_storage_persistent_id;Brijesh GiriCan we have a clear comment on this?
Generally, we discourage to use string type in mojom, but in this case, we need to pass the C++ side string as-is? In such a case, can we clarify how we use this string in the remote end?
Please let me know if the provide comment is as per expectations.
I did some digging to verify this and it appears that this field is unique across all sessions - [reference here](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/session_storage_namespace.h;drc=5a250038051d51f9573947ccf0f77f72da780725;bpv=1;bpt=1;l=21#:~:text=//%20Returns%20the%20ID,across%20browser%20runs.)
Let's update the comment to reflect that 👍
string guid;Brijesh GiriHum... It seems this is std::string also in C++ side, but can we use base/uuid.h and mojo_base.mojom.Uuid for this guid value?
Need a little help to get more insights into what this field represents and why we store this field as a string in `components/sessions/core/session_types.h` instead of a `base/uuid.h`
Is this `guid` unique across sessions?
In this one, guid is a way to associate the specific tab being worked with. It was introduced in [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/2093691).
This allows the embedder to associate and persist state with a tab. Though I wasn't able to find any callers or usages for this field. I am surprised this is even getting populated at this point in time.
My thoughts here are to leave the field in the struct that way there is a 1:1 mapping between mojom and c++, and update the comment. Separately, in the future double back and see if this field can be removed entirely if it is genuinely unused.
Related details
- Brijesh Giri
- Takashi Toyoshima
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
int32 window_id;
int32 tab_id;Brijesh GiriHow about defining `SessionID` to avoid unnecessary general type uses here?
```
// This represents `components/sessions/core/session_id.h`
struct SessionID {
int32 id;
}
```
Takashi ToyoshimaJust providing some context here, this data always flows from the browser → renderer down the privilege gradient. It is only meant to display debugging information on a debug WebUI.
Our understanding is that data validation with stronger types is required when data flows from renderer → browser, but for the opposite direction (browser → renderer), the renderer is allowed to trust the browser’s data.
Reference: https://bit.ly/3XgRekg
```
When passing objects up a privilege gradient (from less → more privileged), the callee must validate the inputs before acting on them. When passing objects down a privilege gradient, such as from browser → renderer, it is OK for the callee to trust the caller.
```
Brijesh GiriWe have a review guideline here.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.zlvrdwxzhuu
So, mojom prefers specific type rather than generic type, such as integers and string, regardless of the direction.Also, just in case, are these all in this file for debugging purpose?
Otherwise, it's better to split the interface, and give it to the UI renderer only when the debugging is enabled.
https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.hus2vlat0wvqThis comes from the section, "Scope capabilities and privileges with separate interfaces".
But, IIUC, these all are for chrome://tab-strip-internals/ and this interface is given only to renderers that host it. In such a case, please ignore my second comment above.
Regarding point 2 (splitting the interface), yes, I can confirm that all the fields in this mojom file are meant to display read-only debugging information in `chrome://tab-strip-internals`.
The overall goal of this page is to provide quick access to TabStrip and SessionRestore information to help developers debug tab behavior.
PS: I have declared the rqeuired type, please let me know if it's as per expectations.
thank you for your double check, and update the SessionID.
LGTM.
string guid;Brijesh GiriHum... It seems this is std::string also in C++ side, but can we use base/uuid.h and mojo_base.mojom.Uuid for this guid value?
Darryl JamesNeed a little help to get more insights into what this field represents and why we store this field as a string in `components/sessions/core/session_types.h` instead of a `base/uuid.h`
Is this `guid` unique across sessions?
In this one, guid is a way to associate the specific tab being worked with. It was introduced in [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/2093691).
This allows the embedder to associate and persist state with a tab. Though I wasn't able to find any callers or usages for this field. I am surprised this is even getting populated at this point in time.
My thoughts here are to leave the field in the struct that way there is a 1:1 mapping between mojom and c++, and update the comment. Separately, in the future double back and see if this field can be removed entirely if it is genuinely unused.
The plan SGTM
string title;
string workspace;Brijesh Giriditto, detailed comments for string uses.
Takashi ToyoshimaPlease let me know if the provide comment is as per expectations.
Acknowledged
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. |