Closed Bug 1930776 Opened 1 year ago Closed 2 months ago

Implement userScripts.execute

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox153 fixed)

RESOLVED FIXED
153 Branch
Tracking Status
firefox153 --- fixed

People

(Reporter: robwu, Assigned: robbendebiene)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [wecg][addons-jira])

Attachments

(2 files, 1 obsolete file)

This bug tracks our implementation of userScripts.execute as specified in https://github.com/w3c/webextensions/blob/main/proposals/user-scripts-execute-api.md (added in https://github.com/w3c/webextensions/pull/540).

This was mentioned before as the end of the list at https://bugzilla.mozilla.org/show_bug.cgi?id=1875475#c3 (and is not part of the MVP).

Chrome has not implemented this yet. I cannot even find an issue on bugs.chromium.org that tracks an implementation.

Severity: -- → N/A
Priority: -- → P3

Related Chromium bug seems to be private:
https://issues.chromium.org/issues/382190916
https://issues.chromium.org/issues/326657581

Status: Duplicate of 326657581

This is already planned, hopefully can be done in Q1 2025.

The current proposal specifies userScripts.execute to take one script only, and I wonder whether it should be an array instead: https://github.com/w3c/webextensions/issues/477#issuecomment-2561976716

Furthermore, userScripts.execute also accepts documentId to make sure that the script is executed in the desired document. This has not been implemented in Firefox yet, bug 1891478 tracks documentId work.

There is another pending query on the expected behavior: https://github.com/w3c/webextensions/pull/745#pullrequestreview-2556289504

Whiteboard: [wecg] → [wecg][addons-jira]

Note, for BCD when userScripts.execute is documented as part of dev-doc-needed:

Chrome has enabled userScripts.execute in Chrome version 135.
Commit: https://chromium.googlesource.com/chromium/src/+/47209ae4c98108dae21d9a52031e504547bf964e
Version: https://chromium.googlesource.com/chromium/src/+/47209ae4c98108dae21d9a52031e504547bf964e/chrome/VERSION

Duplicate of this bug: 1516478

I'm still interested in this bug/feature. Would you consider a contribution or are there still other things blocking it?
I read about the unspecified behaviour on runtime errors, but this doesn't seem like a major blocker.

If nothing speaks against a contribution, can someone please give me some short guidance or a starting point?

Patches are welcome indeed. Here are some pointers feel free to ask clarification if needed.

General contribution guidelines are at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

The entry point, the first place in the implementation that handles API calls in the parent process is at https://searchfox.org/firefox-main/rev/a249a43995d7634e6e4a56bfd6496039031648bc/toolkit/components/extensions/parent/ext-userScripts.js#88-187

This is the relevant place showing where how a user script is represented internally: https://searchfox.org/firefox-main/rev/8352bcb6d75d53f3e2190221b71190e47afa0bfc/toolkit/components/extensions/ExtensionUserScripts.sys.mjs#481

For a one-off user script, look at the tabs.executeScript and scripting.executeScript implementations for inspiration:

The object constructed there is handled here: https://searchfox.org/firefox-main/rev/8352bcb6d75d53f3e2190221b71190e47afa0bfc/toolkit/components/extensions/ExtensionContent.sys.mjs#1453

Most unit tests for MV3 userScripts are currently written as xpcshell tests. These are preferred where possible because they are self contained and can run in parallel with other tests. The current tests are found here: https://searchfox.org/firefox-main/search?q=test_ext_userScripts_mv3&path=&case=false&regexp=false

Unfortunately there is one issue with xpcshell tests: they don't have browser windows and tabs, so the tabIds do not exist. As an alternative you can write the test as a browser chrome test, or a mochitest. Browser tests only run with desktop Firefox, mochitests are also run on Android. Since userScripts is a cross-platform API, a mochitest is therefore preferred. Here is a minimal example: https://searchfox.org/firefox-main/rev/8352bcb6d75d53f3e2190221b71190e47afa0bfc/toolkit/components/extensions/test/mochitest/test_ext_executeScript_mozextension.html#329-378

I am currently working on documentId support (bug 1891478); if that were to be implemented, it would be possible to inject in any context even if it is not in a tab, including in xpcshell tests.

