Closed Bug 1395215 Opened 4 years ago Closed 4 years ago

ExtensionStorageSync.addOnChangedListener doesn't need to open a collection

Categories

(WebExtensions :: Untriaged, enhancement, P5)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

References

Details

Attachments

(1 file)

Originally, ExtensionStorageSync#getCollection was used to ensure that Sqlite connections were cleaned up when extensions were unloaded. However, this has long since changed and we now have cleaner mechanisms ensuring that Sqlite connections are closed. However, we still use getCollection to signal that an extension wants to be synced, even in places that don't actually need a Kinto collection like `addOnChangedListener`. This leads to failures like https://bugzilla.mozilla.org/show_bug.cgi?id=1377533#c9 where we try to open a collection asynchronously after an extension has unloaded.

`getCollection` should really be split into two methods, one of which signals that an extension wants to be synced, and another which actually gets a Kinto collection asynchronously. This would resolve whatever hacks :leplatrem is putting into the patch to fix the other bug.
See Also: → 1377533
Priority: -- → P5
Comment on attachment 8904718 [details]
Bug 1395215: remove asynchrony from addListener,

https://reviewboard.mozilla.org/r/176504/#review182450

::: toolkit/components/extensions/ExtensionStorageSync.jsm:1090
(Diff revision 1)
>      }
>    }
>  
> +  registerInUse(extension, context) {
> +    // Register that the extension and context are in use.
> +    if (!extensionContexts.has(extension)) {

Nit: Please just make `extensionContexts` a `new DefaultWeakMap(() => new Set())`

::: toolkit/components/extensions/ExtensionStorageSync.jsm:1229
(Diff revision 1)
>    addOnChangedListener(extension, listener, context) {
>      let listeners = this.listeners.get(extension) || new Set();
>      listeners.add(listener);
>      this.listeners.set(extension, listeners);
>  
> -    // Force opening the collection so that we will sync for this extension.
> +    this.registerInUse(extension, context);

Won't this prevent us from syncing, and notifying for, remote changes to the collection until the extension does a get() or set() call?
Comment on attachment 8904718 [details]
Bug 1395215: remove asynchrony from addListener,

https://reviewboard.mozilla.org/r/176504/#review182450

> Nit: Please just make `extensionContexts` a `new DefaultWeakMap(() => new Set())`

I can't use a WeakMap variant because I iterate over the keys to figure out what extensions to sync, and WeakMaps don't allow me to iterate over keys. However, I have changed this to a DefaultMap.

> Won't this prevent us from syncing, and notifying for, remote changes to the collection until the extension does a get() or set() call?

No, because `registerInUse` is the thing that makes us start syncing the collection (because it adds it to `extensionContexts`).
Comment on attachment 8904718 [details]
Bug 1395215: remove asynchrony from addListener,

https://reviewboard.mozilla.org/r/176504/#review182450

> No, because `registerInUse` is the thing that makes us start syncing the collection (because it adds it to `extensionContexts`).

Great. Thanks
Comment on attachment 8904718 [details]
Bug 1395215: remove asynchrony from addListener,

https://reviewboard.mozilla.org/r/176504/#review182514
Attachment #8904718 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/192a10e664c7
remove asynchrony from addListener, r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/192a10e664c7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(eglassercamp)
No, this bug does not require manual testing.
Flags: needinfo?(eglassercamp)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.