Closed Bug 1682632 Opened 8 months ago Closed 2 months ago

Expose in extension background service workers WebExtensions API webidl bindings

Categories

(WebExtensions :: General, task, P1)

task

Tracking

(firefox91 fixed)

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: mv3:m2 [mv3-m2])

Attachments

(15 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

The extension Manifest V3's background service worker should have access to the same set of extension APIs currently available to the extensions background page with Manifest V2 (minus some API methods that can't be made available on the worker threads, e.g. browser.extension.getViews, which does return the active window objects related to the other extension window globals that are part of the same extension).

After some discussions with interested parties (in particular with :asuth, for the DOM workers perspective, and :baku, for the WebIDL perspective) we have determined that the webidl bindings are the preferred approach to provide the WebExtensions API namespaces on the worker thread.

Implementation strategy

Given that a lot of the WebExtensions framework is currently implemented in privileged JS code running on the main thread, we agreed to proceed by initially integrating the WebExtensions API WebIDL bindings with the rest of the existing WebExtensions framework and the existing APIs implementation ext-*.js modules using the following strategy:

  • webextensions webidl stub methods shared by all WebExtensions APIs:
    A good amount of the API methods provided by the WebExtensions API webidl bindings will be implemented by stubs methods (shared by all WebExtensions API webidl bindings) that are going to forward the "API requests" (e.g. API methods being called, API event listeners being added or removed, or API properties being accessed) from the worker thread to the main thread:

  • JSONSchema-based parameter validations: the API requests received on the main thread will be validated using the WebExtensions API JSONSchema data ([1])

  • creating WebIDL definitions from the existing WebExtensions JSONSchema files: to simplify creating the API namespaces WebIDL definitions from the JSONSchema files we plan to provide an in-tree (python-based) script.

  • API Requests handling: on the main thread an API requests handler will receive the API requests forwarded by the worker thread and (after validating the parameter using the JSONSchema metadata) the API requests to be:

    • forwarded to the parent process, when the API is completely implemented in the parent process
    • forwarded to the related child/ext-*.js API module, when the API is completely implemented in the child process
  • API - Special cases: there will be a subset of API methods that can't forward their parameters to the main thread (e.g. because they accept parameters that aren't "structured cloneable") and/or they will have to do part of their work on the worker thread , in those cases:

    • those API method will not be handled by one of the "shared stub methods", but an actual custom method will be implemented to cover the particular case
    • in some cases the same API method may still have to call the "API requests" handler on the main thread to cover part of the work ([2]), and in that case the method should be able to reuse the same set of underlying helpers that the "stub methods" shared by all WebExtensions APIs will be using internally
  • Testing coverage:

    • creating a new set of unit tests to explicitly cover the shared stub methods and the underlying "API request forwarding" mechanism
    • adapting existing WebExtensions API tests to also run the same set of tests using a background service worker instead of a background page
    • creating additional new tests for testing scenario specifically related to the background service worker

[1] the metadata we have in the WebExtensions API JSONSchema files doesn't unfortunately translate 1-1 with the WebIDL definitions, and there are some other out of the tree components that are currently parsing and using them (the addons-linter in particular, which is used to validate the extension when they are submitted to AMO)

