Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Multiple user script worlds #560

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

rdcronin
Copy link
Contributor

@rdcronin rdcronin commented Mar 8, 2024

This adds a new API proposal for supporting multiple user script worlds, allowing user script managers to better isolate individual user scripts when multiple may be injected on a given site.

This proposal follows the new API proposal process.

This adds a new API proposal for supporting multiple user script worlds,
allowing user script managers to better isolate individual user scripts
when multiple may be injected on a given site.

This proposal follows the new API proposal process.
@rdcronin rdcronin requested review from xeenon, Rob--W and mukul-p March 8, 2024 01:24
@rdcronin
Copy link
Contributor Author

rdcronin commented Mar 8, 2024

@Rob--W @xeenon @mukul-p please take a look (or delegate) on behalf of the other browsers.

@EmiliaPaz , FYI, as this (somewhat) relates to your upcoming work on userScripts.execute().

@tophf FYI, as well, given your interest and contribution in previous user script discussions.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the feedback below, I'd also like to point out that the runtime.Port and runtime.MessageSender API definitions should be extended with a userScriptWorldId property to enable extensions to distinguish user script worlds from each other.

proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rdcronin rdcronin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Rob and Timothy!

@Rob--W , good callout on the new properties on runtime.Port and runtime.MessageSender; I've added those as well. (Github won't let me reply to your comment there directly...)

proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
@rdcronin
Copy link
Contributor Author

@Rob--W friendly ping : )

@rdcronin
Copy link
Contributor Author

@Rob--W , back to you!

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to sign off, but upon doing a last pass where I examined the impact on individual API methods, I got one new question. Let's resolve that and then this should be good to merge.

