Opened on 03/26/2019 at 01:19:49 PM

Closed on 03/28/2019 at 12:30:38 PM

#7406 closed change (fixed)

Stop using Utils.runAsync

Reported by:	kzar	Assignee:	mjethani
Priority:	P2	Milestone:
Module:	Core	Keywords:	manifestv3
Cc:	mjethani, sebastian, hfiguiere, greiner	Blocked By:	#7381
Blocking:	#7338	Platform:	Unknown / Cross platform
Ready:	yes	Confidential:	no
Tester:	Unknown	Verified working:	no
Review URL(s):	https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/40

Description (last modified by mjethani)

Background

With #7338 we're no longer able to use some browser APIs from our background page. One thing we can no longer provide therefore is the Utils.runAsync function.

What to change

Stop using Utils.runAsync, namely in adblockpluscore/lib/downloader.js

Integration notes

Remove Utils.runAsync() from lib/utils.js in adblockpluschrome.

Hints for testers

Same as #7376.

Change History (13)

comment:1 Changed on 03/26/2019 at 01:32:21 PM by mjethani

Cc hfiguiere added

comment:2 Changed on 03/26/2019 at 01:32:35 PM by mjethani

Component changed from Unknown to Core

comment:3 Changed on 03/26/2019 at 01:39:08 PM by kzar

Keywords manifestv3 added

comment:4 Changed on 03/26/2019 at 09:01:20 PM by sebastian

Description modified (diff)

comment:5 follow-up: ↓ 8 Changed on 03/27/2019 at 06:14:07 AM by mjethani

I have looked into this and I would like to suggest the following:

In the download() method in lib/downloader.js in adblockpluscore, we return a promise that resolves or rejects when the download is complete or has failed; this would make it asynchronous anyway
We modify lib/subscriptionInit.js in adblockpluschrome and messageResponder.js in adblockplusui to do what formerly Utils.runAsync() would do for the case when the document is not in the ready state; adblockplusui would check the ready state, while adblockpluschrome would do the same for now but later switch to using ServiceWorkerContainer.ready or whatever other extension API is appropriate

comment:6 Changed on 03/27/2019 at 06:14:28 AM by mjethani

Cc greiner added

comment:7 Changed on 03/27/2019 at 06:16:12 AM by mjethani

Blocked By 7381 added

comment:8 in reply to: ↑ 5 Changed on 03/27/2019 at 06:45:23 AM by mjethani

Replying to mjethani:

In the download() method in lib/downloader.js in adblockpluscore, we return a promise that resolves or rejects when the download is complete or has failed; this would make it asynchronous anyway

Just to be clear, it doesn't have to return the promise for the purpose of resolving this issue. It could just keep the promise internal. In fact it could just use setTimeout() with a timeout of 0 as Utils.runAsync() does.

I have to point this out because returning a promise would entail further changes in the _download() method's implementation to make sure everything resolves/rejects correctly.

comment:9 Changed on 03/27/2019 at 07:34:50 PM by sebastian

For reference, the logic in Utils.runAsync() was a workaround that makes sure to call the callback not before all background scripts have been loaded/executed. There is a good chance that this workaround is no longer necessary, as most of our code is bundled together with webpack, and it might have been a browser bug in the first place that deferred loading of background scripts.

comment:10 Changed on 03/28/2019 at 09:22:02 AM by mjethani

Owner set to mjethani

comment:11 Changed on 03/28/2019 at 09:40:07 AM by mjethani

Review URL(s) modified (diff)
Status changed from new to reviewing

comment:12 Changed on 03/28/2019 at 12:29:17 PM by abpbot

A commit referencing this issue has landed:
Issue 7406 - Replace Utils.runAsync() with promise

comment:13 Changed on 03/28/2019 at 12:30:38 PM by mjethani

Description modified (diff)
Resolution set to fixed
Status changed from reviewing to closed

Context Navigation