Integrate WebIDL-based WebExtensions API bindings with the WebExtensions framework internals
Categories
(WebExtensions :: General, task, P1)
Tracking
(firefox95 fixed)
Tracking | Status | |
---|---|---|
firefox95 | --- | fixed |
People
(Reporter: rpl, Assigned: rpl)
References
(Blocks 5 open bugs)
Details
(Whiteboard: [mv3-m2])
Attachments
(27 files, 2 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
As part of this issue, built on top of Bug 1682632 patches, we should introduce the following pieces of the implementation strategy described in Bug 1682632 comment 0:
-
creating WebIDL definitions from the existing WebExtensions JSONSchema files: a script should read all the JSONSchema files that describes the WebExtensions APIs and allow to:
- generate a webidl definition for a given an API namespace
- generate an example boilerplate c++ cpp and related header file (just a basic boilerplate to make it easier to bootstrap the files needed when introducing a new webidl binding for a WebExtensions API)
- NOTE:
- we decided (in past meetings with baku while prototyping the implementation strategy) to don't aim to generate the WebIDL files at build time yet, but to pre-generate, commit and review the generated webidl files as part of the patches that are going to wiring up new WebIDL-based WebExtensions API bindings.
- In follow-ups we may evaluate to run the script automatically to make sure that the generated WebIDL file would match the one in tree (to ensure that the WebIDL definitions and the API jsonschema definitions for an API are not diverging without being noticed)
-
API Requests handling: as part of Bug 1682632, a new idl file describes a
mozIExtensionAPIRequestHandler
interface to be implemented in privileged JS code running on the main thread, as part of this issue theExtensionAPIRequestHandler
object (defined as empty in Bug 1682632, and mocked as part of the unit tests included in Bug 1682632) should be implemented and take care of:- validate and normalize arguments got as part of the API requests received (and in case of schema validation errors it should return a representation of the error that will be sent back to the worker thread and be raised to the caller)
- forward to the main process the API request that are related to APIs implemented in the parent process
- forward to the appropriate child ext-*.js API implementation module the API request that are related to APIs implemented in the child process
- send a generic "An unexpected error occurred" in case of unexpected errors while handling an API request
-
Introduce a WebIDL binding for the
browser.test
API available to the extension background service worker, generated using the script described above and handled using the API request handling described right above (and a small smoke test that exercises the API methods and API event provided by this API)- NOTE: some of the
browser.test
API methods cannot be implemented using the stub methods provided by Bug 1682632 methods (in particularbrowser.test.withHandlingUserInput
,browser.test.assertEq
,browser.test.assertRejects
andbrowser.test.assertThrows
), e.g. because their argument can't be serialized, those are part of the API special cases mentioned in Bug 1682632 comment 0 and will be implemented in their own C++ methods.
- NOTE: some of the
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
Assignee | ||
Comment 3•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
|
||
Assignee | ||
Comment 6•4 years ago
|
||
This is currently a temporary quick approach to more quickly verify the
worker child context with a sample of API calls proxied to the main process
(currently using the idle API, as it is pretty small and simple).
It is likely better for this patch to create a new parent context that is specifically
meant to be used for the worker proxied contexts.
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
After adding ExtensionAlarms.webidl (and related header and cpp files), the following error was preventing to complete
the build successfully:
...
0:40.61 In file included from UnifiedBindings7.cpp:2:
0:40.61 /home/rpl/mozlab/projects/webext-v3-serviceworker/mc-rebasing-patchstack/obj-x86_64-pc-linux-gnu/dom/bindings/GeometryUtilsBinding.cpp:384:5: error: static_assert failed due to requirement 'IsRefcounted<mozilla::dom::Document>::value' "We can only store refcounted classes."
0:40.61 static_assert(IsRefcounted<mozilla::dom::Document>::value, "We can only store refcounted classes.");
The build errors seems to be triggered from GeometryUtilsBindings.cpp
, and the following seems to be the reason behind it:
GeometryUtils.webidl
does include a TextOrElementOrDocument union typeGeometryUtilsBinding.cpp
generated file will include abool OwningTextOrElementOrDocument::TrySetToDocument(...)
method due to the union type
(See in generated file here)GeometryUtilsBindings.h
is including a forward declaration for themozilla::dom::Document
classGeometryUtilsBinding.cpp
doesn't contain an explicit#include "mozilla/dom/Document.h"
I'm not yet sure why adding an apparently unrelated webidl file would trigger this error (which isn't triggered if I just revert the patch adding the ExtensionAlarms.webidl,
but keep all the other patches attached to both Bug 1682632 and Bug 1688040), my current guess is that adding ExtensionAlarms.webidl may be changing the order or some
of the included headers.
After some digging, I did notice that adding an explict #include "mozilla/dom/Document.h"
to GeometryUtilsBinding.cpp
was enough to let the build to complete successfully,
but given that it is a generated file I did look into what could be the change to apply on dom/bindings/Codegen.py
to detect that a certain WebIDL file is going to include
a unionType like the one in GeometryUtils.webidl
and to be sure to include "mozilla/dom/Document.h" in the generated cpp file in that case.
Submitting this patch as part of Bug 1688040 stack of patches for now, mainly to be able to discuss about the issue with other engineers
with more experience related to this kind of issues, but it may be moved to a separate bugzilla issue if the fix seems reasonable, or removed
if the issue turned out to be triggered by some of the proposed changes in Bug 1682632 and Bug 1688040 patches.
Depends on D102644
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 24•4 years ago
|
||
This patch does:
- add to ExtensionBrowser two new data members to keep track of value for the browser.runtime.lastError and if it was checked while
the chrome compatible callback was being executed - add to ExtensionBrowser 3 new methods to set, get and clear the lastError value
- add a reference to the ExtensionBrowser to all API namespaces and API objects classes, because it has to be then propagated to the
ChromeCompatCallbackHandler instances that are being attached to the promise result of the async API methods calls if the caller did
pass the optional callback parameter - tweak the ChromeCompatCallbackHandler class to set the lastError value before calling the callback in ChromeCompatCallbackHAndler::RejectedCallback
and then clear it after the call has been completed and report it to the console if it wasn't checked - change the ExtensionRuntime::GetLastError methhod to restrieve and return the value from the ExtensionBrowser instance
Depends on D106706
Assignee | ||
Comment 26•4 years ago
|
||
The extensions expect that ExtensionPort instances they get in the calls to the
port event listeners to always be the same, and to be also strictly equal to the
object port got from browser.runtime.connect or browser.runtime.onConnect:
const port = browser.runtime.connect();
port.onDisconnect.addListener(disconnectedPort => {
// port === disconnectedPort => true
});
This patch does add an extension port lookup map in the ExtensionBrowser
class and a new ExtensionBrowser::GetPort method which is responsible of
providing the expected behavior (returning an existing istance if one is
found in the lookup map).
Depends on D107553
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 28•3 years ago
|
||
This patch is a follow up to part 8.2, both of the TODOs added in part 8.2 and removed in this patch
depends from the changes introduced in "part 10.1", "part 10.22" and "part 10.3" right before this one.
This patch also adds an additional assertion to the smoke tests for the runtime API part of
test_ext_webidl_api_request_handler.js to confirm that the ExtensionPort instance received as the
last parameter of the port.onMessage listener is strictly equal to the port
the listener was
added on.
Depends on D107555
Assignee | ||
Comment 29•3 years ago
|
||
This patch fixes a leak that I spotted while investigating a separate shutdown leak triggered by D121683
(This one is not strictly related to D121683, but apparently none of the xpcshell tests part of this
stack of patches reported it at a shutdown leak, nevertheless it was detected as a shutdown leak
while running the test_ext_identity.html mochitest on the background service worker, after
the other leak specific to D121683 was fixed, and after investigating it using cc logs and heapgraph's
find_roots.py I confirmed that it's a shutdown leak introduced here in RequestWorkerRunnable::ProcessHandlerResult).
Depends on D122967
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 30•3 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/925f23a58da6 part1.1: Prototype forwarding API request for local implemented API to ext-*.js modules. r=zombie https://hg.mozilla.org/integration/autoland/rev/f719b9d0bc25 part1.2: Prototype handling API requests for API modules implemented on the main process r=zombie https://hg.mozilla.org/integration/autoland/rev/cfdf6e87d820 part1.3: Fix pre-existing issue test.onMessage.removeListener not removing previously added listeners. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/d413ec19bded part1.4: minimal ProcessConduits prototype r=zombie https://hg.mozilla.org/integration/autoland/rev/30cc613912ce part1.5: use ProcessConduit in worker child context r=zombie https://hg.mozilla.org/integration/autoland/rev/49d3e1baf577 part1.6: adapting parent context to workers r=zombie https://hg.mozilla.org/integration/autoland/rev/615b683acb00 part2.1: Allow storing normalized arguments in the mozIExtensionAPIRequest object. r=baku https://hg.mozilla.org/integration/autoland/rev/d7d6b1bec92a part2.2: validate and normalize webidl API requests arguments using the JSON API schemas r=zombie,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/15e0689a57f0 part3: python script to generate webidl definitions from WebExtension API JSON schema files. r=robwu https://hg.mozilla.org/integration/autoland/rev/7d0ce555263f part4.1: Initial WebIDL-based bindings for browser.test API namespace r=baku https://hg.mozilla.org/integration/autoland/rev/6a2ebb56325d part4.2: Hook up browser.test API webidl binding to ExtensionBrowser. r=baku https://hg.mozilla.org/integration/autoland/rev/db374d34839a part4.3: cover WebIDL-based browser.test API with smoke tests r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/b82645d5b7c7 part5: Added WebIDL-based binding for the browser.alarms WebExtensions API. r=baku https://hg.mozilla.org/integration/autoland/rev/08737e7ea047 part6: Added chrome global as a BindingAlias of the browser global. r=baku https://hg.mozilla.org/integration/autoland/rev/20e38838aa89 part7.1: Added WebIDL-based binding for the browser.runtime WebExtensions API. r=baku https://hg.mozilla.org/integration/autoland/rev/b7dcade29dab part7.2: Listen for background service worker changes and emit internal event to ensure extension.wakeupBackground promise is resolved. r=zombie https://hg.mozilla.org/integration/autoland/rev/81e957717eb1 part7.3: Adapt child/ext-runtime.js to handle webidl API request. r=zombie https://hg.mozilla.org/integration/autoland/rev/c8d890f1a8e2 part7.4: Add xpcshell test for browser.runtime API called from a background service worker. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/4d9b2a9dd247 part8.1: Add stub method for string and jsvalue properties. r=baku https://hg.mozilla.org/integration/autoland/rev/64221db54d98 part8.2: Handle ExtensionPort API requests r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/7fc233bf144c part8.3: Add xpcshell tests for browser.runtime.connect/onConnect and Port API. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/922ca2f20fa1 part9: Implement setting/clearing and reporting browser.runtime.lastError from ChromeCompatCallbackHandler::RejectedCallback. r=baku https://hg.mozilla.org/integration/autoland/rev/fdbced7a94f3 part10.1: Support prepending or appending api object instance to the listener call arguments. r=baku https://hg.mozilla.org/integration/autoland/rev/8d33bdd8d0b8 part10.2: Return existing ExtensionPort instance based on the ExtensionPortDescriptor portId. r=baku https://hg.mozilla.org/integration/autoland/rev/88395ab5a5b5 part10.3: Store weakptr in the ports lookup map and clear the entry when released. r=baku https://hg.mozilla.org/integration/autoland/rev/36cdc1600e66 part10.4: Test ExtensionPort instance appended to port.onMessage callback arguments is strictly same instance of the same port. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/85ba163b6423 part11: Fix dom::Promise leaked by RequestWorkerRunnable::ProcessHandlerResult when handling the result of an async method call. r=baku
Comment 31•3 years ago
|
||
Backed out for causing multiple failures.
Failure log for gv-junit
Failure log for gv-junit-e10s-single
Failure log for bc failures on browser_ext_chrome_settings_overrides_home.js
Failure log for bc failures on browser_identityPopup_clearSiteData_extensions.js
Assignee | ||
Comment 33•3 years ago
|
||
(In reply to Agi Sferro | :agi | ni? for questions | β° PST | he/him from comment #32)
FYI the
mSearchesOngoing
crash is unrelated, see Bug 1733737
Thanks Agi! I'll keep in mind that part is actually unrelated and wouldn't point me in the right direction.
I've already started to look into the browser_ext_chrome_settings_overrides_home.js failure and I think that I have already identified which part of the entire stack of patches may be triggering it (and obviously it looks that it may be related to just 3 lines in the first patch).
It is not unlikely that the root cause may be the same also for the gv-junit onces (I missed to notice earlier that the failure it is not the same from the pre-existing intermittents already tracked by other bugzilla issue, and the test case is extensions-related).
I'm going to keep the needinfo assigned to me, I'll clear it once a got a push to try that confirms me that the changes I've been trying locally do fix also the gv-junit failures.
Assignee | ||
Comment 34•3 years ago
|
||
The previous failing jobs looks good in the new try push:
which confirms that the few lines change that I was expecting to be the one triggering the browser_ext_chrome_settings_home.js failure, it is also the ones behind the browser_identityPopup_clearSiteData_extensions.js and gv-junit failures.
I'll update D86198 on Phabricator accordingly.
Comment 35•3 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/8800f18c12e9 part1.1: Prototype forwarding API request for local implemented API to ext-*.js modules. r=zombie https://hg.mozilla.org/integration/autoland/rev/36fb51bf13d9 part1.2: Prototype handling API requests for API modules implemented on the main process r=zombie https://hg.mozilla.org/integration/autoland/rev/45984423f329 part1.3: Fix pre-existing issue test.onMessage.removeListener not removing previously added listeners. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/a498045b4fb9 part1.4: minimal ProcessConduits prototype r=zombie https://hg.mozilla.org/integration/autoland/rev/65d9a0a61133 part1.5: use ProcessConduit in worker child context r=zombie https://hg.mozilla.org/integration/autoland/rev/e1d6df054a6b part1.6: adapting parent context to workers r=zombie https://hg.mozilla.org/integration/autoland/rev/8c38fb6bb94c part2.1: Allow storing normalized arguments in the mozIExtensionAPIRequest object. r=baku https://hg.mozilla.org/integration/autoland/rev/6b3fc60259c3 part2.2: validate and normalize webidl API requests arguments using the JSON API schemas r=zombie,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/7d6bee4ac76f part3: python script to generate webidl definitions from WebExtension API JSON schema files. r=robwu https://hg.mozilla.org/integration/autoland/rev/78b1fcc32a94 part4.1: Initial WebIDL-based bindings for browser.test API namespace r=baku https://hg.mozilla.org/integration/autoland/rev/f7465c8d4504 part4.2: Hook up browser.test API webidl binding to ExtensionBrowser. r=baku https://hg.mozilla.org/integration/autoland/rev/d64be3a48624 part4.3: cover WebIDL-based browser.test API with smoke tests r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/7591f72b60c4 part5: Added WebIDL-based binding for the browser.alarms WebExtensions API. r=baku https://hg.mozilla.org/integration/autoland/rev/049e4405cf1c part6: Added chrome global as a BindingAlias of the browser global. r=baku https://hg.mozilla.org/integration/autoland/rev/5679e7ef5e59 part7.1: Added WebIDL-based binding for the browser.runtime WebExtensions API. r=baku https://hg.mozilla.org/integration/autoland/rev/edc0e33c8988 part7.2: Listen for background service worker changes and emit internal event to ensure extension.wakeupBackground promise is resolved. r=zombie https://hg.mozilla.org/integration/autoland/rev/7bd6baf30639 part7.3: Adapt child/ext-runtime.js to handle webidl API request. r=zombie https://hg.mozilla.org/integration/autoland/rev/4a7d253247b3 part7.4: Add xpcshell test for browser.runtime API called from a background service worker. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/921f4f47a654 part8.1: Add stub method for string and jsvalue properties. r=baku https://hg.mozilla.org/integration/autoland/rev/a33fd04e1d2b part8.2: Handle ExtensionPort API requests r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/d3c074819ea5 part8.3: Add xpcshell tests for browser.runtime.connect/onConnect and Port API. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/bd8300f5245e part9: Implement setting/clearing and reporting browser.runtime.lastError from ChromeCompatCallbackHandler::RejectedCallback. r=baku https://hg.mozilla.org/integration/autoland/rev/9215397dc155 part10.1: Support prepending or appending api object instance to the listener call arguments. r=baku https://hg.mozilla.org/integration/autoland/rev/e02c88dd86f5 part10.2: Return existing ExtensionPort instance based on the ExtensionPortDescriptor portId. r=baku https://hg.mozilla.org/integration/autoland/rev/a78b64d966b2 part10.3: Store weakptr in the ports lookup map and clear the entry when released. r=baku https://hg.mozilla.org/integration/autoland/rev/9bbc0c5d8ebe part10.4: Test ExtensionPort instance appended to port.onMessage callback arguments is strictly same instance of the same port. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/bed7b8df0aa7 part11: Fix dom::Promise leaked by RequestWorkerRunnable::ProcessHandlerResult when handling the result of an async method call. r=baku
Comment 36•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8800f18c12e9
https://hg.mozilla.org/mozilla-central/rev/36fb51bf13d9
https://hg.mozilla.org/mozilla-central/rev/45984423f329
https://hg.mozilla.org/mozilla-central/rev/a498045b4fb9
https://hg.mozilla.org/mozilla-central/rev/65d9a0a61133
https://hg.mozilla.org/mozilla-central/rev/e1d6df054a6b
https://hg.mozilla.org/mozilla-central/rev/8c38fb6bb94c
https://hg.mozilla.org/mozilla-central/rev/6b3fc60259c3
https://hg.mozilla.org/mozilla-central/rev/7d6bee4ac76f
https://hg.mozilla.org/mozilla-central/rev/78b1fcc32a94
https://hg.mozilla.org/mozilla-central/rev/f7465c8d4504
https://hg.mozilla.org/mozilla-central/rev/d64be3a48624
https://hg.mozilla.org/mozilla-central/rev/7591f72b60c4
https://hg.mozilla.org/mozilla-central/rev/049e4405cf1c
https://hg.mozilla.org/mozilla-central/rev/5679e7ef5e59
https://hg.mozilla.org/mozilla-central/rev/edc0e33c8988
https://hg.mozilla.org/mozilla-central/rev/7bd6baf30639
https://hg.mozilla.org/mozilla-central/rev/4a7d253247b3
https://hg.mozilla.org/mozilla-central/rev/921f4f47a654
https://hg.mozilla.org/mozilla-central/rev/a33fd04e1d2b
https://hg.mozilla.org/mozilla-central/rev/d3c074819ea5
https://hg.mozilla.org/mozilla-central/rev/bd8300f5245e
https://hg.mozilla.org/mozilla-central/rev/9215397dc155
https://hg.mozilla.org/mozilla-central/rev/e02c88dd86f5
https://hg.mozilla.org/mozilla-central/rev/a78b64d966b2
https://hg.mozilla.org/mozilla-central/rev/9bbc0c5d8ebe
https://hg.mozilla.org/mozilla-central/rev/bed7b8df0aa7
Updated•1 year ago
|
Description
•