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

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

Open
Juraj-Masiar opened this issue Apr 13, 2024 · 16 comments
Labels
opposed: chrome Opposed by Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari

Comments

@Juraj-Masiar
Copy link

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.

@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 13, 2024
@fregante
Copy link

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
Copy link

tophf commented Apr 14, 2024

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.

@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
@dotproto
Copy link
Member

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

  • In order to be more confident about the specific document being awaited, we should allow developers to target a page via documentId.
  • An API like this should probably allow the developer to provide an AbortController.
  • Consider adding a bool to signal whether or not redirects would be taken into consideration.

@fregante
Copy link

I'll raise a few questions:

  • at what point does the promise resolve exactly?
  • what happens with content scripts running on document_idle? Should it wait for content scripts to run?
  • what happens if the content script is injected manually via scripting API?
  • if the API follows the content script loading and is specifically targeting the ability to message the content script, does its name make sense?
  • why does it return a promise instead of just being a regular tabs.onReady event?

@xeenon
Copy link
Collaborator

xeenon commented Apr 25, 2024

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.

@Rob--W Rob--W added supportive: firefox Supportive from Firefox and removed needs-triage: firefox Firefox needs to assess this issue for the first time labels Apr 25, 2024
@Juraj-Masiar
Copy link
Author

@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
Copy link

tophf commented Apr 26, 2024

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
Copy link
Collaborator

xeenon commented Apr 26, 2024

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

  • target (object): Specifies target identifiers and options for the tab, frame, or document:

    • tabId (integer, optional): The ID of the tab.
    • frameId (integer, optional): The ID of a specific frame within the tab. If only frameId is specified, it defaults to the main frame unless otherwise noted.
    • documentId (string, optional): A unique identifier for the document within the tab or frame.
    • subFrames (boolean, optional): If true, waits for all subframes of the specified frame to reach the specified state or event. Defaults to false.

    At least one of tabId, frameId, or documentId must be specified. If only frameId is specified, it must be non-zero.

  • options (object, optional):

    • state (string, optional): Specifies the load state that the promise will wait for before resolving. This option is mutually exclusive with event. Valid options are "document_start", "document_end", and "document_idle". Defaults to "document_idle" if no event is specified.
    • event (string, optional): Specifies a specific DOM event that the promise will wait for before resolving. This option is mutually exclusive with state. Valid options are "DOMContentLoaded" or "load".
    • timeout (integer, optional): The maximum number of milliseconds to wait before the promise rejects. Defaults to 30_000 milliseconds (30 seconds).

Returns

  • Promise: Resolves after the specified target reaches the specified state or event, and after all content scripts for that state have executed, or at the time a content script would typically run if registered at that state. The promise resolves immediately if the specified state or event has already been reached for the target and any specified frames. If the target cannot reach this state or event, or if the timeout expires without reaching the state or triggering the event, the promise will reject.

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
Copy link
Collaborator

xeenon commented Apr 27, 2024

@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
Copy link

tophf commented Apr 27, 2024

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

@xeenon
Copy link
Collaborator

xeenon commented Apr 27, 2024

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

@xeenon
Copy link
Collaborator

xeenon commented Apr 27, 2024

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:

  • Targeting a specific tab, if the same URL happens to load in a second tab at the same time.
  • Knowing when the tab is already at the desired state or event so the promise resolves immediately.
  • Properly handling subFrames and waiting for messages from each frame.

@tophf
Copy link

tophf commented Apr 27, 2024

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
Copy link
Author

@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
Copy link
Collaborator

xeenon commented Apr 28, 2024

@Juraj-Masiar Bummer!

@rdcronin
Copy link
Contributor

I'm not in favor of this on the Chromium side. There are two main reasons:

There isn't a single definition of what "waitForTab()" should do.

In conversations, we've covered all kinds of different possible events:

  • navigation commits
  • final navigation commits (i.e., we ensure there's no redirects)
  • correspond to the loaded DOM event
  • correspond to the DOMContentLoaded DOM event
  • tab is ready to accept messages from the extension (which implies a content script has ran -- at potentially different run locations -- and properly set up its message listeners)

In addition, there's other handling that raises questions, e.g.

  • The tab is closed before load completes
  • The tab navigates to another URL (e.g., if using this in response to tabs.create() or tabs.update())

I think @xeenon 's comment here does a good job of highlighting some of the complexities here.

We could choose some of these as defaults, but likely any single configuration of these we choose wouldn't work for the majority of extension developers. Additionally, I don't think adding individual toggles for all of these at the browser level doesn't scale well and brings much more complexity to the platform.

This is already doable with existing extension APIs.

One can fairly trivially create a listener that listens for the tab to be loaded -- for the definition of "loaded" that meets their needs.

I do agree that this is a common pain point that many extension developers face; however, rather than solving this at the platform level, I think this is a good opportunity for library support. A library could provide a variety of all these different functionalities in a relatively straightforward way, including providing developers configuration options for listening to the event in the way they want to.

I think library support is something that we've historically had a dearth of in the extensions space, but I don't think that's a reason to add all these components to the platform. Rather, I'd like to see us encouraging this library development (and, ideally, as browser vendors, we can perhaps even maintain a few common libraries -- I think this would be a great candidate for that).

I agree with the motivation behind this, but don't think it's something that needs to be, or should be, solved at the platform level.

@rdcronin rdcronin added opposed: chrome Opposed by Chrome and removed needs-triage: chrome Chrome needs to assess this issue for the first time labels Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
opposed: chrome Opposed by Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

7 participants