w3c / webextensions

Charter and administrivia for the WebExtensions Community Group (WECG)
Other
576 stars 50 forks source link

Request immediate injection of manifest script registered with `document_idle` #600

Open fregante opened 2 months ago

fregante commented 2 months ago

Context

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

fregante commented 2 months ago

Related

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

xeenon commented 2 months ago

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.

Rob--W commented 2 months ago

Relevant previous discussions:

Rob--W commented 2 months ago

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

fregante commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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