Skip to content
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

Open
jakearchibald opened this issue Jan 28, 2021 · 25 comments
Open

Session storage and changing browsing contexts #119

jakearchibald opened this issue Jan 28, 2021 · 25 comments

Comments

@jakearchibald
Copy link
Contributor

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.

@annevk
Copy link
Member

annevk commented Jan 28, 2021

I tend to agree with your assessment, but I would love to hear from @asutherland and maybe @janvarga about sharing sessionStorage across the process boundary. If sessionStorage has a different storage architecture from localStorage I wonder if it's worth salvaging or whether sessions should be reintroduced as part of buckets (and not have the problematic aspects there, such as cloning).

@jakearchibald
Copy link
Contributor Author

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).

@mkruisselbrink
Copy link

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.

@mkruisselbrink
Copy link

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).

@asutherland
Copy link
Collaborator

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?

@annevk
Copy link
Member

annevk commented Jan 30, 2021

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 sessionStorage simultaneously.

@jakearchibald
Copy link
Contributor Author

Yeah it's the prerender case where access can happen simultaneously.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Feb 23, 2021

Would a "clone & swap" system like this be easier than truly shared storage @mkruisselbrink @asutherland?

  1. A top-level page contain a same-origin prerender. The same-origin prerendered page receives a clone of session storage.
  2. Wait until the prerendered page activates.
  3. Switch the session storage of the now-activated prerendered page from its clone to the session storage used by the old top-level page.
  4. Fire "storage" events for every difference between the old cloned storage and the new storage.

This means that clicking back would take you to the previous page without changing session storage.

@asutherland
Copy link
Collaborator

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?

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Feb 24, 2021

Ignore the specifics of step 4 for now, I'm getting ahead of myself.

Are the modifications performed by the pre-rendering page to sessionStorage discarded by the bottle transplant?

Yes, discarded, although some event may provide insight into which keys changed as a result of the transplant, and provide their old value.

So:

  1. Main page has session storage { foo: 'bar', hello: 'world' }.
  2. Prerender created, which has a clone of session storage, so { foo: 'bar', hello: 'world' }.
  3. Main page sets 'username' to 'sarah' in session storage, so { foo: 'bar', hello: 'world', username: 'sarah' }.
  4. Prerender sets 'foo' to 'yo', and '123' to '456', so { foo: 'yo', hello: 'world', '123': '456' }.

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:

  • 'foo' changed, was 'yo'
  • 'username' changed, was undefined
  • '123' changed, was '456'

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.

@jakearchibald
Copy link
Contributor Author

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.

@asutherland
Copy link
Collaborator

asutherland commented Feb 24, 2021

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:

  • Maximizes web-compat by being deterministic instead of allowing races that will fundamentally depend on browser implementation details.
    • As a direct consequence, this should also simplify future browser implementation. That said, the fact that LocalStorage potentially involves races means that browsers might have to deal with implementing "shared" anyways.
  • Maximizes the ability of web developers to reason about what's going on by being deterministic and not involving races.
    • As per the above, LocalStorage will still be a problem here for developers (because races will continue be inherent), but their experience with StorageEvents across LocalStorage and SessionStorage should help their logic act consistently in both cases.
    • I could see developers expecting that more of a "merge" would happen rather than than the swap/replacement, but it seems appropriate for the window with the user interaction to "win"; SessionStorage should be reflecting user actions and intent and any mutations the pre-rendered window would perform would by definition not be as a result of direct user action (in the page).
  • Avoids the potential need for synchronous IPC in the implementation.
  • Could be relaxed to a "shared" model in the future if desired without regressing sites that properly handle storage events.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Feb 24, 2021

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.

@jakearchibald
Copy link
Contributor Author

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':

  • Prerender
  • Portals
  • Bfcache

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 'pageshow'? For prerender, we can make sure the storage is swapped before that event.

The only sharp edge is that the 'pageshow' event fires after 'load' for some reason. We'd need to make sure there's an easy way to ignore the 'pageshow' event for a page that wasn't 'hidden'.

@asutherland
Copy link
Collaborator

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 oldValue will not be present. So for pages that are frozen/etc., they'd just accumulate a list of dirty keys and when they become unfrozen, we fire a bunch of synthetic storage events at them. This seems friendlier to content and with reasonable memory costs since we're just storing a set of keys.

@jakearchibald
Copy link
Contributor Author

  1. Session storage is in old { foo: 'bar', hello: 'world' } state.
  2. Swap happens (or page is shown after being in bfcache), so it's now { foo: 'yo', hello: 'world', '123': '456' }.
  3. "storage" event for 'foo' key.
  4. "storage" event for '123' key.

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?

@asutherland
Copy link
Collaborator

asutherland commented Feb 25, 2021

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.

@mkruisselbrink
Copy link

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).

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue May 12, 2021
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}
DamieFC pushed a commit to DamieFC/chromium that referenced this issue May 12, 2021
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}
pull bot pushed a commit to Yannic/chromium that referenced this issue May 18, 2021
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}
DamieFC pushed a commit to DamieFC/chromium that referenced this issue May 18, 2021
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}
mkarolin added a commit to brave/brave-core that referenced this issue May 27, 2021
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
mariospr pushed a commit to brave/brave-core that referenced this issue Jun 2, 2021
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
mariospr pushed a commit to brave/brave-core that referenced this issue Jun 2, 2021
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
mariospr pushed a commit to brave/brave-core that referenced this issue Jun 4, 2021
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
mkarolin added a commit to brave/brave-core that referenced this issue Jun 7, 2021
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
mkarolin added a commit to brave/brave-core that referenced this issue Jun 7, 2021
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
mkarolin added a commit to brave/brave-core that referenced this issue Jun 29, 2021
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
@hiroshige-g
Copy link

Draft WPT: web-platform-tests/wpt#31080.
This is unstable (timeout) on wpt.fyi, but according to Chromium CI and locally installed Firefox, this confirms the behavior at #119 (comment) (Chrome fires storage events for modifications during a page is BFCached while Firefox/Safari don't).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
Bug: 1107415, whatwg/storage#119
Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2022
Bug: 1107415, whatwg/storage#119
Change-Id: I53c92b2d5f8f4791a43c2c702a441029fdbc7101
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2022
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
@hiroshige-g
Copy link

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)?
FYI @rakina @mkruisselbrink

@hiroshige-g
Copy link

Friendly ping; any opinion about whether we should queue storage events during BFCache or not?

@annevk
Copy link
Member

annevk commented May 3, 2022

Above @asutherland argued for dispatching a sequence of events for "dirty keys" (with oldValue being null). I think I personally would somewhat prefer not firing events at all and use this renewed interest in bfcache to teach web developers that this can have changed. It seems it has the potential to get rather noisy otherwise.

@asutherland
Copy link
Collaborator

I defer to @annevk in all things; my key concern in general would be that anything in BFCache should not accumulate an unbounded list of changes that could result in unbounded memory usage and @annevk's proposal does cover that.

@rakina
Copy link
Member

rakina commented May 4, 2022

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!

@hiroshige-g
Copy link

OK, the concensus here is #119 (comment).
I updated the test to expect no events are fired for storage updates while the page is in BFCache, and filed a bug against Chromium (https://crbug.com/1328939).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 24, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 24, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 26, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 27, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 27, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 31, 2022
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2022
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}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 10, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants