Add proposal: Synchronous data at startup#793
Conversation
|
Nice to see some progress to fix this very ancient problem, thank you! I'm quite happy with this proposal, but I must signal two major problems making it unsuitable as a replacement for the current XHR work-around (one is a blocker, the second is slightly less critical):
|
|
@Rob--W Thanks for the proposal — this solves a real problem, but I strongly prefer a single Splitting into
We already enforce content script access control for There’s also no precedent for changes in one namespace triggering events in another, and introducing that pattern (e.g., Also, I support keeping |
xeenon
left a comment
There was a problem hiding this comment.
Requesting changes based on my above comment.
These two areas are not linked to each other; they are independent. One is meant to be used by content scripts, the other by the background script. I'll clarify.
The availability to content scripts is controlled through the I see the following options:
Alternative B is better than nothing, but forces extension devs to choose sync data in content scripts XOR extension-private initialization data in background scripts. Alternative C requires a bit more thought on the semantics of
Alternative D is a version of C without the need to worry about the following getting out of sync: access level, persistent flag, key-value pair. |
|
I think splitting into two storage areas would both benefit security and developer's experience. Many developers don't realize and consider that a compromised renderer may access the extension storage, and put sensitive data on it. Naming the storage "contentScriptConfig" would definitely signal developers not to. |
Changes to the API: - Drop getSync / getKeysSync; make `get` and `getKeys` sync instead. - To avoid the odd situation of being to write without reading, the `contentScriptConfig` area is now also synchronously readable by extension contexts. Updates to the proposal: - Explicitly list all exposed APIs for clarity. - Emphasize independence of storage areas for clarity. - Add expectations for flushing and method resolution behavior. - Drop "TODO" remark about potentially excluding oldValue/newValue from onChanged event as an optimization, because the consensus is to maintain the same semantics as other storage `onChanged` events. - Expand "Future work"
5149b28 to
4440fe7
Compare
|
I have updated the proposal based on previous feedback and the feedback from the WECG F2F meeting in London. There was a request from @xeenon to use one namespace instead of the proposed two. @rdcronin expressed a strong preference for keeping them separate as initially proposed. I kept them separate, with extra clarifications in the proposal. I dropped One consequence of this change is that
If anyone prefers the alternatives over the current approach, I'm willing to reconsider these. Other than the above API change, the revised proposal adds a bit more detail to leave less to the imagination:
|
rdcronin
left a comment
There was a problem hiding this comment.
Thanks, Rob! Excited to see this!
|
|
||
| ``` | ||
| // Methods available to extension contexts and content scripts: | ||
| any ConfigStorageArea.get(keyOrKeysOrObjectWithDefaults: string | string[] | null | object) |
There was a problem hiding this comment.
get() is pretty confusing on the existing StorageArea interface as a result of the various different kinds of inputs it can take. Do you think it would make sense to use this opportunity to straight this out? Especially since this is designed to be synchronous (and reads necessarily must be cheaper), it seems like we could just have a single get() call that takes a string and returns a value, and the getting-multiple-strings or getting-with-defaults cases can be handled extension side without the magic transformations we currently apply for storage.
|
|
||
| number ConfigStorageArea.QUOTA_BYTES; | ||
|
|
||
| // In content scripts, the set/remove/clear methods are unavailable: |
There was a problem hiding this comment.
Is this available in user scripts? (Conditionally, configurably, as with messaging?)
| ConfigStorageArea browser.storage.extensionConfig; | ||
|
|
||
| dictionary ConfigStorageAreaSetOptions { | ||
| persistent: boolean // default false |
There was a problem hiding this comment.
Having this be optionally-persistent means that we need to have a pass-through storage mechanism where we write values to disk (in yet-another new storage area) and also update shared memory, which is passed through to all processes. That's doable, of course, but I wonder if we would get 95% of the benefit of this API with having this be in-memory / session only.
This:
- Is simpler architecturally, and thus also provides more guarantees around consistency (less chance that the in-memory store and on-disk store get out of sync -- though, of course, they may still be out of sync between renderers during the course of a storage update)
- Incentivizes using this for lightweight data and reduces the likelihood that it would grow with stale data that wasn't collected (though this is also mitigated by an aggressive storage limit)
- Anecdotally, would be used by many of these use cases, which use it for "secrets" that are better generated at runtime rather than persisted.
Downside: Extensions that don't need dynamic / changing values would need to set this once per chrome session, possibly waking up unnecessarily at startup. It also means this probably wouldn't be available synchronous for the first content scripts that run at browser run -- but that's already not guaranteed and would also not be guaranteed with a disk-backed storage (since we wouldn't block renderer load on loading that storage, at least in Chrome).
And, of course, if we decide later we definitely do need persistence, we could always add it in later. WDYT?
| number ConfigStorageArea.QUOTA_BYTES; | ||
|
|
||
| // In content scripts, the set/remove/clear methods are unavailable: | ||
| ConfigStorageArea browser.storage.contentScriptConfig; |
There was a problem hiding this comment.
I find the "contentScriptConfig" naming to be both clunky and confusing (I'd expect this to be a configuration around a content script, which is much more what scripting.RegisteredContentScript is).
Preferred alternative: cache? Maybe storage.cache for trusted contexts; storage.untrustedCache for untrusted contexts? IMO, that leans more into the "rapid, easy-access storage that probably shouldn't be super large." (And, see also below.)
Alternative: "lite"? Don't love that one, though.
| asynchronously. | ||
|
|
||
| Updates to the storage area are propagated to all processes where a context may | ||
| exists with a need for synchronous data access. This proposal specifies the |
There was a problem hiding this comment.
| exists with a need for synchronous data access. This proposal specifies the | |
| exist with a need for synchronous data access. This proposal specifies the |
|
|
||
| By default, data only lasts for the duration of the browser session. The `set` | ||
| method has a `persistent` option that extends the lifetime past browser and | ||
| extension restarts. The data may be cleared when an extension is uninstalled. |
There was a problem hiding this comment.
| extension restarts. The data may be cleared when an extension is uninstalled. | |
| extension restarts. The data will be cleared when an extension is uninstalled. |
| Updates are asynchronous. The returned `Promise` may await writes to disk when | ||
| the persistent flag is set, but does not guarantee the delivery of data to | ||
| other processes. This ensures that non-responsive processes cannot delay the | ||
| resolution of the `Promise`. The caller resides in the extension process and | ||
| is guaranteed to receive the updated value when `get` or `getSync` are called. |
There was a problem hiding this comment.
This ensures that non-responsive processes cannot delay the resolution of the
Promise
I understand the motivation, but that also means that the extension won't know if it can expect that a message it sends to a renderer would be able to access the data it set.
Brainstorming: Do you think it would be reasonable to say that it's guaranteed that any message sent after the promise resolves would arrive after the storage is updated in the renderer? I think we can make that guarantee in Chrome -- we'd update the shared memory and any messages sent would go across the same IPC channel that the shared memory update would have already gone over.
| Each key has its own associated `persistent` flag, which affect the behavior of | ||
| the latest `set()` call, and any following `remove()` or `clear()` calls. | ||
| The `set` method can update multiple keys at once, and the `persistent` option | ||
| applies to all specified keys. To have different `persistent` flags for keys, | ||
| call the `set()` method again, with a different `persistent` option value. |
There was a problem hiding this comment.
see above about just not having "persistent", but if we do have it, we should call out the behavior of conflicting persistent values. I assume that if a mark a key as persistent, then later set it to a non-persistent value, the persistent value is wiped. That is, there can only be a single key, and the latest setting for persistent / non-persistent is used.
| - `browser.storage.extensionConfig.onChanged` | ||
| - `browser.storage.extensionConfig.QUOTA_BYTES` | ||
|
|
||
| ### Behavior |
There was a problem hiding this comment.
We should include a section on incognito.
I assume that, similar to the storage API, this does not respect incognito boundaries.
This proposal offers a mechanism for extensions to specify values that are synchronously available when a content script or background script executes
This covers the use cases of #536, #703, #501, #747, among others.