w3c / webextensions

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

chrome.scripting.executeScript doesn't resolve/reject for `frozen` state tabs #527

Open piyu-sh opened 5 months ago

piyu-sh commented 5 months ago

Prior context: I originally raised an issue here https://bugs.chromium.org/p/chromium/issues/detail?id=1429905 then started a thread on google groups where @oliverdunk suggested to open an issue here

Problem: When trying to run chrome.scripting.executeScript over tabs that are in frozen state, the promise doesn't return, neither it resolves nor rejects. The operation is queued, and when the tab is unfrozen, then the operation runs. This is problematic, because a developer has no way of knowing that a tab is frozen, so they might try running an operation on such a tab but will get stuck because executeScript will just get stuck.

Suggestions on handling this:

  1. resolve/reject from executeScript and throw an error just like executeScript throws error for unloaded/discarded tabs
  2. provide a field like isFrozen in Tab Object which is returned from chrome.tabs.query() so that user can decide what to do with such tabs
maxnikulin commented 5 months ago

I like the idea to be able to determine if a script can be executed in the particular tab. Besides tabs.query() it would be handy to have similar fields in Tab objects passed to action.onClicked and contextMenus.onClicked event listeners.

I am unsure however that it is always possible. A tab may be busy in a tight loop executing some page script function. In general it is susceptible to race conditions since tab state may change between tabs.query() and sctipting.executeScript() calls.

To handle scripting.executeScript() issues I suggested to add an AbortSignal parameter, see #415.

It seems there are 2 different use cases for scripting.executeScript():

To make second use case an atomic operation, it is necessary to add a scripting.executeScript() parameter determining execution policy.

Rob--W commented 4 months ago
  1. resolve/reject from executeScript and throw an error just like executeScript throws error for unloaded/discarded tabs

I'd be willing to specify this behavior more clearly, and/or change the default behavior based on options.

  1. provide a field like isFrozen in Tab Object which is returned from chrome.tabs.query() so that user can decide what to do with such tabs

Depending on what "frozen" means, this is prone to race conditions and would also require potentially expensive bookkeeping on the browser's end.

fregante commented 4 months ago

Frozen just means that JavaScript thread is busy. It "freezes" by definition of being single-threaded, so what you're asking is essentially to have a timeout:

When adding code to execute to the event loop, via DOM events or via setTimeout, you basically expect the code to "run eventually". I'm not sure that this situation is substantially different. You can implement your own timeout or perhaps add support for signal in executeScripts

oliverdunk commented 4 months ago

Frozen just means that JavaScript thread is busy.

In this case it's a discrete state the browser can use: https://developer.chrome.com/docs/web-platform/page-lifecycle-api#states. I don't know if there are internal timeouts but theoretically, it would be possible for it to remain in this state for a long time and then for the script to execute when it is eventually restored.

Depending on what "frozen" means, this is prone to race conditions and would also require potentially expensive bookkeeping on the browser's end.

Agreed on the race condition concern, and a new property on APIs like tabs.query would only resolve things for developers who go out of their way to handle this state. I don't think bookkeeping is a problem (at least in Chrome) since we show the state at chrome://discards/.

Also I just wanted to clarify - I'm not a huge fan of the behaviour today. I mentioned in the mailing list that the current behaviour might be ok, but I think that was bad phrasing on my part. What I wanted to call out was that this isn't necessarily a bug, since the request wasn't being lost and did have a chance of eventually being handled. I agree it isn't particularly useful behaviour though (at least, I don't think so).

fregante commented 4 months ago

Browsers freeze pages as a way to preserve CPU/battery/data usage

Oh, I didn't realize this. Since this is intentional, the browser should really just thaw the tab. The same thing happens when you message the worker: it wakes up after the browser deactivates it.

Also yes, since this is an official "state" of the tab, then probably it should be exposed the same way "discarded" is

piyu-sh commented 4 months ago
  1. provide a field like isFrozen in Tab Object which is returned from chrome.tabs.query() so that user can decide what to do with such tabs

Depending on what "frozen" means, this is prone to race conditions and would also require potentially expensive bookkeeping on the browser's end.

Hmm, agreed on the race condition part, but can this race condition not happen currently for discarded tabs as well, then why allow it for a certain state and not for other?

  1. resolve/reject from executeScript and throw an error just like executeScript throws error for unloaded/discarded tabs

I'd be willing to specify this behavior more clearly, and/or change the default behavior based on options.

Would adding a flag like failIImediately to executeScript's ScriptInjection help? This flag when true rejects the promise early if tab is frozen instead of queing the task.