Opened 3 years ago

Closed 21 months ago

Last modified 19 months ago

#4579 closed change (fixed)

Target promise-based browserext API

Reported by: kzar Assignee: mjethani
Priority: P3 Milestone: Adblock-Plus-3.0.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, oleksandr, greiner Blocked By: #4551, #4783, #5028
Blocking: #4580 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29573892/
https://codereview.adblockplus.org/29678657/

Description (last modified by kzar)

Background

We want to target the browserext standard instead of the Chrome extension API. This should hopefully mean Edge and in the future Gecko can be supported without pollyfills.

What to change

  • Add a polyfill for the Chrome extension to add browserext compatibility, adding the browser namespace, wrapping asynchronous APIs to use promises and mapping the others directly. (See this existing polyfill
  • Replace any other use of the Chrome APIs with the with the browserext API.

Change History (37)

comment:1 Changed 3 years ago by kzar

  • Blocked By 4551 added; 4511 removed

comment:2 Changed 3 years ago by kzar

  • Description modified (diff)

comment:3 Changed 3 years ago by kzar

  • Description modified (diff)
  • Summary changed from Remove ext compatibility layer, target browser extension API to Target the browser extension API

comment:4 Changed 3 years ago by sebastian

  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 3 years ago by kzar

  • Blocking 4580 added

comment:6 Changed 3 years ago by kzar

  • Summary changed from Target the browser extension API to Target the browserext API

comment:7 Changed 3 years ago by kzar

  • Cc oleksandr added
  • Owner set to kzar

comment:8 Changed 3 years ago by kzar

  • Blocked By 4783 added

comment:9 Changed 3 years ago by greiner

  • Cc greiner added

comment:10 Changed 3 years ago by sebastian

Giving it a second though (and having a go myself) it seems too early to target (and polyfill) the Promise based API. As of now, neither Chrome nor Microsoft Edge supports Promises with their API. Only Firefox does, which also optionally accepts callbacks. As a result callbacks (as opposed to Promises) are the common denominator of all targeted browsers. Implementing a polyfill that allows us to use Promises will add complexity and overhead.

But what I think we should already do is replacing all occurrences of chrome with browser, aliasing browser = chrome on Chrome. We already did it the other way around in the edge branch (#3695), and we might need it for Firefox/WebExtensions as well. However, it feels backwards, to keep using the chrome.* namespace, while this code runs on other browser, and browser is going to be standardized.

comment:11 Changed 3 years ago by kzar

I already made a lot of progress towards targeting the browser API, if you're curious to have a look here's my branch. It has been a while since I looked at it but IIRC I didn't quite finish, and also I planned to try using a lighter weight polyfill that Wladimir suggested. (Also I need to rebase again...) I did have mostly everything working however.

But I kind of agree with you, I think it might well be a little early to make the switch.

comment:12 Changed 3 years ago by kzar

  • Owner kzar deleted

Unassigned myself since I don't have the time to continue working on this currently and as per the above discussion we're not sure if it's the right time to make the switch anyway.

comment:13 Changed 3 years ago by sebastian

  • Blocked By 5028 added

comment:14 Changed 3 years ago by sebastian

  • Ready unset

I moved the part about using browser.* with callbacks (instead of promises), for now, into #5028.

comment:15 Changed 3 years ago by sebastian

  • Summary changed from Target the browserext API to Target promise-based browserext API

comment:16 Changed 2 years ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:17 Changed 2 years ago by mjethani

  • Status changed from new to reviewing

comment:18 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Promisify APIs

comment:19 Changed 2 years ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:20 Changed 2 years ago by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:21 Changed 2 years ago by mjethani

  • Milestone Adblock-Plus-for-Chrome-Opera-Firefox-next deleted

comment:22 Changed 2 years ago by kzar

I notice a whole bunch of exceptions in the background console since this landed:

Uncaught (in promise) {message: "The message port closed before a response was received."}
func.call.result @ polyfill.js:80
sendResponseAndClearCallback @ extensions::messaging:415
disconnectListener @ extensions::messaging:439
EventImpl.dispatchToListener @ extensions::event_bindings:403
publicClassPrototype.(anonymous function) @ extensions::utils:140
EventImpl.dispatch_ @ extensions::event_bindings:387
EventImpl.dispatch @ extensions::event_bindings:409
publicClassPrototype.(anonymous function) @ extensions::utils:140
dispatchOnDisconnect @ extensions::messaging:376

comment:23 Changed 2 years ago by mjethani

It seems that when we call runtime.sendMessage with a callback, and we don't get a response, Chrome considers that to be something worth reporting in runtime.lastError

We may have to handle this case in the wrapper, since it's usually not an error the caller is interested in.

comment:24 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Wrap rejection reason in Error object

comment:25 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Ignore runtime.lastError caused by wrapper

comment:26 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Do not wrap non-existent APIs

comment:27 follow-up: Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Wrap runtime.onMessage

comment:28 in reply to: ↑ 27 Changed 21 months ago by kzar

Replying to abpbot:

A commit referencing this issue has landed:
Issue 4579 - Wrap runtime.onMessage

This last commit is causing a couple of linting errors Manish:

adblockpluschrome/polyfill.js

  133:39  error  'hasListener' is assigned a value but never used  no-unused-vars
  160:15  error  Unsafe usage of ThrowStatement                    no-unsafe-finally
Last edited 21 months ago by kzar (previous) (diff)

comment:29 Changed 21 months ago by mjethani

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

comment:30 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Fix ESLint errors

comment:31 Changed 21 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:32 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Fix ESLint errors

comment:33 Changed 21 months ago by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:34 Changed 21 months ago by mjethani

Changeset 7247eb4632fa has caused a regression due to the way the wrappers are implemented. We expect tabs.insertCSS to throw an error, which it does, but that error is turned into a promise rejection by the wrapper. We could simply handle the rejection, but that would be asynchronous and would cause the code in lib/cssInjection.js to become more complicated, aside from being inconsistent with Firefox.

The wrapper should also throw an error if the wrapped function throws an error.

I have logged #6343 for this.

Last edited 21 months ago by mjethani (previous) (diff)

comment:35 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 4579, 6343 - Let errors from wrapped API calls bubble up

comment:36 Changed 21 months ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reopened to closed

comment:37 Changed 19 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Didn't see any of the errors described above. And using browser.runtime.sendMessage (for example) in the console works.

ABP 3.0.2.1983
Firefox 51 / 58 / Windows 10
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7

Note: See TracTickets for help on using tickets.