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

Adding a temporary listener just for the current run of the service worker or event page so it doesn't wake up next time for this event #719

Open
tophf opened this issue Nov 2, 2024 · 7 comments
Labels
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

Comments

@tophf
Copy link

tophf commented Nov 2, 2024

The problem I'm trying to solve is that addListener called from a "lazy" background script (an MV3 service worker or an event page) sets an internal flag in the browser to wake up the background script next time even if I add the listener asynchronously (i.e. not during the startup of the background script), which may be wasteful in case the extension chose to recreate its state fully, for example because it's faster or more reliable than reading the saved state from indexedDB or chrome.storage.session, applying the difference, and finally saving the result to storage yet again.

There's also no way to remove the listener reliably right before the service worker terminates.

The events I have in mind are chrome.tabs.onRemoved and onReplaced, which I use in a userstyle extension to track which tab ids should be removed from my data map that I use to avoid calling chrome.tabs.get for each iframe that loads in an existing tab or when broadcasting to tabs that had been injected with the userstyles. Ideally I wouldn't want to keep hundreds of ports open just to track hundreds of tabs and frames. Instead, each time the extension wakes up for a webNavigation or a webRequest event for this frame's document it calls chrome.tabs.query, which illustrates there's no need to wake up for those auxiliary onRemoved/onReplaced events.

My workaround for the reported problem is that... I stopped using these events altogether and switched to reacting to an API error when sending a message to a missing tab, which should be rare hopefully.

Here's my idea:

  • chrome.tabs.onRemoved.addEagerListener(fn) as an antonym to the "lazy listener" concept used in Chromium's source code.
@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 Nov 2, 2024
@tophf
Copy link
Author

tophf commented Nov 3, 2024

OTOH, the current Chromium's behavior is conceptually a bug, because an asynchronously added listener cannot hear the event next time the SW wakes up.

@polywock
Copy link

polywock commented Nov 5, 2024

I don't know the specifics, but with adding a listener asynchronously, wouldn't it just wake up once in excess? As long as you don't keep adding the listener, it shouldn't be that wasteful.

@tophf
Copy link
Author

tophf commented Nov 5, 2024

It's necessary every time the extension wakes up, so it'll be an eternal problem.

@polywock
Copy link

polywock commented Nov 5, 2024

Ah, that makes sense.

@dotproto
Copy link
Member

dotproto commented Nov 8, 2024

In abstract I'm in favor of providing extensions with a way to register an event listener in an evented background context that won't wake the background context. The behavior described in the original issue is Chromium-specific. In Firefox and Safari event listeners registered a short period after the background context starts will not wake the context in the future.

As @xeenon said in today's meeting, Firefox and Safari have their own heuristics for this at the moment, but ideally the event listener registration call give developers a way to signal their intent. IMO it would be ideal if we could provide this kind of signal in an options object passed into the event's addListener() call, but given the current API surface I don't think that's reasonable.

I'm not entirely comfortable with introducing a new event listener registration method that's identical to addListener() except it explicitly signals that it shouldn't wake the context when the context idles. Besides largely duplicating existing functionality, it also doesn't provide us with much in the way of flexibility for future evolution.

I'd be more inclined to introduce a successor to addListener() – let's call it registerListener() (bikeshedable) – that takes a callback and an options object that we can leverage in the future. This object could subsume all other arguments that can be passed into the event listener registration call, like filter or extraInfoSpec. With this addition, we would have a single place to expose a new Boolean persist (bikeshedable) field that specifies whether the event listener registration should persist across context lifetimes. This would also enable us to add support for nice-to-have concepts like once or signal from the web platform.


@tophf, I'm curious about a couple of details you shared in the original issue description.

First, you mentioned that you call tabs.query "each time the extension wakes up for a webNavigation or a webRequest event for this frame's document." Are you using a UrlFilter on these events to limit wakeups to top level navigation on sites with userstyles? It sounds like you are, but I wanted double check. If so, I assume you're registering an unfiltered listener synchronously and later replacing it with a filtered listener once you've loaded the parameters from storage. If you're doing something else I'd be curious to learn more.

Second, you mentioned that you maintain a data map "to avoid calling chrome.tabs.get for each iframe that loads in an existing tab". I don't follow why you would call chrome.tabs.get() for each iframe. Is that a consequence of using webNavigation or webRequest events as your primary signal?

Based on your description, I assume that the motivating use case here is to update the injected userstyles in each relevant frame. I also assume that you don't want to lazily update styles as the user switches between tabs because that would result in a flash of incorrectly styled content because of the delay required for lazy update propagation.

If I'm on the right track, perhaps another approach is to eagerly update a subset of tabs/sub-frames based on what's currently visible to the user (browser.tabs.query({active: true})) and to perform a more expensive update on all frames a good moment, like as when a style editing session ends. That might allow you to avoid the boo keeping for tracking style injection contexts and reduce the total number of background context startups required.

@polywock
Copy link

polywock commented Nov 8, 2024

In Firefox and Safari event listeners registered a short period after the background context starts will not wake the context in the future.

For Firefox, there's a minor exception: if you add the listener initially, remove it, and then re-add it asynchronously, it will register for future wake-ups. https://bugzilla.mozilla.org/show_bug.cgi?id=1869125

@tophf
Copy link
Author

tophf commented Nov 8, 2024

@dotproto, I need the map to get the tab's url for its iframes quickly to avoid the possibly long delay to perform the call to chrome.tabs.get when a lot of things are happening during a page load and possibly many extensions are making such or similar calls. This scenario (accumulating data for the iframe's container or tab in a variable) is useful for many extensions including ad blockers.

This map is also used to broadcast just to known styled tabs when the user changes a style, there's some other info as well, which is why I want to use tabs.onRemoved to avoid wasting memory. In my case the waste is small though so I can even resort to doing a periodical cleanup in case the background script is not unloading for a long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

No branches or pull requests

3 participants