w3c / webextensions

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

Proposal: add new `browser.tabs.waitForTab(tabId: number)` that will wait for tab to load #591

Open Juraj-Masiar opened 2 months ago

Juraj-Masiar commented 2 months ago

The issue is always the same, but every time slightly different:

  1. I have a code running in one of my contexts (background script, tab, popup, sidebar...)
  2. I want to open new window/tab/page/popup/sidebar (something that loads HTML and JS), or update URL in existing tab.
  3. from my original context, I want to send it some message / execute script / use some API using tabId.

And this is one huge race condition, because some API calls in some browsers will fail if the page is not loaded (example, example).

Existing workarounds are complex:

  1. let the opened page ask for work - this is a spaghetti code solution, because the workflow started in another context that may want to await the job execution and even get a result.
  2. let opened page send message to the caller that it's ready for work - if programmed right, it can work fine (except when updating tab URL), but it requires a lot of code.
  3. use tabs.onUpdated to watch for status "complete" event - again, a lot of code needed and was very buggy in the past (for example "complete" fired twice in some cases).
  4. use webNavigation.onCompleted is similarly complex plus requires webNavigation permission

The proposed, browser.tabs.waitForTab API would solve all this in one line, example:

const tab = await browser.tabs.create({url: '/quetions.html'});
await browser.tabs.waitForTab(tab.id);
const response = await browser.tabs.sendMessage(tab.id, {type: 'ask_user_for_something'});

There could be a second optional parameter in this API that would specify, whether to await the DOMContentLoaded or load event. With DOMContentLoaded being a default.

And if the tab is not in the "loading" state, it would resolve right way.

Alternative solution 1: Updating specification for the tabs.create, tabs.update, windows.create and similar API, to require them to resolve the returned promise only after the DOMContentLoaded event.

Alternative solution 2: Updating specification for scripting.executeScript, tabs.sendMessage and similar, to require them to await tab getting ready before executing them.

fregante commented 2 months ago

This is a common issue: When is a tab ready to be messaged?

The best solution at this point appears to be: sendMessage on repeat until it's successful. This is not great.

I think a better solution for messaging could be:

const tab = await browser.tabs.create({url: '/quetions.html'});
const response = await browser.tabs.sendMessage(
  tab.id,
  {type: 'ask_user_for_something'},
  {waitForListener: true} // 👈 
);

This way the message would wait indefinitely until the content script called runtime.onMessage.addListener, or until the tab is closed (throwing an error)

Messaging as a whole is painful and I think it needs to be reevaluated. The worst offender is runtime.sendMessage acting as a broadcast

tophf commented 2 months ago

a second optional parameter in this API that would specify, whether to await the DOMContentLoaded or load event. With DOMContentLoaded being a default.

document_start is also necessary for extensions that run content scripts at document_start.

Updating specification for the tabs.create, tabs.update, windows.create and similar API, to require them to resolve the returned promise only after the DOMContentLoaded event.

It should be optional and we should be able to specify the timing e.g. document_start.

sendMessage [...] waitForListener: true

Just waiting for a listener may be unreliable with chrome.tabs.update because it may send to the current contents of the tab before it is navigated as the browser doesn't destroy it until the new URL's response is received.

It could be something like onNewDocumentInjected: true.

dotproto commented 2 months ago

Some considerations that were raised during today's discussion include:

fregante commented 2 months ago

I'll raise a few questions:

xeenon commented 2 months ago

I would be more supportive of having a tabs.waitForTab() method, as opposed to adding options to the various APIs. A standalone method can be used for various APIs.

Juraj-Masiar commented 2 months ago

@fregante regarding the Promise VS event, using Promise allows caller to await the tab creation operation (page load) and continue working with the tab without loosing the context, for example sending it work/command/request/query. This will make the execution flow easy to reason about (see my original comment for an example).

I would say the Promise should resolve after all content scripts are executed - that is, for the specific load event the API is targeting. I can imagine the default could be document_idle with option to change that to document_end using some optional resolveAt parameter.

That should fix all race conditions since in both cases you would be sure that the page code was executed so all top level registered handlers are ready. This includes extension pages, content script powered pages and normal websites. And ideally also popup and toolbar, although I'm not sure now how to target those

tophf commented 2 months ago

Maybe waitForLoad({contentScriptInjected: true})? Because there are other cases that don't need to wait for JS content scripts: screen capture, dynamic insertCSS, doing something else first before dynamically injecting the scripts.

document_start should be also possible as it allows to inject functionality that the extension wants to be available while the page is still loading, which may take many seconds for pages with big HTML, especially on a mobile/WiFi network.

xeenon commented 2 months ago

I like the waitForLoad name, since "tab" is redundant with the tabs namespace. I ran with this and wrote this idea up further in an MDN-like style.


Proposal for browser.tabs.waitForLoad()

Signature

browser.tabs.waitForLoad(target, options)

Parameters

Returns

Example

const tab = await browser.tabs.create({ url: '/questions.html' });
await browser.tabs.waitForLoad({ tabId: tab.id, subFrames: true }, { event: 'DOMContentLoaded', timeout: 5_000 });
const response = await browser.tabs.sendMessage(tab.id, { type: 'ask_user_for_something' });
xeenon commented 2 months ago

@tophf That was the intention for all states. If you have a content script at any run_at mode this function can be used to wait for it to have listeners registered (including document_idle).

tophf commented 2 months ago

Ah, sorry, I see it's already specifically mentioned. But maybe you could specify what happens if there are no content scripts.

xeenon commented 2 months ago

@tophf I updated the proposal with that and also added event and timeout options.

xeenon commented 2 months ago

This proposal likely could be polyfilled by using scripting.registerContentScripts() with a script for the tab's current URL in matches at the run_at for the state (or document_start and listens for the specified event.) Then the script sends a message that the API is listening for and resolves the promise, or it rejects at the timeout. Then unregister the content script so it does not fire again if the same URL loads in another tab.

That would be a good way to test this design out and guide any further changes needed. I likely won't have time to write that polyfill, but anyone else is welcome to take a crack at it!

The only complications I see with a polyfill are:

tophf commented 2 months ago

could be polyfilled by using scripting.registerContentScripts

Yes, for the content script case, which is arguably the most popular one, but waitForLoad may be useful even without host access and a content script, in which case it can be polyfilled by using chrome.webNavigation.onCommitted (or chrome.tabs.onUpdated) + tabs.onRemoved + tabs.onReplaced and that won't have the problem of targeting the tab.

Juraj-Masiar commented 2 months ago

@xeenon great idea with the registerContentScripts API! I was so excited I've tested it right away, but there is a catch. Some of my many use-cases is actually for awaiting my own extension page load for example:

const newTab = await browser.tabs.duplicate(thisTabId);
await browser.tabs.waitForLoad({tabId: newTab.id});
await browser.tabs.sendMessage(newTab.id, {type: 'openSearch'});

And although it works in Firefox where you can register content scripts even for extension pages, it won't work in Chrome where you can't.

const scriptId = 'browser.tabs.waitForLoad_' + JSON.stringify(target);
await browser.scripting.registerContentScripts([{
  id: scriptId,
  matches: [
    '<all_urls>',
    browser.runtime.getURL('') + '*',
  ],
  js: ['/wait_for_load.content_script.js'],
  runAt: 'document_end',
}]);

It will complain about: Error: Script with ID 'browser.tabs.waitForLoad_{"tabId":1435791607}' has invalid value for matches[1]: Invalid scheme.

xeenon commented 2 months ago

@Juraj-Masiar Bummer!