```diff
export namespace userScripts {
export interface WorldProperties {
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
+ /**

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

`register()`, and others are left unchanged.

When the developer specifies a `worldId` in either the `WorldProperties` or
a `RegisteredUserScript`, the browser will create a separate user script world
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important design question: what are the persistence/lifetime characteristics of worldId + configureWorld? worldId should not persist across extension updates for consistency with the existing userScripts API design (https://developer.chrome.com/docs/extensions/reference/api/userScripts#extension_updates), but there is still an open question of the lifetime of worldId (and consequently, potentially reduce the duration of persistence).

Previously, configureWorld would have a limited scope, exactly one. Now, with the introduction of worldId, there may be an unbounded number. According to the current proposal, the concept of a specific world (identified by worldId) becomes concrete whenever it is used in RegisteredUserScript or WorldProperties. Because the number of worlds is now dynamic, we should clarify when a worldId exists and when it ceases to exist. As currently written, worldIds are instantiated and never discarded.

This section seems to have been written with userScripts.register in mind. The issue that I'm thinking of would have become much more obvious if the proposal explicitly spelled out the expected behavioral change (or non-change) per method (from the proposal process perspective - I think that it would be a good idea for future proposals to exhaustively enumerate all uses of a type to make sure that the behavior is fully specified where needed):

  • userScripts.register() - may introduce a worldId. Does not necessarily have to do anything until the script is used in the renderer (unless there is a desire to broadcast all known worldIds?).
  • userScripts.configureWorld() - may create a persistent configuration for a world. What if the worldId does not exist yet?
  • userScripts.getScripts() - does this have to "create a separate user script world"?
  • userScripts.unregister() - when the last use of worldId disappears, would that delete the configuration?
  • userScripts.update() - when the worldId changes, and the previous one was the last one, does that delete the registered configured worlds?

I think that there are roughly two ways to address this issue (not mutually exclusive):

  1. Explicit world management (userScripts.registerWorld + userScripts.unregisterWorld). Using an unregistered worldId in RegisteredUserScript or WorldProperties is an error.
  2. Automatic world cleanup: a world configuration is guaranteed to persist when there is at least one RegisteredUserScript that references it. What happens when there is no reference to it is more interesting:
    • When there are no references, we could unregister the world as soon as the refcount drops to zero would be obvious (by userScripts.unregister or userScripts.update).
    • Do we want to support calling userScripts.configureWorld before userScripts.register? If so, then configureWorld should register a world implicitly, despite there not being any scripts that reference it.
      • to avoid accumulating junk, unused worlds should eventually be discarded. This is non-trivial.
      • an alternative is to enable the world to be configured along with registering a user script.
      • another alternative is to not support worldId in configureWorld unless the worldId has been allocated explicitly by userScripts.register. Extensions that really care about ensuring setup of the world before execution can register a dummy script with a worldId, then configure the world, then register the actual script for that world, and then unregister the dummy script. Is this an acceptable pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Rob--W !

I was thinking about most of these already (complete with a few TODOs, e.g. here) and was planning on submitting an update to the proposal as we got nearer, but I hadn't considered the update behavior and allowing developers to clear out old registrations -- both of which I agree are valuable.

The way I'm thinking about worlds is that:

  • A user script is registered with a world. That determines where it injects.
  • The configuration for that world may or may not exist, depending on whether the developer wants to use non-default values (for messaging and CSP, but potentially other APIs in the future).

As such, there isn't really the "creation" of a world when you register a script -- worlds are created in the document in which they run, and are created according to the configuration provided (if any).

I don't like the idea of either reference counting or requiring a user script be registered for every specified worldId -- that sounds very difficult for both us (the browser) and for developers to keep track of, and seems like it would very likely lead to some unpredictable or unexpected edge cases (e.g., a developer updates a user script world via unregister + register, and doesn't realize that unregistered the user script world). It also leads to a (very small) race where it'd be impossible to ensure a user script injected with given world properties, since you'd have to register the script before the configuring the world, but then the script may inject before that configuration took hold. I'd much rather lean on the developers handling the registration.

I've expanded the proposal to include:

  • Two new functions, removeWorld() and getWorldConfigurations().
  • Notes about how the worlds behave when injecting.
  • Notes on world persistence and world limits.

Please take a look and let me know what you think.

Expand the proposal to include:
- Two new functions, `removeWorld()` and `getWorldConfigurations()`.
- Notes about how the worlds behave when injecting.
- Notes on world persistence and world limits.
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Show resolved Hide resolved
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, this proposal looks like it is in a good shape.

I would encourage solliciting feedback from developers of User Script managers, perhaps timeboxed to e.g. a week or two, to make sure that the API works for them without unsurmountable issues.

proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
proposals/multiple_user_script_worlds.md Show resolved Hide resolved
@rdcronin
Copy link
Contributor Author

I think that's everyone!

@Rob--W , is this good to merge in?

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, I'll suggest a change and then use Github's UI to make the change before merging.

proposals/multiple_user_script_worlds.md Outdated Show resolved Hide resolved
@Rob--W Rob--W merged commit 5869199 into w3c:main Apr 26, 2024
3 checks passed
github-actions bot added a commit that referenced this pull request Apr 26, 2024
SHA: 5869199
Reason: push, by Rob--W

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 26, 2024
SHA: 5869199
Reason: push, by Rob--W

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to patrickkettner/webextensions that referenced this pull request May 2, 2024
SHA: 5869199
Reason: push, by patrickkettner

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to patrickkettner/webextensions that referenced this pull request May 2, 2024
SHA: 5869199
Reason: push, by patrickkettner

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
carlosjeurissen pushed a commit to carlosjeurissen/webextensions that referenced this pull request Jun 25, 2024
* Proposal: Multiple user script worlds

This adds a new API proposal for supporting multiple user script worlds,
allowing user script managers to better isolate individual user scripts
when multiple may be injected on a given site.

This proposal follows the new API proposal process.

* Address Rob's and Timothy's feedback

* Update diff styling

* Collapse code sections since diffing makes it clear

* Expand API for better world management

Expand the proposal to include:
- Two new functions, `removeWorld()` and `getWorldConfigurations()`.
- Notes about how the worlds behave when injecting.
- Notes on world persistence and world limits.

* Address Rob's latest comments

* Address more review comments

* Fix trailing parenthesis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants