Closed Bug 1365562 Opened 4 years ago Closed 4 years ago

Ensure that SessionStore is initialized before any interaction via WebExtension APIs

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Details

(Whiteboard: [sessions]triaged)

Attachments

(1 file)

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.
Attachment #8870809 - Flags: feedback?(mdeboer)
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 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 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.
Attachment #8870809 - Flags: review?(mixedpuppy) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/715ae99bc126
Ensure that SessionStore is initialized before any interaction via WebExtension APIs, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/715ae99bc126
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.