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
|
[puLL-Merge] - brave/brave-core@33313 DescriptionThis PR implements session persistence for Brave Containers, enabling container tabs to survive browser restarts, "Reopen closed tab" operations, and cross-device Sync. The core approach encodes the container's The encoding strategy ensures that if Containers is disabled (or on an older browser), the encoded URL is unhandleable, preventing accidental mixing of container storage with default storage. Possible Issues
Security Hotspots
ChangesChanges
Patches
sequenceDiagram
participant Tab as Container Tab
participant NE as NavigationEntry
participant Builder as ContentSerializedNavigationBuilder
participant SNE as SerializedNavigationEntry
participant Driver as ContentSerializedNavigationDriver
participant Disk as Disk/Sync
Note over Tab,Disk: Serialization (Save Session)
Tab->>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->>Driver: PrefixTopURL("containers+uuid:")
SNE->>Disk: WriteToPickle(prefix + url)
SNE->>Disk: Sync(prefix + url)
Note over Tab,Disk: Deserialization (Restore Session)
Disk->>SNE: ReadFromPickle("containers+uuid:https://...")
SNE->>Driver: Sanitize()
Driver->>Driver: RestoreStoragePartitionKeyFromUrl()
Driver->>SNE: set_virtual_url("https://...")
Driver->>SNE: set_storage_partition_key({containers, uuid})
Driver->>SNE: RemoveTopURLPrefix from PageState
SNE->>Builder: ToNavigationEntry()
Builder->>NE: SetStoragePartitionKeyToRestore({containers, uuid})
NE->>Tab: Create SiteInstance with StoragePartitionConfig
|
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
| #include "content/public/browser/browser_context.h" | ||
|
|
||
| #if BUILDFLAG(ENABLE_CONTAINERS) | ||
| #include "brave/components/containers/content/browser/storage_partition_utils.h" |
There was a problem hiding this comment.
This chromium_src file directly includes brave/components/containers/content/browser/storage_partition_utils.h and brave/components/containers/core/common/features.h. The same pattern appears in chromium_src/components/sessions/content/content_serialized_navigation_builder.cc and content_serialized_navigation_driver.cc. Per policy, chromium_src should not have GN dependencies on //brave/components/ targets. Consider using forward declarations with link-time resolution via sources.gni instead. best practice
| // container prefix, extracts the storage partition key (partition_domain, | ||
| // partition_name) and the original URL (https://example.com). | ||
| COMPONENT_EXPORT(CONTAINERS_CONTENT_BROWSER) | ||
| std::optional<GURL> RestoreStoragePartitionKeyFromUrl( |
There was a problem hiding this comment.
RestoreStoragePartitionKeyFromUrl uses out-parameters (storage_partition_key and url_prefix_length) alongside an optional<GURL> return. Consider returning a struct containing all three values instead, which eliminates the mutable reference parameters and makes the API safer and easier to use. best practice
| #define RemoveScrollOffset() \ | ||
| RemoveScrollOffset() const; \ | ||
| PageState PrefixTopURL(const std::string& prefix) const; \ | ||
| PageState RemoveTopURLPrefix(const size_t& prefix_length) |
There was a problem hiding this comment.
RemoveTopURLPrefix(const size_t& prefix_length) passes a primitive type by const reference. Pass size_t by value instead. best practice
| // Verify data is set | ||
| content::EvalJsResult cookie_result = | ||
| content::EvalJs(web_contents, GetCookiesJS()); | ||
| EXPECT_TRUE(cookie_result.ExtractString().find( |
There was a problem hiding this comment.
nit: EXPECT_TRUE(cookie_result.ExtractString().find(...) != std::string::npos) gives no diagnostic context on failure — just "expected true, got false". This pattern appears ~15 times in this file. Use EXPECT_THAT(cookies, ::testing::HasSubstr("persistent_cookie=persistent_value")) instead, which automatically includes the actual value in the failure output. best practice
| }); | ||
| } | ||
|
|
||
| std::optional<GURL> RestoreStoragePartitionKeyFromUrl( |
There was a problem hiding this comment.
I would consider making return value [[nodiscard]]. The caller would always want to be checking the value
|
|
||
| bool IsValidStoragePartitionKeyComponent(std::string_view component) { | ||
| return !component.empty() && | ||
| std::ranges::all_of(component, [](const char& c) { |
There was a problem hiding this comment.
Partition key is realistically GUID/UUIDs. But should it be allowing plain text in here too? and no minimum length?
ex: containers+lol-lol-lol:http://example.com or containers+ :http://example.com would be OK?
There was a problem hiding this comment.
I don't want to explicitly restrict things here to guid. We only care about restriction to have it serialized correctly (the emptiness check and the valid chars check). Other than that we may use some other ID syntax (prefixes for ex.) for ephemeral containers or something like that.
Maybe we can add a reasonable minimal length requirement, that makes sense I think.
| ? frame_tree_->frame_entry->site_instance() | ||
| : nullptr) { | ||
| const auto& storage_partition_config = | ||
| site_instance->GetStoragePartitionConfig(); |
There was a problem hiding this comment.
Where does the original value for a storage partition config come from?
Let's say we open a new tab in a container. Where does that UUID get generated?
There was a problem hiding this comment.
the guid is generated when you create a new container via settings page. see ContainersSettingsHandler::AddContainer.
|
Chromium major version is behind target branch (145.0.7632.109 vs 146.0.7680.32). Please rebase. |
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