Support contained tabs session restore and sync.#33313
Support contained tabs session restore and sync.#33313
Conversation
2242590 to
69e5237
Compare
fb2f727 to
6247a7a
Compare
69e5237 to
6af0f4f
Compare
|
Chromium major version is behind target branch (144.0.7559.110 vs 145.0.7632.31). Please rebase. |
6af0f4f to
2571210
Compare
9de458a to
48c57a0
Compare
c74e22c to
5b4e565
Compare
|
For reviewers: please refer to a short doc that explains what's going on here in more details https://github.com/brave/brave-core/blob/containers-session-restore-sync/docs/containers/session_persistence.md |
|
Sorry for late - I'll review tomorrow. |
simonhong
left a comment
There was a problem hiding this comment.
++ with some trivial question. 👍🏼
chromium_src/components/sessions/content/content_serialized_navigation_driver.cc
Show resolved
Hide resolved
patches/chrome-browser-sessions-chrome_serialized_navigation_driver.cc.patch
Show resolved
Hide resolved
5b4e565 to
053645b
Compare
fmarier
left a comment
There was a problem hiding this comment.
I read through the design doc and skimmed through the changes. The approach sounds right to me.
One minor thing I'd suggest testing is restoring/syncing URLs that already have a virtual scheme (e.g. view-source:https://example.com) just to make sure the view-source: doesn't get lost in the serialization/deserialization process.
netzenbot
left a comment
There was a problem hiding this comment.
Review via brave-core-bot
chromium_src/third_party/blink/public/common/page_state/page_state.h
Outdated
Show resolved
Hide resolved
components/containers/content/browser/storage_partition_utils.cc
Outdated
Show resolved
Hide resolved
|
Chromium major version is behind target branch (145.0.7632.109 vs 146.0.7680.32). Please rebase. |
bsclifton
left a comment
There was a problem hiding this comment.
Tested it out; works great! Used the doc to try some of the tests. Turned on, loaded tabs into container. Turned off containers, made sure it uses the special prefix.
Left nit about one thing. Code LGTM! 😎👍
053645b to
6ae80ad
Compare
|
[puLL-Merge] - brave/brave-core@33313 DescriptionThis PR implements session persistence for Brave Containers, allowing container tabs to be correctly restored after browser restart, "Reopen closed tab", and via Sync. The core approach encodes the container's Possible Issues
Security Hotspots
ChangesChanges
Patches (6 new patch files)
sequenceDiagram
participant Tab as Container Tab
participant NE as NavigationEntry
participant Builder as ContentSerializedNavigationBuilder
participant Driver as ContentSerializedNavigationDriver
participant SNE as SerializedNavigationEntry
participant Disk as Disk/Sync
participant Restore as Tab Restore
Note over Tab,Disk: Serialization (Save)
Tab->>NE: Has SiteInstance with StoragePartitionConfig
NE->>Builder: FromNavigationEntry()
Builder->>NE: GetStoragePartitionKeyToRestore()
NE-->>Builder: {"containers", "<uuid>"}
Builder->>SNE: set_virtual_url_prefix("containers+<uuid>:")
Builder->>SNE: set_storage_partition_key({"containers","<uuid>"})
SNE->>Driver: GetSanitizedPageStateForPickle()
Driver->>SNE: PrefixTopURL("containers+<uuid>:")
SNE->>Disk: WriteToPickle() with prefixed virtual_url
Note over Disk,Tab: Deserialization (Restore)
Disk->>SNE: Read from disk/sync
SNE->>Driver: Sanitize()
Driver->>Driver: RestoreStoragePartitionKeyFromUrl()
Driver->>SNE: set_virtual_url(original URL)
Driver->>SNE: set_storage_partition_key({"containers","<uuid>"})
Driver->>SNE: RemoveTopURLPrefix from PageState
SNE->>Builder: ToNavigationEntry()
Builder->>NE: SetStoragePartitionKeyToRestore()
NE->>Restore: GetStoragePartitionConfigToRestore()
Restore->>Tab: Create tab with correct StoragePartitionConfig
|
| return *this; | ||
| } | ||
|
|
||
| state.top.url_string = state.top.url_string->substr(prefix_length); |
There was a problem hiding this comment.
RemoveTopURLPrefix dereferences state.top.url_string without a null check. PrefixTopURL correctly guards with if (!state.top.url_string) { return *this; } — this method should do the same, since it processes deserialized PageState that may have a null URL. best practice
Implement session persistence for Containers.
Containers use
StoragePartitionConfigfor data isolation, but the config information wasn't persisted across sessions, causing tabs to lose isolation on restore.This change encodes
StoragePartitionConfiginto URLs during serialization using a virtual prefix:containers+<uuid>:<actual_url>.On serialization, the prefix is added to both
SerializedNavigationEntryandPageState. On deserialization,StoragePartitionConfigis restored and the prefix removed fromPageState(when feature is enabled).When the feature is disabled, the prefix remains in
PageState. Blink attempts to navigate to the unhandleablecontainers+uuid:scheme, resulting in a blank page rather than loading in the wrong partition.This works with existing session/sync infrastructure and enables container tabs to survive browser restart, crash recovery, "reopen closed tab", and cross-device sync while maintaining storage isolation.
Please refer to the documentation https://github.com/brave/brave-core/blob/containers-session-restore-sync/docs/containers/session_persistence.md
Resolves brave/brave-browser#47306
Resolves brave/brave-browser#46353