Closed
Bug 1454456
Opened 6 years ago
Closed 6 years ago
Make getAddonBlocklistState API asynchronous
Categories
(Toolkit :: Blocklist Implementation, enhancement, P1)
Toolkit
Blocklist Implementation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
(Whiteboard: [fxperf:p1])
Attachments
(1 file)
As the next part of making the blocklist async, this API is the less important add-on API (the more important one being the getAddonBlocklistEntry one that is being fixed in bug 1452618). There are a few callers: https://dxr.mozilla.org/mozilla-central/search?q=getAddonBlocklistState+-path%3Aobj-x86&redirect=false ------- browser/base/content/test/plugins/blocklist_proxy.js toolkit/mozapps/extensions/internal/AddonTestUtils.jsm Are basically versions that define their own copy of the method to help with tests. ------- toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm Uses this to check if updates for add-ons are also blocklisted. There is only 1 callsite: https://dxr.mozilla.org/mozilla-central/search?q=getnewestcompati&=mozilla-central in XPIInstall.jsm, in a method that's already async, so I assume we can just make this method async and then await the blocklist result, too. ------- toolkit/mozapps/extensions/nsBlocklistService.js has the public and private versions of the API. I'm making the public one async, the private ones will all change once we switch to indexeddb anyway. ------- toolkit/mozapps/extensions/test/xpcshell/test_blocklist_osabi.js toolkit/mozapps/extensions/test/xpcshell/test_overrideblocklist.js Are just some tests that need updating. ------- xpcom/system/nsIBlocklistService.idl has the API definition. ------- So hopefully this should be a straightforward change...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a049c7083de6fe8fea2d99ab0eabe8b427d70d0
Assignee | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8968307 [details] Bug 1454456 - make getAddonBlocklistState API asynchronous, https://reviewboard.mozilla.org/r/236982/#review243104 ::: browser/components/extensions/extension.css:7 (Diff revision 1) > /* stylelint-disable property-no-vendor-prefix */ > > /* Global */ > html, > body { > - background: transparent; > + /*background: transparent; */ Egh, this shouldn't have been in this commit. :-\
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8968307 [details] Bug 1454456 - make getAddonBlocklistState API asynchronous, https://reviewboard.mozilla.org/r/236982/#review243140 ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:126 (Diff revision 1) > unregister() { > MockRegistrar.unregister(this.originalCID); > } > > - getAddonBlocklistState(addon, appVersion, toolkitVersion) { > + async getAddonBlocklistState(addon, appVersion, toolkitVersion) { > + await Promise.resolve(); What is this for?
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #4) > Comment on attachment 8968307 [details] > Bug 1454456 - make getAddonBlocklistState API asynchronous, > > https://reviewboard.mozilla.org/r/236982/#review243140 > > ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:126 > (Diff revision 1) > > unregister() { > > MockRegistrar.unregister(this.originalCID); > > } > > > > - getAddonBlocklistState(addon, appVersion, toolkitVersion) { > > + async getAddonBlocklistState(addon, appVersion, toolkitVersion) { > > + await Promise.resolve(); > > What is this for? I'm attempting to force the mock to be asynchronous when the real API is also asynchronous, in an effort to have the test realistically reflect actual app usage even when it's using a mock. I don't know to what degree this is an effective way of doing so given microtask scheduling... That is, I could probably leave the mock as a non-async function and I expect consumers wouldn't mind - but it would possibly hide bugs. As it is, the commit is causing the test_update.js xpcshell test to fail, and I don't yet understand why, so...
Comment 6•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #5) > (In reply to Dave Townsend [:mossop] from comment #4) > > Comment on attachment 8968307 [details] > > Bug 1454456 - make getAddonBlocklistState API asynchronous, > > > > https://reviewboard.mozilla.org/r/236982/#review243140 > > > > ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:126 > > (Diff revision 1) > > > unregister() { > > > MockRegistrar.unregister(this.originalCID); > > > } > > > > > > - getAddonBlocklistState(addon, appVersion, toolkitVersion) { > > > + async getAddonBlocklistState(addon, appVersion, toolkitVersion) { > > > + await Promise.resolve(); > > > > What is this for? > > I'm attempting to force the mock to be asynchronous when the real API is > also asynchronous, in an effort to have the test realistically reflect > actual app usage even when it's using a mock. I don't know to what degree > this is an effective way of doing so given microtask scheduling... As I understand it you shouldn't need the await there to force that, promises resolve in a future microtask so the calling code should continue regardless.
Comment 7•6 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #6) > (In reply to :Gijs (he/him) from comment #5) > > I'm attempting to force the mock to be asynchronous when the real API is > > also asynchronous, in an effort to have the test realistically reflect > > actual app usage even when it's using a mock. I don't know to what degree > > this is an effective way of doing so given microtask scheduling... > > As I understand it you shouldn't need the await there to force that, > promises resolve in a future microtask so the calling code should continue > regardless. Yeah, with our current microtask handling, this should have exactly the same behavior as it would have without the await call, unless perhaps the addons map gets changed by another microtask (which it shouldn't). Something like `await new Promise(executeSoon)` might make sense, though.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a86aa22d1f00305f546452ec726311dd6b2c056
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #9) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a86aa22d1f00305f546452ec726311dd6b2c056 This is now green on try, at least... \o/
Comment 11•6 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #7) > Something like `await new Promise(executeSoon)` might make sense, though. Why?
Flags: needinfo?(kmaglione+bmo)
Comment 12•6 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #11) > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > blocked) from comment #7) > > Something like `await new Promise(executeSoon)` might make sense, though. > > Why? Because unlike a pre-resolved promise, that would have us return to the event loop, and execute resolution handlers after any other pending micro-tasks that get queued in the mean time. It's not super likely to make a difference in practice, but it does make the behavior closer to the behavior of the real blocklist service, where we'll have to actually read a file or make an indexedDB lookup, and I can pretty easily imagine a scenario where we accidentally depend on those promise handlers executing in order, as part of the current micro-task queue.
Flags: needinfo?(kmaglione+bmo)
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8968307 [details] Bug 1454456 - make getAddonBlocklistState API asynchronous, https://reviewboard.mozilla.org/r/236982/#review244384
Attachment #8968307 -
Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e2b0e792967f make getAddonBlocklistState API asynchronous, r=mossop
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2b0e792967f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in
before you can comment on or make changes to this bug.
Description
•