[2] e.g. to validate the subset of the parameters that can be structured cloned, or to delegate to the WebExtensions framework running on the main thread of the child or parent process some parts of the work needed (e.g. a good chunk of the browser.tests.assertRejects method will have to run on the worker thread because some of the parameters can't be structured cloned, but the outcome of the assertion will be forwarded to the API request handler to be sent to the test harness running in the main process).

Blocks: 1609923
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
  • Extension API WebIDL to be part of a new dom/extensions-webidl directory
    (and all webidl in this directory associated with WebExtensions::General
    bugzilla component)

  • Extension API C++ implementation in a new toolkit/components/extensions/webidl-api
    directory

  • Lock Extensions API WebIDL bindings provided to extensions workers global on:

    • the preference "extensions.backgroundServiceWorker.enabled" being set to true
    • checking explicitly that the worker is an extension service worker declared
      in the extension manifest.json file.
  • Changes to WorkerPrivate, WorkerScope.h/.cpp to expose the WebIDL
    bindings to the extension service workers (if the service worker has been
    detected as the background service worker specified in the manifest),
    plus small changes to RemoteWorkerChild.cpp to detect if the worker
    is the background service worker (and mark it as so in the WorkerPrivate
    instance associated to it)

Define a new WebExtensionStub extended attribute in the WebIDL generator
to be used in the WebExtensions API WebIDL definitions.

Depends on D70372

An ExtensionAPIBase class which provides a collection of shared stub methods and
shared helpers and used as a base class for the actual WebExtensions API webidl classes.

Depends on D84681

WebIDL interface and c++ class to be used by all WebExtension API webidl bindings
to expose the expected WebExtensions API events (e.g. browser.tabs.onUpdated etc.)

Depends on D84682

New XPCOM idl interfaces to define the components used to:

  • represent an API request (call to API methods, add/remove API event listener,
    get the value of an API property getter)
  • define the API request handler defined by privileged JS code running on the
    main thread

New ExtensionAPIBase shared stub methods and helper methods to create and forward
the API requests from the worker to the main thread and translate the results
got from the API request handler in the value to return or resolve (or the errors
to be raised) to the extension code running on the worker thread.

Depends on D80604

An ExtensionEventListener xpcom interface that wraps an API event callback
and included in the API requests forwarded to the API request handler.

The ExtensionEventListener xpcom interface provides a method to forward
to the worker thread the calls to the wrapped API event callpack originated
from the WebExtensions framework running on the main thread.

Depends on D75311

The ExtensionPort represents the webidl definitions of the runtime.Port WebExtensions API object.

A webidl dictionary will represent the serializable definition of the port properties, sent from
the API request handler to provide all the internal properties needed to create an instance
of the ExtensionPort webidl interface on the worker thread (which will be returned to the
extension code as the return value of the runtime.connect/connectNative methods and as a
parameter of the calls to the runtime.onConnect API event listeners).

Depends on D84684

ExtensionMockAPI is a fake WebExtensions API, locked behind a pref and
used in unit tests related to the API requests handling and WebExtensionStub
methods without relying on a specific WebExtension API.

Depends on D84687

  • Added a new set of xpcshell-tests in toolkit/components/extensions/test/xpcshell/webidl-api
    for unit tests related to the WebIDL bindings API request forwarding

Depends on D99886

Severity: -- → N/A
Priority: -- → P1
Blocks: 1688040
Attachment #9193469 - Attachment description: Bug 1682632 - part1.1: WebExtensionStub extended attribute in WebIDL generator. → WIP: Bug 1682632 - part1.1: WebExtensionStub extended attribute in WebIDL generator.
Attachment #9193472 - Attachment description: Bug 1682632 - part2: Extension API request handling. → WIP: Bug 1682632 - part2: Extension API request handling.
Attachment #9193472 - Attachment description: WIP: Bug 1682632 - part2: Extension API request handling. → Bug 1682632 - part2: Extension API request handling.
Attachment #9193469 - Attachment description: WIP: Bug 1682632 - part1.1: WebExtensionStub extended attribute in WebIDL generator. → Bug 1682632 - part1.1: WebExtensionStub extended attribute in WebIDL generator.
Whiteboard: mv3:m2
Attachment #9193480 - Attachment description: Bug 1682632 - part2.8: WebExtensions API request handling unit tests. → WIP: Bug 1682632 - part2.8: WebExtensions API request handling unit tests.
Attachment #9193480 - Attachment description: WIP: Bug 1682632 - part2.8: WebExtensions API request handling unit tests. → Bug 1682632 - part2.8: WebExtensions API request handling unit tests.
See Also: → 1713877
Whiteboard: mv3:m2 → mv3:m2 [mv3-m2]
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/006dfdbfe446
part1: add ExtensionBrowser webidl API skeleton to Extension Background Service Worker. r=baku,mixedpuppy,asuth
https://hg.mozilla.org/integration/autoland/rev/16d334e1548f
part1.1: WebExtensionStub extended attribute in WebIDL generator. r=peterv,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f78f3e3c8fa4
part1.2: ExtensionAPIBase class. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/017891ebb9dd
part1.3: ExtensionEventManager webidl interface. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6a9f2fdec9ff
part2: Extension API request handling. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/afcbc61ae254
part2.1: ExtensionEventListener. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/1bc53013f8b3
part2.2: ExtensionEventListener xpcom interface base implementation. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/89322328bf07
part2.3: ExtensionEventListener callListener promised return value. r=baku
https://hg.mozilla.org/integration/autoland/rev/8660a7bcc4e1
part2.4: ExtensionPort. r=baku
https://hg.mozilla.org/integration/autoland/rev/36540900695b
part2.5: ExtensionPort passed as a ExtensionEventListener callListener parameter. r=baku
https://hg.mozilla.org/integration/autoland/rev/9e321d3b964b
part2.6: ExtensionEventListener callListener sendResponse callback parameter. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/bf010d24288c
part2.7: ExtensionMockAPI. r=baku
https://hg.mozilla.org/integration/autoland/rev/3e47d00b7822
part2.8: WebExtensions API request handling unit tests. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/e14034a66620
part3: Restrict Extension API webidl bindings to nightly builds. r=baku,mixedpuppy

Backed out for mbu failures.

backout: https://hg.mozilla.org/integration/autoland/rev/11f571b7506c574cb869523b017bfe10db1fc612

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=e14034a66620ba25709c06c51f65a4483fe9022c&selectedTaskRun=BjFEsVcaR32nDsM03IU2gQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=342295625&repo=autoland&lineNumber=740

[task 2021-06-09T19:42:26.863Z]  0:20.35 ============================= test session starts ==============================
[task 2021-06-09T19:42:26.863Z]  0:20.35 platform linux -- Python 3.6.9, pytest-3.6.2, py-1.10.0, pluggy-0.6.0 -- /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/python-test/bin/python
[task 2021-06-09T19:42:26.864Z]  0:20.35 rootdir: /builds/worker/checkouts/gecko, inifile: /builds/worker/checkouts/gecko/config/mozunit/mozunit/pytest.ini
[task 2021-06-09T19:42:26.865Z]  0:20.35 collecting ... collected 5 items
[task 2021-06-09T19:42:26.865Z]  0:20.35 
[task 2021-06-09T19:42:26.866Z]  0:20.35 python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_browser TEST-UNEXPECTED-FAIL
[task 2021-06-09T19:42:26.867Z]  0:20.35 python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_extensions PASSED
[task 2021-06-09T19:42:26.867Z]  0:20.35 python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_js PASSED
[task 2021-06-09T19:42:26.868Z]  0:20.35 python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_memory PASSED
[task 2021-06-09T19:42:26.868Z]  0:20.35 python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_mobile_android TEST-UNEXPECTED-FAIL
[task 2021-06-09T19:42:26.869Z]  0:20.35
Flags: needinfo?(lgreco)

ouch... linting error due to a missing '{Enable|Disable}' part in the new config option help message :-(

D104707 just updated accordingly.

Push to try confirms that with that one line change the mbu job does now pass as expected:

Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/ba742f7e256f
part1: add ExtensionBrowser webidl API skeleton to Extension Background Service Worker. r=baku,mixedpuppy,asuth
https://hg.mozilla.org/integration/autoland/rev/7b8016e5f3fb
part1.1: WebExtensionStub extended attribute in WebIDL generator. r=peterv,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/9452456d249f
part1.2: ExtensionAPIBase class. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6c1f4efb4975
part1.3: ExtensionEventManager webidl interface. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c85363089c29
part2: Extension API request handling. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/55553e0dc6ca
part2.1: ExtensionEventListener. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a647c0cb85e4
part2.2: ExtensionEventListener xpcom interface base implementation. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/9195b13525d0
part2.3: ExtensionEventListener callListener promised return value. r=baku
https://hg.mozilla.org/integration/autoland/rev/57652a2152ac
part2.4: ExtensionPort. r=baku
https://hg.mozilla.org/integration/autoland/rev/7fa45afd83a0
part2.5: ExtensionPort passed as a ExtensionEventListener callListener parameter. r=baku
https://hg.mozilla.org/integration/autoland/rev/8b8bd2385503
part2.6: ExtensionEventListener callListener sendResponse callback parameter. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/d3a153070b38
part2.7: ExtensionMockAPI. r=baku
https://hg.mozilla.org/integration/autoland/rev/61380029a38b
part2.8: WebExtensions API request handling unit tests. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c5acc19db606
part3: Restrict Extension API webidl bindings to nightly builds. r=baku,mixedpuppy

Backed out 14 changesets (Bug 1682632) for causing hazard bustages in ExtensionEventManager.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/3a8309d8fd8b49e7de3ba7ab89936854ad293385
Push with failures, failure log.

Flags: needinfo?(lgreco)

(In reply to Alexandru Michis [:malexandru] from comment #19)

Backed out 14 changesets (Bug 1682632) for causing hazard bustages in ExtensionEventManager.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/3a8309d8fd8b49e7de3ba7ab89936854ad293385
Push with failures, failure log.

I'm looking into it (at the moment figuring out how to run that tests locally so that I can work on it more easily).

Flags: needinfo?(lgreco)

This change is meant to handle the mach hazards failure that triggered a backout of this stack of patches,
ExtensionEventManager::AddListener/RemoveListener should use a Rooted object as a key in the GCHashMap
(as the inline comment right above class GCHashMap is also pointing out explicitly).

The GCHashMap has been added into ExtensionEventManager as part of D80610 ("part2.2: ExtensionEventListener xpcom interface
base implementation"), and so we could as well absorb this patch into D80610 if the approach looks ok from the reviewers
perspective and we prefer that.

(I've also double-checked locally that these changes should not be triggering the mach hazards failure anymore
and confirmed that all the unit tests part of this stack of patches are completing successfully).

Depends on D104707

Attachment #9226535 - Attachment description: Bug 1682632 - EventEventManager should use a JS::Rooted<JSObject*> as key in the mListeners JS::GCHashMap. r?baku!,sfink → Bug 1682632 - ExtensionEventManager should use a JS::Rooted<JSObject*> as key in the mListeners JS::GCHashMap. r=baku,sfink
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/433f69c6d2ad
part1: add ExtensionBrowser webidl API skeleton to Extension Background Service Worker. r=baku,mixedpuppy,asuth
https://hg.mozilla.org/integration/autoland/rev/d15ff67b02ff
part1.1: WebExtensionStub extended attribute in WebIDL generator. r=peterv,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c40eaa3c0050
part1.2: ExtensionAPIBase class. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6b39a6bc633f
part1.3: ExtensionEventManager webidl interface. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/364fa6fe0b31
part2: Extension API request handling. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/591a2b97be13
part2.1: ExtensionEventListener. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/40821b435c18
part2.2: ExtensionEventListener xpcom interface base implementation. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/de38a1aa514f
part2.3: ExtensionEventListener callListener promised return value. r=baku
https://hg.mozilla.org/integration/autoland/rev/09ab8ace3537
part2.4: ExtensionPort. r=baku
https://hg.mozilla.org/integration/autoland/rev/53fed3c3fa6a
part2.5: ExtensionPort passed as a ExtensionEventListener callListener parameter. r=baku
https://hg.mozilla.org/integration/autoland/rev/a502d01a3975
part2.6: ExtensionEventListener callListener sendResponse callback parameter. r=baku,sfink
https://hg.mozilla.org/integration/autoland/rev/10cd26277351
part2.7: ExtensionMockAPI. r=baku
https://hg.mozilla.org/integration/autoland/rev/9c836dd75d4c
part2.8: WebExtensions API request handling unit tests. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/00cf51e23d67
part3: Restrict Extension API webidl bindings to nightly builds. r=baku,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/cd87dcdae2f3
ExtensionEventManager should use a JS::Rooted<JSObject*> as key in the mListeners JS::GCHashMap. r=sfink,baku

🎉🎉🎉🎉🎉🎉🎉

See Also: → 1716159
Regressions: 1716300
You need to log in before you can comment on or make changes to this bug.