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

Request immediate injection of manifest script registered with document_idle #600

Open
fregante opened this issue Apr 25, 2024 · 8 comments
Labels
follow-up: chrome Needs a response from a Chrome representative needs-triage: chrome Chrome needs to assess this issue for the first time supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari

Comments

@fregante
Copy link

Context

  • I have a content script registered on *://*/* and run_at: document_idle.
  • I also use the Commands API to run scripts via keyboard shortcuts, context menus, browser action clicks, etc

Problem

On larger websites, like nytimes.com, the page (and therefore the content script) takes several seconds to load. This is fine with regular usage, but if the user activates the extension, the content script cannot respond (or even receive messages) until the browser loads it.

I could use the Scripting API to manually inject it, but the browser will follow it with its own injection, or maybe cause race conditions.

Possible solutions

  • add a manifest key to enable immediate injection (e.g. run_on_active_tab: true)
  • add a method to request a specific content script to be injected immediately, without duplicating the native injection (e.g. scripting.ensureScript(3), where the number is the index in the manifest)
@github-actions github-actions bot added needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time labels Apr 25, 2024
@fregante
Copy link
Author

Related

This is somewhat related to the ability to determine when a tab is ready to receive messages:

@xeenon
Copy link
Collaborator

xeenon commented Apr 25, 2024

Safari does not currently support document_idle, it just maps to document_end.

In general I would be supportive of something like this, we do have the support to do this internally in WebKit at least.

@xeenon xeenon added supportive: safari Supportive from Safari and removed needs-triage: safari Safari needs to assess this issue for the first time labels Apr 25, 2024
@Rob--W
Copy link
Member

Rob--W commented Apr 25, 2024

@Rob--W
Copy link
Member

Rob--W commented Apr 25, 2024

Why don't you use document_end instead? You can use regular DOM APIs to detect load completion and run your desired logic.

@fregante
Copy link
Author

@Rob--W I don’t think that resolves the issue, the script might still load long after the user called/activated the extension. I could use document_start but that would impact performance even when not immediately needed.

@tophf
Copy link

tophf commented Apr 26, 2024

Maybe "run_at": "document_idle_or_extension_invoked" with the same heuristics for user gesture as the activeTab permission?

And something like "run_at": "extension_invoked" to inject only on user gesture in the active tab?

@Rob--W, an example of why document_end may take a very long time would be a slow network e.g. a spotty WiFi signal.

@dotproto
Copy link
Member

dotproto commented May 9, 2024

At the moment, the approach I find most compelling is @fregante's suggestion that we introduce a new run_on_active_tab boolean on the content script declaration object.

I'm also interested in the idea of exposing a runtime API to let extensions request injection of a given content script, but I'm a bit uncomfortable with the suggestion that we do so based on content script declaration indices. First, this approach only works with content scripts declared in the extensions manifest; runtime declarations via scripting.registerContentScripts() or scripting.updateContentScripts() wouldn't be accessible with this approach. Second, while array indices are static for content scripts declared int he manifest, they are more difficult to reason about than a unique identifier. I expect that this approach would lead to subtle bugs and it would make extension source more difficult for both extension developers and reviewers to reason about.

I think a better approach would be to add an id field to the content script declaration object. A unique identifier could be used by both statically and dynamically declared content scripts, be easier for developers to reason about, be less likely to result in unexpected bugs when changing only a manifest, be more predictable and easier to work with in situations where the manifest is generated at build time.

// manifest.json
{
  "content_scripts": [{
    "id": "main",
    "matches": ["*://*.example.com/*"],
    "scripts": ["content.js"],
    "run_at": "document_idle"
  }]
}

// background.js
browser.action.onClicked.addListener((tab) => {
  scripting.executeScript({
    target: { tab: tabId },
    scriptId: "main"
    // "files" and "func" omitted because the appropriate value would be reused
    // by from the "main" content script's declaration.
  });
});

I'm also concerned that introducing a document_idle_or_extension_invoked value for run_at would set a president of combining existing phases with _or_extension_invoked which could lead to a combinatorial explosion if we expand the set of supported lifecycle injection points or new methods of triggering script injection.

One way to combat this is to allow run_at to accept an array of lifecycle phases (for example, "run_at": ["document_idle", "extension_invoked"]). That said, I'm somewhat hesitant about this approach because it only seems useful in combination with extension_invoked. Other potential uses of this array feel like footguns; for example, what would the expected behavior of "run_at": ["document_idle", "document_start"] be? Inject at each distinct phase? Only inject on the first? The browser errors on manifest parse because that declaration doesn't make senes? IMO the design of the API should try to discourage usage patterns that we don't expect or want.

@Rob--W Rob--W added follow-up: chrome Needs a response from a Chrome representative supportive: firefox Supportive from Firefox and removed needs-triage: firefox Firefox needs to assess this issue for the first time labels May 9, 2024
@Rob--W
Copy link
Member

Rob--W commented May 9, 2024

This topic was discussed in today's meeting (meeting notes pending review before merging in #608).

This specific part of the proposal was discussed in more detail:

add a method to request a specific content script to be injected immediately, without duplicating the native injection (e.g. scripting.ensureScript(3), where the number is the index in the manifest)

I expressed concerns over this capability in isolation, because the currently expected behavior is for the script to execute whenever the API is called. This could be countered by introducing a way for extension authors to explicitly signal that a content script should run once per page. If that capability were to exist, then it can be combined with the existing executeImmediately flag of the scripting.executeScript API to execute immediately.

FYI: In Firefox, there is currently no deduplication of already-injected scripts. In Chrome, content scripts are currently run only once even when specified multiple times. This is even the case if the scripts target different worlds, which someone reported before at https://issues.chromium.org/issues/324096753 ("Shared content script in different worlds conflicts with each other")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up: chrome Needs a response from a Chrome representative needs-triage: chrome Chrome needs to assess this issue for the first time supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

5 participants