Closed Bug 1454456 Opened 6 years ago Closed 6 years ago

Make getAddonBlocklistState API asynchronous

Categories

(Toolkit :: Blocklist Implementation, enhancement, P1)

enhancement

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 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 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?
(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...
(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.
(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.
(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/
(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)
(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 on attachment 8968307 [details]
Bug 1454456 - make getAddonBlocklistState API asynchronous,

https://reviewboard.mozilla.org/r/236982/#review244384
Attachment #8968307 - Flags: review?(dtownsend) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2b0e792967f
make getAddonBlocklistState API asynchronous, r=mossop
https://hg.mozilla.org/mozilla-central/rev/e2b0e792967f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.