Closed
Bug 1395215
Opened 7 years ago
Closed 7 years ago
ExtensionStorageSync.addOnChangedListener doesn't need to open a collection
Categories
(WebExtensions :: Untriaged, enhancement, P5)
WebExtensions
Untriaged
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.
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=772cfb323fda
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2348b6197bc
Updated•7 years ago
|
Priority: -- → P5
Comment 4•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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 7•7 years ago
|
||
mozreview-review-reply |
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 8•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•7 years ago
|
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/192a10e664c7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
No, this bug does not require manual testing.
Flags: needinfo?(eglassercamp)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•