-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add new strings for notRestoredReason #11381
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
base: main
Are you sure you want to change the base?
Conversation
@domenic @smaug---- could you help to review this change? This is an addition to #9360 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@annevk could you help to take another look? Thanks! |
<dd>The <code>Document</code> was created from an HTTP response whose | ||
`<code data-x="http-cache-control">Cache-Control</code>` header included the | ||
"<code data-x="">no-store</code>" token, and it has created a <code>WebSocket</code> connection | ||
which might be used to receive sensitive information, so the page was not in a state that could be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused with this stuff when there is https://html.spec.whatwg.org/#unloading-document-cleanup-steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference between the new reason string websocket-used-with-cache-control-no-store
and the existing websocket
is that the new one also covers the case where a websocket is created and then destroyed before unloading the document.
This is needed as part of the security mitigation when enabling BFCache for page with cache-control: no store
header, because the page may receive some sensitive information from the already-closed websocket. We have discussed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smaug---- does it resolve your confusion, and could you help to review again? Thanks
Adding "agenda+" to help unblock this simple PR that has been stuck for over a month. Of course, if we can do so async, that would be even better. |
To be clear, I don't think WebKit has a strong position either way. I just chimed in because I noticed something editorially. I suspect you need @smaug---- or someone else from Mozilla to unblock this. |
Well, we need you to dismiss your "changes requested" review at least :) |
As part of the project that enables pages with Cache-control: not store header, we will evict the BFCache-eligible page if it has ever used WebSocket, WebRTC and WebTransport. These are implemented, but the not restore reason string was not specified. Currently Chromium shares the string with the ones used for active WebSocket/WebRTC/WebTransport, but it could be confusing. We are proposing to create 3 new strings for the reason introduced by the CCNS changes.
see https://github.com/fergald/explainer-bfcache-ccns/blob/main/README.md for more information.
(See WHATWG Working Mode: Changes for more details.)
/nav-history-apis.html ( diff )