Thanks for the detailed description. I get to it and report back when I have something.

handleActorExecute currently only accepts a single js code block but the js property of the userScripts.execute() function allows an array of userScripts.ScriptSource wherefore multiple code blocks can be provided with a single call.

Any suggestion on how I should deal with this? E.g. through multiple calls to tab.queryContent("Execute", options); or should I alter the handleActorExecute function for which in turn I have to change more code down the line like https://searchfox.org/firefox-main/source/toolkit/components/extensions/ExtensionContent.sys.mjs#422

Assignee: nobody → robbendebiene
Status: NEW → ASSIGNED

I pushed my first iteration for a first review to get a better feeling whether I'm on the right track.

I'm working on the test cases now.

I'm unsure whether I should extend the test_ext_scripting_executeScript_XXX.html tests to also test the userScripts.execute() API or create completely separate test files. The APIs share a lot of code so integrating them makes sense, but on the other hand it will bloat/complicate the testcases further and prevent potential separation in the future. I attached an example where I integrated the userScripts.execute() test into test_ext_scripting_executeScript_activeTab.html. Are there any guidelines for this?

Flags: needinfo?(rob)

(In reply to robbendebiene from comment #9)

handleActorExecute currently only accepts a single js code block but the js property of the userScripts.execute() function allows an array of userScripts.ScriptSource wherefore multiple code blocks can be provided with a single call.

Any suggestion on how I should deal with this? E.g. through multiple calls to tab.queryContent("Execute", options); or should I alter the handleActorExecute function for which in turn I have to change more code down the line like https://searchfox.org/firefox-main/source/toolkit/components/extensions/ExtensionContent.sys.mjs#422

The current script injection logic executes script files first, then optionally a code string (see getCompiledScripts). In practice, these are mutually exclusive:

  • code and file are mutually exclusive in tabs.executeScript
  • func and file are mutually exclusive in scripting.executeScript (func and args are serialized and ultimately assigned to jsCode)

In the userScripts.execute API, they can both be specified, an arbitrary number of times. Your patch tries to implement support by populating the jsPaths and (new) jsCode arrays, but the bug there is that the code would not execute in the specified order, which I'd consider to be a bug.

Since ordering is important, I recommend exploring a way to get js to contain the scripts that need to execute. Right now, there are two distinct code paths:

  • for MAIN world injection, a code string is turned into a data:-URL, compiled asynchronously and executed. This is mainly to have a work-around to bypass CSP.
  • In ISOLATED worlds (and there is a TODO comment to do something similar for USER_SCRIPT worlds), code is evaluated with Cu.evalInSandbox. The latter is synchronous.
  • For userScripts.register, we compile code to blob:-URLs (at #makeInternalUserScript) and pass it to jsPaths.

For simplicity, it is feasible for userScript.execute to unconditionally go through the "generate URL and compile" approach.

There are a few things to account for:

  • We don't want to store huge URLs as the key in the script cache. E.g. if you were to populate jsPaths with a data:-URL and rely on the existing implementation, then getCompiledScripts would populate the script cache with a long URL.
  • In the same theme, if we use one-off blob:-URLs, we don't want to keep them in the cache either, because nothing would ever hit the cache.
  • handleActorExecute's error handling logic assumes that if an error occurs while cloning the result of a script execution, that it is in the last file or else a code snippet (source. Depending on your implementation I can see you ending up with a data:-URL there; make sure that it doesn't happen.

There is more, but I expect that to come up naturally during code review if needed.

(In reply to robbendebiene from comment #14)

I'm unsure whether I should extend the test_ext_scripting_executeScript_XXX.html tests to also test the userScripts.execute() API or create completely separate test files. The APIs share a lot of code so integrating them makes sense, but on the other hand it will bloat/complicate the testcases further and prevent potential separation in the future. I attached an example where I integrated the userScripts.execute() test into test_ext_scripting_executeScript_activeTab.html. Are there any guidelines for this?

I suggest creating a separate test file, as that would be easier to run, and intermittent test failures are easier to classify/attribute and separate.

Flags: needinfo?(rob)
Attachment #9584258 - Attachment is obsolete: true
See Also: → 2041680
See Also: → 1824896
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 153 Branch
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: