Implement an userScript.onBeforeScript API event and provide a userScript API object to the listeners

VERIFIED FIXED in Firefox 65

Status

enhancement
P2
normal
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks 2 bugs)

unspecified
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 verified, firefox66 verified)

Details

User Story

As a brief summary of the changes to the userScripts API part of this bugzilla issue:  

- add a new userScripts.onBeforeScript API event:
  - only available as a "content child process"-only API (as we did for the userScripts.setScriptAPIs)
  - fires as soon as the userScript sandbox has been created (but before any userScript code had any chance to be executed into it)
  
- the event handlers subscribed to this new API event receive a userScript API object with the following properties:

  - userScript.metadata (which provide access to the metadata associated to the userScript in the userScripts.register API call)

  - userScript.global (which provide access to the isolated sandbox where the userScript code is going to be evaluated)

  - userScript.defineGlobals({...}) (which is similar to the current userScripts.setScriptAPIs API method, but it can also create API methods nested in an API object, 
    as it is going to be needed by greasemonkey to provide the global GM object: https://wiki.greasespot.net/Greasemonkey_Manual:API)  

  - userScript.export(value) (which can be used by the API methods exported to the userScript sandbox to create new "API objects" to pass or return to the userScript code)

- remove the userScripts.setScriptAPIs (as it is replaced by userScript.defineGlobals)

Follows a small snippet which shows how an API script looks like with these changes to the userScripts API:

browser.userScripts.onBeforeScript.addListener(userScript => {
  const ScriptError = userScript.global.Error;

  userScript.defineGlobals({
    GM: {
      info: {
        scriptHandler: "MyUserScriptsManager",
        version: "0.1",
        script: userScript.metadata,
        ...
      },
      async deleteValue() {
        ...
        if (/*something went wrong*/) {
          throw new ScriptError("Something went wrong");
        }
        ...
      },
      ...
      async anotherAPIMethod(cb) {
        const cbParam = userScript.export({
          somedata: "value",
          async myFn() { ... }
        });

        cb(cbParam);
      }
    },
  });
});

Attachments

(5 attachments)

Assignee

Description

7 months ago
During the review rounds on the patches attached to Bug 1498739, we (:robwu, :zombie and I) have identified some limitations/issues in the current API design, and we have been discussing (quite a lot) about them to finally agree on which changes we would like to apply to the userScripts API design.

The goal of this bug is to implement the changes to the userScripts API design that we agreed on.

These changes are only impacting the "content child process"-only part of the userScript API namespace (in other words no changes to the userScripts.register available to the extension pages).
Assignee

Updated

7 months ago
Assignee: nobody → lgreco
Blocks: 1437098
Status: NEW → ASSIGNED
Iteration: --- → 65.4 - Dec 3-9
Priority: -- → P2
Assignee

Updated

7 months ago
Duplicate of this bug: 1498739
Assignee

Updated

7 months ago
User Story: (updated)
Assignee

Updated

7 months ago
Blocks: 1445909

Comment 6

7 months ago
Regarding userScript.defineGlobals(), are these done in addition to browser.userScripts.setScriptAPIs() or in its place?!

In order to define both types of APIs, it would be good to define one type and then map the other to the defined one. That is to say, for example,

browser.userScripts.setScriptAPIs({

  async GM_getValue([key], scriptMetadata, scriptGlobal) {

    // .....
  },
//...


An then something like

const GM = {
  getValue: GM_getValue,
  // .....
};

Or vice versa ...
Assignee

Comment 7

7 months ago
(In reply to erosman from comment #6)
> Regarding userScript.defineGlobals(), are these done in addition to
> browser.userScripts.setScriptAPIs() or in its place?!

As I wrote above in the "User Story" field, the plan is that (as part of these changes) we are also removing browser.userScripts.setScriptAPIs (as it is replaced by userScript.defineGlobals), and so that is what the attached patch are doing.

I've also discussed a bit about this with Rob, in my opinion "having both" would be more confusing than helpful, and I feel that we should avoid to have two different API methods to do basically the same thing (but in a sightly different way).

Comment 8

7 months ago
Some time ago there was a comment by Rob [Bug 1445909,comment 6] regarding the performance implications of using a callback with the UserScript APIs. If this event is triggered for each registered userscript on every page load then would this cause a noticeable impact on page loads or script timings? While I like the flexibility that the event brings I'm a bit concerned about whether it will cause scripts to execute later and potentially miss the requested time window (say, document_start). An issue that Greasemonkey currently struggles with.

Comment 9

7 months ago
Out of curiosity .... is it not possible to define userScript.defineGlobals() globally?!

What I meant was, extension using the API would register/define defineGlobals() on install, the same way (but not necessarily on manifest.json)an extension registers "commands" globally on install.

The userScript_defineGlobals would only be available to the extension (not another extension) and only to the userScripts injected by the extension (not by another extension). I would imagine limiting it to the extension domain (similar to storage and other APIs) should be a possibility.
Maybe have a sub-domain for userscripts!!!?

This way, API can be registered once on install/enable and no need to use a per script/per page onBeforeScript.addListener()

For example:

  "user_scripts": {
    "api_script": "customUserScriptAPIs.json"
  }

Then:

{
    GM: {
      info: {
        scriptHandler: "MyUserScriptsManager",
        version: "0.1",
        script: userScript.metadata,
        ...
      },
      async deleteValue() {
        ...
        if (/*something went wrong*/) {
          throw new ScriptError("Something went wrong");
        }
        ...
      },
      ...
      async anotherAPIMethod(cb) {
        const cbParam = userScript.export({
          somedata: "value",
          async myFn() { ... }
        });

        cb(cbParam);
      }
    },
    GM_info: () => {}
    ...
}

Comment 10

7 months ago
(In reply to sdaniele3 from comment #8)
> Some time ago there was a comment by Rob [Bug 1445909,comment 6] regarding
> the performance implications of using a callback with the UserScript APIs.

That comment is about callbacks being triggered in the background page.
In this new API, the execution flow is approximately:
- Extension declares API script as a string in the user_scripts.api_script field in manifest.json
- Background page registers user scripts for some documents.
- When a user script matches a URL, the API script is executed (as if it were a content script) if it was not executed before.
  In this script, the userScripts.onBeforeScript event is fired.
- If the event is not canceled (which we consider supporting to support bug 1445909), the user script is executed.

> If this event is triggered for each registered userscript on every page load
> then would this cause a noticeable impact on page loads or script timings?

The event is only fired when the given URL pattern matches. User script managers such as Greasemonkey are advised to choose a minimal list of URL patterns.

> While I like the flexibility that the event brings I'm a bit concerned about
> whether it will cause scripts to execute later and potentially miss the
> requested time window (say, document_start).

This API has similar timing characteristic as the contentScripts.register API. Once registered, the script is executed equally soon as scripts that were declared in manifest.json.

Comment 11

7 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/7fdaa201c7b3
Split userScript's apiScript methods test into smaller xpcshell-test tasks. r=robwu
https://hg.mozilla.org/integration/autoland/rev/39b6008cb9cf
Implement a userScript.onBeforeScript API event. r=zombie,robwu
https://hg.mozilla.org/integration/autoland/rev/541cb39b6323
Implement a UserScript API object and remove the userScripts.setScriptAPIs method. r=zombie,robwu
https://hg.mozilla.org/integration/autoland/rev/911cad5eea69
Support exporting apiScript arrays using the UserScript's export API method. r=zombie,robwu
Assignee

Comment 13

6 months ago
Hi Victor,
the attached file contains an updated version of the test extension attached to Bug 1437861 comment 44, changed to use the new API design as implemented in this issue.

As agreed over IRC, this updated test extension is meant to be used to verify this bug (and the updated userScripts API design) on the test scenarios as listed in Bug 1437861 comment 44.
Flags: needinfo?(vcarciu)

Comment 14

6 months ago
(In reply to Luca Greco [:rpl] from comment #13)
> Created attachment 9031903 [details]
> user-script-register-example-v2.xpi
> 
> Hi Victor,
> the attached file contains an updated version of the test extension attached
> to Bug 1437861 comment 44, changed to use the new API design as implemented
> in this issue.
> 
> As agreed over IRC, this updated test extension is meant to be used to
> verify this bug (and the updated userScripts API design) on the test
> scenarios as listed in Bug 1437861 comment 44.

Thank you Luca. 

I tested all scenarios using the new extension attached and everything looks fine using latest FF Nightly and latest FF65 on both Win10x64 and macOS 10.13.6. You can see the results at https://testrail.stage.mozaws.net/index.php?/reports/view/1339.

I will mark the issue as verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(vcarciu)

Comment 15

5 months ago

I've been testing out the API and something that may have been overlooked, unless it's planned for another ticket, is the ability to reject / cancel a userscript from within the onBeforeScript event.

You need to log in before you can comment on or make changes to this bug.