Closed Bug 1509339 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox65 verified, firefox66 verified)

VERIFIED FIXED
mozilla65
Iteration:
65.4 - Dec 3-9
Tracking Status
firefox65 --- verified
firefox66 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

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 files)

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: nobody → lgreco
Blocks: 1437098
Status: NEW → ASSIGNED
Iteration: --- → 65.4 - Dec 3-9
Priority: -- → P2
Duplicate of this bug: 1498739
User Story: (updated)
Blocks: 1445909
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 ...
(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).
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.
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: () => {}
    ...
}
(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.
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
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)
(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)

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.