-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Session storage and changing browsing contexts #119
Comments
I tend to agree with your assessment, but I would love to hear from @asutherland and maybe @janvarga about sharing |
I think there'd be a lot of value in something like session-idb, and yeah, it wouldn't clone when opening auxiliary browsing instances (although the "duplicate tab" feature might). |
The chrome implementation of session storage today does rely in a number of ways on there always being at most one renderer process accessing a particular session storage. It's been a while since I looked at/touched that code, but I think this made it much easier to ensure we always dispatch the right events even in the presence of copy-on-write behavior. So while supporting sharing one session storage across multiple renderer processes is probably possible, it would certainly be a non-trivial effort. |
Having said that, it doesn't sound like case 1) would actually involve two processes accessing the same storage at the same time, so it would still be okay. 2) might be more problematic if that does actually let two processes access the same storage at the same time. My understanding was that at some point portals was considering doing the same, but ended up deciding to block storage before activation instead (partially to solve this problem). |
Firefox's SessionStorage implementation currently assumes only a single content process (akin to renderer) will actively be using the storage for a given (Top level browsing context (session) id, origin) tuple. It's expected and handled that as history is traversed that this will result in this notional active process changing. In the event the concurrent access assumption is violated, each content process will effectively fork the state on first SessionStorage access and live in their own universes, never to perceive the canonical state that a new content process would see. If needed, this could instead be made to converge with the current LocalStorage NextGen implementation wherein multiple processes can access the same data with a consistent snapshot established for the duration of a task the first time LS is accessed within an task, with atomic write-back of any deltas occurring at the end of the task. In the event of a race, mutation batches will stack and all processes will perceive the canonical state during the next task. Sync-ish IPC is sadly involved. In general it would be nice if the single content/renderer process invariant can be maintained. Which maybe is to say that a SessionStorage storage bottle can only be accessed from one agent cluster at a time? In cases where that would be violated, we must clone/copy/stick-in-the-replicator the bottle or create a new empty bottle. It sounds like that's what's being proposed here? |
I think we're actually fine and I slightly misunderstood the implications of something Jake said. When you navigate from origin A without COOP to origin A with COOP there is a process swap, but it's not the case that both would have access to the same |
Yeah it's the prerender case where access can happen simultaneously. |
Would a "clone & swap" system like this be easier than truly shared storage @mkruisselbrink @asutherland?
This means that clicking back would take you to the previous page without changing session storage. |
Are the modifications performed by the pre-rendering page to sessionStorage discarded by the bottle transplant? Or is step 4 more than bringing the pre-render up-to-speed with the changes in the old top-level-page and implies also applying the changes made by the pre-render? |
Ignore the specifics of step 4 for now, I'm getting ahead of myself.
Yes, discarded, although some event may provide insight into which keys changed as a result of the transplant, and provide their old value. So:
If prerender becomes the main page, it's session storage will be replaced with the main page's storage, and (maybe) it gets an event that tells it:
I don't know if this is a good idea yet, but I'm trying to get a sense for how complex it is vs making session storage 'work' across processes. |
I asked folks on Twitter how they were using session storage, here's an analysis of how prerendering could disrupt that, and how I landed on the above idea https://docs.google.com/document/d/1cCTBZR6nWsVC2TlQ8PBse7eBb4ro0rtPJxX0zCou1lw. |
Thank you for the clarification and excellent example and summary! Also, thank you very much for creating and linking the amazing document in which you've gathered use-cases and analyzed the impacts of the different solutions on the use-cases! I think you make a great case for "clone & swap". In particular, I think it:
|
Cheers! There might still be a race between what the script in the main page sets in session storage before it's cloned for the prerender (edit: although that will be resolved at swap time). The "empty" solution has fewer races, but means the prerender is more likely to be 'correct', and fetch meaningful data. |
I've been thinking about the event a bit more, and I don't think we need it. In fact, if "clone & swap" ends up being a stopgap before a shared model, we don't really want a bespoke event that might be useless in future. There are a few ways for a page to become active 'later':
And in all cases, you need to look at session storage if you have state there. Firefox and Safari do not queue storage events for bfcached pages (Chrome's experimental implementation does, but is what what we want @altimin?). Maybe the best rule for developers is to check session storage for changes after The only sharp edge is that the |
Firefox's storage implementation originally intended to queue storage events but the naive implementation logic for that never actually worked, just temporarily consumed memory as it buffered the events without coalescing, so we removed it in a cleanup. Defining "pageshow" to mean "welcome to the future! everything has changed! reconsider everything!" is very appealing as an implementer, but if you're going to expose SessionStorage to a pre-rendered page at all, maybe it's better to instead generate synthetic StorageEvent events with the constraint that |
I'm worried that, in step 3, the view of storage is the final state, rather than the state between updating key 'foo' and '123'. Or does that already happen? |
I believe that already happens. The current spec mutates the single map and calls broadcast which queues a task. This works in a straightforward fashion under a single-process model assuming all windows run on the same event loop/main thread; the current state of the map always reflects the JS code that has executed and the events are after-the-fact delta notifications. We don't specify what happens under a multi-process model; I think this would be the first time we're addressing it. Practically speaking, this can be very implementation dependent. On Firefox Release, Legacy LocalStorage processes storage broadcasts as they are received via IPC and updates the underlying (local process) map just before dispatching the events so content is perceiving a replay of what happened in the other process like the events had been dispatched synchronously instead of asynchronously via a task. On Firefox Nightly, LocalStorage NextGen approximates the single-process situation wherein a (coherent snapshot of) the single backing map will be seen that is entirely up-to-date when the event is dispatched / perceived. |
I could have sworn I left a comment earlier, but seems I didn't. This does seem like a pretty reasonable proposal, yes. I do share @jakearchibald's concerns about storage events happening when they don't match the view of storage though. Currently at least in Chrome we try pretty hard to guarantee that the view of storage always matches what events are dispatched. I wonder if "everything has changed" wouldn't be simpler from a website point of view either... (Not sure what we do today with Local/SessionStorage and bfcache in chrome, I think we just keep queueing up events, perhaps eventually evicting the page from bfcache if there are too many changes? Not sure). |
Currently there is an issue that the Session Storage is not carried over to the prerendering page. This is because a new Session Storage Namespace is used for the prerendering page. To fix this issue, this CL changes PrerenderHost::PageHolder to copy the Session Storage Namespace from the initiator page to the prerendering page. We don’t want the Session Storage state in the storage service be updated by the prerendering page. And we want to synchronize the Session Storage state of the prerendering page with the initiator page when the prerendering page is activated. So this CL introduces a flag |is_session_storage_for_prerendering_| in CachedStorageArea, and make CachedStorageArea not to send the changes of the Session Storage state to the storage service, and make StorageArea recreate |cached_area_| when the prerendering page is activated. This is the "clone & swap" mechanism for session storage in prerendering described in whatwg/storage#119. This CL still has an issue that when the initial renderer process is reused after the back navigation from a prerendered page, the Session Storage state is not correctly propagated to the initial renderer process. This issue will be fixed in the next CL. https://crrev.com/c/2849654 Bug: 1197383 Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#881985}
This reverts commit eefb8c5. Reason for revert: PrerenderBackForwardCacheBrowserTest.SessionStorageAfterBackNavigation flaky on multiple bots. Example: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-ubsan-vptr/4247/test-results Original change's description: > Use the same SessionStorageNamespace for prerendering > > Currently there is an issue that the Session Storage is not carried over > to the prerendering page. This is because a new Session Storage > Namespace is used for the prerendering page. > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > Session Storage Namespace from the initiator page to the prerendering > page. > > We don’t want the Session Storage state in the storage service be > updated by the prerendering page. And we want to synchronize the Session > Storage state of the prerendering page with the initiator page when the > prerendering page is activated. So this CL introduces a flag > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > CachedStorageArea not to send the changes of the Session Storage state > to the storage service, and make StorageArea recreate |cached_area_| > when the prerendering page is activated. > > This is the "clone & swap" mechanism for session storage in prerendering > described in whatwg/storage#119. > > This CL still has an issue that when the initial renderer process is > reused after the back navigation from a prerendered page, the Session > Storage state is not correctly propagated to the initial renderer > process. This issue will be fixed in the next CL. > https://crrev.com/c/2849654 > > Bug: 1197383 > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#881985} Bug: 1197383 Change-Id: Iff45106b5e3f5d6f9b2c71f9273769cf93ff48c5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891974 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Owen Min <zmin@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Auto-Submit: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#882084}
This is a reland of eefb8c5 The original CL was reverted because the PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check the behavior of BackForwardCache. It was just running the same tests of PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial process have been killed before the back navigation, the test failed. To make the BackForwardCache logic work this CL changed the browser test as followings: - Added enable_same_site flag. - Stopped using BroadcastChannel which prevent BFCache. PS1 is the same as the original CL. Original change's description: > Use the same SessionStorageNamespace for prerendering > > Currently there is an issue that the Session Storage is not carried over > to the prerendering page. This is because a new Session Storage > Namespace is used for the prerendering page. > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > Session Storage Namespace from the initiator page to the prerendering > page. > > We don’t want the Session Storage state in the storage service be > updated by the prerendering page. And we want to synchronize the Session > Storage state of the prerendering page with the initiator page when the > prerendering page is activated. So this CL introduces a flag > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > CachedStorageArea not to send the changes of the Session Storage state > to the storage service, and make StorageArea recreate |cached_area_| > when the prerendering page is activated. > > This is the "clone & swap" mechanism for session storage in prerendering > described in whatwg/storage#119. > > This CL still has an issue that when the initial renderer process is > reused after the back navigation from a prerendered page, the Session > Storage state is not correctly propagated to the initial renderer > process. This issue will be fixed in the next CL. > https://crrev.com/c/2849654 > > Bug: 1197383 > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#881985} Bug: 1197383 Change-Id: If83c11d44e37b598111ab1c5ce4a78dfd3757176 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#883819}
This reverts commit c226ef4. Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing on linux-chromeos-chrome since first run with this CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 Original change's description: > Reland "Use the same SessionStorageNamespace for prerendering" > > This is a reland of eefb8c5 > > The original CL was reverted because the > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > the behavior of BackForwardCache. It was just running the same tests of > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > process have been killed before the back navigation, the test failed. > > To make the BackForwardCache logic work this CL changed the browser test > as followings: > - Added enable_same_site flag. > - Stopped using BroadcastChannel which prevent BFCache. > > PS1 is the same as the original CL. > > > Original change's description: > > Use the same SessionStorageNamespace for prerendering > > > > Currently there is an issue that the Session Storage is not carried over > > to the prerendering page. This is because a new Session Storage > > Namespace is used for the prerendering page. > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > Session Storage Namespace from the initiator page to the prerendering > > page. > > > > We don’t want the Session Storage state in the storage service be > > updated by the prerendering page. And we want to synchronize the Session > > Storage state of the prerendering page with the initiator page when the > > prerendering page is activated. So this CL introduces a flag > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > CachedStorageArea not to send the changes of the Session Storage state > > to the storage service, and make StorageArea recreate |cached_area_| > > when the prerendering page is activated. > > > > This is the "clone & swap" mechanism for session storage in prerendering > > described in whatwg/storage#119. > > > > This CL still has an issue that when the initial renderer process is > > reused after the back navigation from a prerendered page, the Session > > Storage state is not correctly propagated to the initial renderer > > process. This issue will be fixed in the next CL. > > https://crrev.com/c/2849654 > > > > Bug: 1197383 > > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#881985} > > Bug: 1197383 > Change-Id: If83c11d44e37b598111ab1c5ce4a78dfd3757176 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 > Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Fergal Daly <fergal@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#883819} Bug: 1197383 Change-Id: I8e506a142374fa10c3f9d475bf24d7ba78af93fc No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2903294 Reviewed-by: Gayane Petrosyan <gayane@chromium.org> Commit-Queue: Gayane Petrosyan <gayane@chromium.org> Auto-Submit: Nate Chapin <japhet@chromium.org> Owners-Override: Nate Chapin <japhet@chromium.org> Cr-Commit-Position: refs/heads/master@{#884008}
Chromium change: https://source.chromium.org/chromium/chromium/src/+/f33e440034f2ff39062fd6e834acf2babc6871a5 commit f33e440034f2ff39062fd6e834acf2babc6871a5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed May 19 05:29:49 2021 +0000 Reland "Reland "Use the same SessionStorageNamespace for prerendering"" This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing n linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383
Chromium change: https://source.chromium.org/chromium/chromium/src/+/f33e440034f2ff39062fd6e834acf2babc6871a5 commit f33e440034f2ff39062fd6e834acf2babc6871a5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed May 19 05:29:49 2021 +0000 Reland "Reland "Use the same SessionStorageNamespace for prerendering"" This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing n linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383
Chromium change: https://source.chromium.org/chromium/chromium/src/+/f33e440034f2ff39062fd6e834acf2babc6871a5 commit f33e440034f2ff39062fd6e834acf2babc6871a5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed May 19 05:29:49 2021 +0000 Reland "Reland "Use the same SessionStorageNamespace for prerendering"" This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing n linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383
Chromium change: https://source.chromium.org/chromium/chromium/src/+/f33e440034f2ff39062fd6e834acf2babc6871a5 commit f33e440034f2ff39062fd6e834acf2babc6871a5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed May 19 05:29:49 2021 +0000 Reland "Reland "Use the same SessionStorageNamespace for prerendering"" This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing n linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383
Chromium change: https://source.chromium.org/chromium/chromium/src/+/f33e440034f2ff39062fd6e834acf2babc6871a5 commit f33e440034f2ff39062fd6e834acf2babc6871a5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed May 19 05:29:49 2021 +0000 Reland "Reland "Use the same SessionStorageNamespace for prerendering"" This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing n linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383
Draft WPT: web-platform-tests/wpt#31080. |
Bug: 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Bug: 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should be fired for the page after becoming active. Results: Chromium: Pass. Firefox/Safari: Fail (storage events are not fired when localStorage is modified when a page is in BFCache) Bug: 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should be fired for the page after becoming active. Results: Chromium: Pass. Firefox/Safari: Fail (storage events are not fired when localStorage is modified when a page is in BFCache) Bug: 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Hi, I'm planning to merge web-platform-tests/wpt#31080 and wondering what is the expected behavior around BFCache: should storage events for storage modification while a page is in BFCache be fired after the page is restored (Chrome's behavior), or not (Safari/Firefox's behavior)? |
Friendly ping; any opinion about whether we should queue storage events during BFCache or not? |
Above @asutherland argued for dispatching a sequence of events for "dirty keys" (with |
As an outsider, @annevk's proposal in #119 (comment) makes sense to me to balance between supporting current usage of the API and not being too much of a burden on the implementation side. Sites can always proactively check for things on "pageshow" too. @mkruisselbrink @jakearchibald please shout if there are problems with this approach! |
OK, the concensus here is #119 (comment). |
Expectation: When localStorage is modified when a page is in BFCache, storage events should be fired for the page after becoming active. Results: Chromium: Pass. Firefox/Safari: Fail (storage events are not fired when localStorage is modified when a page is in BFCache) Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3198458 Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009350}
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3198458 Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009350}
Automatic update from web-platform-tests [WPT] BFCache: storage events Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3198458 Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009350} -- wpt-commits: 337a7a29b66978b71d7e6beacaaa281162a978cf wpt-pr: 31080
Currently there is an issue that the Session Storage is not carried over to the prerendering page. This is because a new Session Storage Namespace is used for the prerendering page. To fix this issue, this CL changes PrerenderHost::PageHolder to copy the Session Storage Namespace from the initiator page to the prerendering page. We don’t want the Session Storage state in the storage service be updated by the prerendering page. And we want to synchronize the Session Storage state of the prerendering page with the initiator page when the prerendering page is activated. So this CL introduces a flag |is_session_storage_for_prerendering_| in CachedStorageArea, and make CachedStorageArea not to send the changes of the Session Storage state to the storage service, and make StorageArea recreate |cached_area_| when the prerendering page is activated. This is the "clone & swap" mechanism for session storage in prerendering described in whatwg/storage#119. This CL still has an issue that when the initial renderer process is reused after the back navigation from a prerendered page, the Session Storage state is not correctly propagated to the initial renderer process. This issue will be fixed in the next CL. https://crrev.com/c/2849654 Bug: 1197383 Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#881985} NOKEYCHECK=True GitOrigin-RevId: eefb8c561ab863863b0541125df363fef040eabb
This reverts commit eefb8c561ab863863b0541125df363fef040eabb. Reason for revert: PrerenderBackForwardCacheBrowserTest.SessionStorageAfterBackNavigation flaky on multiple bots. Example: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-ubsan-vptr/4247/test-results Original change's description: > Use the same SessionStorageNamespace for prerendering > > Currently there is an issue that the Session Storage is not carried over > to the prerendering page. This is because a new Session Storage > Namespace is used for the prerendering page. > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > Session Storage Namespace from the initiator page to the prerendering > page. > > We don’t want the Session Storage state in the storage service be > updated by the prerendering page. And we want to synchronize the Session > Storage state of the prerendering page with the initiator page when the > prerendering page is activated. So this CL introduces a flag > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > CachedStorageArea not to send the changes of the Session Storage state > to the storage service, and make StorageArea recreate |cached_area_| > when the prerendering page is activated. > > This is the "clone & swap" mechanism for session storage in prerendering > described in whatwg/storage#119. > > This CL still has an issue that when the initial renderer process is > reused after the back navigation from a prerendered page, the Session > Storage state is not correctly propagated to the initial renderer > process. This issue will be fixed in the next CL. > https://crrev.com/c/2849654 > > Bug: 1197383 > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#881985} Bug: 1197383 Change-Id: Iff45106b5e3f5d6f9b2c71f9273769cf93ff48c5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891974 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Owen Min <zmin@chromium.org> Commit-Queue: Owen Min <zmin@chromium.org> Auto-Submit: Owen Min <zmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#882084} NOKEYCHECK=True GitOrigin-RevId: 1fa2fab156f293450721192b2376b5ac8f2a6a2e
This is a reland of eefb8c561ab863863b0541125df363fef040eabb The original CL was reverted because the PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check the behavior of BackForwardCache. It was just running the same tests of PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial process have been killed before the back navigation, the test failed. To make the BackForwardCache logic work this CL changed the browser test as followings: - Added enable_same_site flag. - Stopped using BroadcastChannel which prevent BFCache. PS1 is the same as the original CL. Original change's description: > Use the same SessionStorageNamespace for prerendering > > Currently there is an issue that the Session Storage is not carried over > to the prerendering page. This is because a new Session Storage > Namespace is used for the prerendering page. > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > Session Storage Namespace from the initiator page to the prerendering > page. > > We don’t want the Session Storage state in the storage service be > updated by the prerendering page. And we want to synchronize the Session > Storage state of the prerendering page with the initiator page when the > prerendering page is activated. So this CL introduces a flag > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > CachedStorageArea not to send the changes of the Session Storage state > to the storage service, and make StorageArea recreate |cached_area_| > when the prerendering page is activated. > > This is the "clone & swap" mechanism for session storage in prerendering > described in whatwg/storage#119. > > This CL still has an issue that when the initial renderer process is > reused after the back navigation from a prerendered page, the Session > Storage state is not correctly propagated to the initial renderer > process. This issue will be fixed in the next CL. > https://crrev.com/c/2849654 > > Bug: 1197383 > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#881985} Bug: 1197383 Change-Id: If83c11d44e37b598111ab1c5ce4a78dfd3757176 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#883819} NOKEYCHECK=True GitOrigin-RevId: c226ef4e5aaa66edbf29db3239e91bd49bcac2d2
This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing on linux-chromeos-chrome since first run with this CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 Original change's description: > Reland "Use the same SessionStorageNamespace for prerendering" > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > The original CL was reverted because the > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > the behavior of BackForwardCache. It was just running the same tests of > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > process have been killed before the back navigation, the test failed. > > To make the BackForwardCache logic work this CL changed the browser test > as followings: > - Added enable_same_site flag. > - Stopped using BroadcastChannel which prevent BFCache. > > PS1 is the same as the original CL. > > > Original change's description: > > Use the same SessionStorageNamespace for prerendering > > > > Currently there is an issue that the Session Storage is not carried over > > to the prerendering page. This is because a new Session Storage > > Namespace is used for the prerendering page. > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > Session Storage Namespace from the initiator page to the prerendering > > page. > > > > We don’t want the Session Storage state in the storage service be > > updated by the prerendering page. And we want to synchronize the Session > > Storage state of the prerendering page with the initiator page when the > > prerendering page is activated. So this CL introduces a flag > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > CachedStorageArea not to send the changes of the Session Storage state > > to the storage service, and make StorageArea recreate |cached_area_| > > when the prerendering page is activated. > > > > This is the "clone & swap" mechanism for session storage in prerendering > > described in whatwg/storage#119. > > > > This CL still has an issue that when the initial renderer process is > > reused after the back navigation from a prerendered page, the Session > > Storage state is not correctly propagated to the initial renderer > > process. This issue will be fixed in the next CL. > > https://crrev.com/c/2849654 > > > > Bug: 1197383 > > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#881985} > > Bug: 1197383 > Change-Id: If83c11d44e37b598111ab1c5ce4a78dfd3757176 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 > Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Fergal Daly <fergal@chromium.org> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#883819} Bug: 1197383 Change-Id: I8e506a142374fa10c3f9d475bf24d7ba78af93fc No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2903294 Reviewed-by: Gayane Petrosyan <gayane@chromium.org> Commit-Queue: Gayane Petrosyan <gayane@chromium.org> Auto-Submit: Nate Chapin <japhet@chromium.org> Owners-Override: Nate Chapin <japhet@chromium.org> Cr-Commit-Position: refs/heads/master@{#884008} NOKEYCHECK=True GitOrigin-RevId: a0159268472a7ae35a8573518aacad4e4f758b12
This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > Revert "Reland "Use the same SessionStorageNamespace for prerendering"" > > This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. > > Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing on linux-chromeos-chrome since first run with this > CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 > > Original change's description: > > Reland "Use the same SessionStorageNamespace for prerendering" > > > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > > > The original CL was reverted because the > > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > > the behavior of BackForwardCache. It was just running the same tests of > > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > > process have been killed before the back navigation, the test failed. > > > > To make the BackForwardCache logic work this CL changed the browser test > > as followings: > > - Added enable_same_site flag. > > - Stopped using BroadcastChannel which prevent BFCache. > > > > PS1 is the same as the original CL. > > > > > > Original change's description: > > > Use the same SessionStorageNamespace for prerendering > > > > > > Currently there is an issue that the Session Storage is not carried over > > > to the prerendering page. This is because a new Session Storage > > > Namespace is used for the prerendering page. > > > > > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > > > Session Storage Namespace from the initiator page to the prerendering > > > page. > > > > > > We don’t want the Session Storage state in the storage service be > > > updated by the prerendering page. And we want to synchronize the Session > > > Storage state of the prerendering page with the initiator page when the > > > prerendering page is activated. So this CL introduces a flag > > > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > > > CachedStorageArea not to send the changes of the Session Storage state > > > to the storage service, and make StorageArea recreate |cached_area_| > > > when the prerendering page is activated. > > > > > > This is the "clone & swap" mechanism for session storage in prerendering > > > described in whatwg/storage#119. > > > > > > This CL still has an issue that when the initial renderer process is > > > reused after the back navigation from a prerendered page, the Session > > > Storage state is not correctly propagated to the initial renderer > > > process. This issue will be fixed in the next CL. > > > https://crrev.com/c/2849654 > > > > > > Bug: 1197383 > > > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > > > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > > > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#881985} > > > > Bug: 1197383 > > Change-Id: If83c11d44e37b598111ab1c5ce4a78dfd3757176 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 > > Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Reviewed-by: Fergal Daly <fergal@chromium.org> > > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#883819} > > Bug: 1197383 > Change-Id: I8e506a142374fa10c3f9d475bf24d7ba78af93fc > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2903294 > Reviewed-by: Gayane Petrosyan <gayane@chromium.org> > Commit-Queue: Gayane Petrosyan <gayane@chromium.org> > Auto-Submit: Nate Chapin <japhet@chromium.org> > Owners-Override: Nate Chapin <japhet@chromium.org> > Cr-Commit-Position: refs/heads/master@{#884008} Bug: 1197383 Change-Id: Icb2d0b9362904d404f4bca886e99731e75df4a99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2905112 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#884325} NOKEYCHECK=True GitOrigin-RevId: f33e440034f2ff39062fd6e834acf2babc6871a5
Expectation: When localStorage is modified when a page is in BFCache, storage events should NOT be fired for the page even after the page becomes active. Results: Firefox/Safari: Pass. Chromium: Fail (events are fired, https://crbug.com/1328939). Bug: 1328939, 1107415, whatwg/storage#119 Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3198458 Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1009350} NOKEYCHECK=True GitOrigin-RevId: 0b51134a957749ed6f384b1c04671c5b0478d3de
I'm trying to figure out how session storage should behave in a few similar cases.
I think the possible options for each of these are:
a) The new page has fresh, empty session storage
b) The new page has the same session storage
c) The new page has a clone of session storage
…where b is generally what happens while navigating around in the same tab, and c happens when opening a popup.
References to "isolated" here mean
Cross-Origin-Opener-Policy: same-origin
.1. User navigates from non-isolated pageA to isolated pageB in the same tab. My gut feeling is that "b" is ok here. Only visible data would be able to pass from pageA to pageB. This may be the first time a single unit of session storage has been shared between a (process, origin) pair, although it's pretty common with localstorage.
2. pageA performs a
<link rel="prerender">
of pageB. User navigates from pageA to pageB in the same tab and the prerender is used. A prerender will exist in a new browsing context, so this is similar to the previous case, and imo have the same result, "b". The difference is, in the same-origin case, it's more likely that each process will be accessing session storage at the same time.3. User navigates from pageA to pageB by changing the URL in the URL bar. This case feels different to me. Since the navigation hasn't been caused by web content within the session, "a" feels more appropriate. However, if the user clicks back, the previous session storage should be present.
4. Non-isolated pageA opens a popup to isolated pageB. Again, it seems ok to continue with the current behaviour, which is "c". That seems to be what Chrome does now.
@annevk @rakina does the above make sense?
If so, I think I'll change what I'm currently calling "browsing session" to "top level navigable" (making a special kind of navigable), since it's supposed to represent a window/tab. Then I'll give browsing contexts a session bucket/ID. Multiple browsing contexts can share the same session bucket/ID, and a top level navigable may contain history entries that have browsing sessions with different session bucket/IDs.
This would also allow us to add logic that prevents
history.back()
traversing to a history entry with a different session than the current entry, although the browser back button would not have this restriction.The text was updated successfully, but these errors were encountered: