Ensure that SessionStore is initialized before any interaction via WebExtension APIs

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [sessions]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8870809 - Flags: feedback?(mdeboer)

Comment 2

9 months ago
mozreview-review
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?
(Assignee)

Comment 3

9 months ago
mozreview-review-reply
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

9 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 6

9 months ago
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

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/715ae99bc126
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.