1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Bob Silverberg [:bsilverberg]]

This was brought up in a comment on bug 1322060. To quote Mark Hammond:

"SessionStore also has a promiseInitialized promise that should be resolved before interacting with its API. It's not clear to me if this isn't strictly necessary in a webext world or whether the existing use of session store in ext-sessions is hiding a latent bug, but I thought it worth mentioning."

Mike de Boer confirmed that we likely need to do that, so this bug is to add code to ensure that SessionStore is initialized before we attempt to interact with it via the sessions API.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs,

Awaiting the resolution of SessionStore.promiseInitialized as the first step in each API method that needs to interact with SessionStore.
As I needed to await a promise, took the opportunity to migrate the entire file to async/await.

Review commit: https://reviewboard.mozilla.org/r/142370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142370/

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8870809 [details]
Bug 1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs,

https://reviewboard.mozilla.org/r/142370/#review146156

::: browser/components/extensions/ext-sessions.js:16
(Diff revision 1)
>                                    "resource:///modules/sessionstore/SessionStore.jsm");
>  
>  const SS_ON_CLOSED_OBJECTS_CHANGED = "sessionstore-closed-objects-changed";
>  
> +function promiseSessionStoreInitialized() {
> +  return SessionStore.promiseInitialized;

Why not just use SessionStore.promiseInitialized directly?

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8870809 [details]
Bug 1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs,

https://reviewboard.mozilla.org/r/142370/#review146156

> Why not just use SessionStore.promiseInitialized directly?

I did this because we are calling it in a number of places so:

a) I thought it would saves a bit of code from each call, although because of the long function name it's actually more code :P
b) We are encapsulating the code that waits for SessionStore to be initialized in this `promiseSessionStoreInitialized` function. In the event that the code we need to call to do that changes, we'd only have to change this function, and not the 4 places from whence it's called.

Honestly it was just a reflex when faced with calling the same code multiple times, and I am happy to change it, but I still prefer it the way I wrote it. Let me know where you stand on that.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8870809 [details]
Bug 1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs,

https://reviewboard.mozilla.org/r/142370/#review146552

r+ with last comment addressed.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8870809 [details]
Bug 1365562 - Ensure that SessionStore is initialized before any interaction via WebExtension APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142370/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/715ae99bc126
Ensure that SessionStore is initialized before any interaction via WebExtension APIs, r=mixedpuppy

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/715ae99bc126