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

Allow to cancel method calls #415

Open
maxnikulin opened this issue Jul 4, 2023 · 3 comments
Open

Allow to cancel method calls #415

maxnikulin opened this issue Jul 4, 2023 · 3 comments
Labels
enhancement Enhancement or change to an existing feature next manifest version Consider for the next manifest version proposal Proposal for a change or new feature supportive: safari Supportive from Safari

Comments

@maxnikulin
Copy link

Some WebExtensions API methods may take significant amount of time before their results are ready. Ability to cancel execution of such functions should reduce amount of efforts required to reach predictable behavior of browser extensions and decent user experience. It can be achieved, for example, by using AbortSignal argument similar as it was done for fetch().

API methods that should be cancellable

The functions considered further are examples, it is not an exhaustive list.

permissions.request

User actions (action, contextMenus, commands) may ask for additional optional permissions. The user may start another add-on action when a permission request created from the previous action is still displayed.

Currently there is no way to withdraw permission request popups, so users have to make decision for all of them even for stale ones that do not lead to any further execution of the extension code anymore. Cancelling the stale prompt for addition privileges before a new request should be an improvement for user interface.

scripting.executeScript

Starting of a content script may be postponed till completion of some heavy JavaScript running by the web page. Too late result may be confusing for the user.

Currently implementing of cancellable call is doable, but the price for developers is increased complexity of code. It is possible to pass to contents scripts a deadline timestamp accordingly to some timeout. If execution of a content script is delayed due to waiting for an asynchronous call, it may be possible to notify the script by another scripting.executeScript call or by sending a message. The Promise returned by scripting.executeScript may be combined into Promise.race with another one responsible for timeout or another abort reason.

offscreen.createDocument

Due to a programming error, an offscreen document may stuck in loading state if a script is spinning in a loop.

Currently there is no way to destroy the problem page on timeout (cromium bug)`.

The issue should not be wide spread. An infinite loops inside loading stage scripts normally should be found during testing and should not happen in production code. While offscreen.createDocument is not cancellable, developers should prefer minimal page load code and passing a task using messaging instead of adding query parameters to the document URL.

Adding AbortSignal argument

The methods discussed above have the optional callback argument. It may be extended to accept an object with optional fields

{callback?: function, signal?: AbortSignal}

as an additional alternative to the current variant callback?: function.

I can not estimate cost of implementation of the AbortSignal argument in the code of browsers. Handling arguments that may have different types increases code complexity, but I do not see an alternative since the optional callback arguments are retained in Manifest V3 despite methods may return Promise objects now.

Example of API simplification: desctopCapture

the desctopCapture.cancelChooseDesktopMedia method may be unnecessary if the successor of desktopCapture.chooseDesktopMedia accepts an AbortSignal.

The current function

chrome.desktopCapture.chooseDesktopMedia(
  sources: DesktopCaptureSourceType[],
  targetTab?: Tab,
  callback: function,
) => desktopMediaRequestId: number

with (streamId: string, options: object) => void callback signature may be replaced by a function returning a Promise to a {streamId: string, options: object} object and having the last argument similar to the described above alternative
function? | { callback?: function, signal?: AbortSignal}?. desktopMediaRequestId becomes unnecessary.

Keeping the current name desktopCapture.chooseDesktopMedia and allowing the signal option in the last argument means return type

desktopMediaRequestId: number | { streamId: string, options: object }

dependent on passed arguments. It may be confusing for developers and error prone. Perhaps it is better to add a new method and to preserve the existing pair for compatibility.

Merging API calls can not be applied to the offscreen.createDocument and offscreen.closeDocument pair because their calls may be separated by unload and load again cycle, so an AbortSignal will be lost. However aboratble offscreen.createDocument may be an improvement.

For this particular API it would be a cosmetic improvement with some maintenance cost. Old functions must be kept for backward compatibility.

Alternatives

A disadvantage of the approach with AbortSignal is that some boilerplate code is necessary to combine several signals. Consider an AbortSignal.timeout for a specific scripting.executeScript call and an AbortController created for the whole action.onClicked listener with intention of aborting when another listener of a user action is invoked.

A Promise may be used instead of AbortSignal. When the promise is settled (normally rejected), the execution of API call
received the promise as its argument should be interrupted and an exception should be thrown.

Unlike AbortSignal, Promise objects may be directly used in Promise.all, Promise.race, Promise.allSettled methods, but not in fetch(). Usually the reject() function obtained from Promise constructor callback should saved as a variable in addition to the created Promise. It is similar to the AbortController and AbortSignal pair.

Currently it is possible to use runtime.Port.disconnect() to terminate connections. Usage of such approach functions returning a single value, not a sequence of messages, means overhead for no value.

Perhaps introducing some custom interface instead of AbortSignal or Promise is not reasonable.

Closing remarks

Cancellable API methods may remove some obstacles on the way toward reliably working extensions providing decent user experience.

Discussion of the proposal in the chromium-extensions group was not active: Idea: AbortSignal argument of functions that may cause arbitrary delay. Tue, 27 Jun 2023.

Thanks to Oliver Dunk for the cancelChooseDesktopMedia example.

@dotproto dotproto added next manifest version Consider for the next manifest version enhancement Enhancement or change to an existing feature proposal Proposal for a change or new feature and removed needs-triage labels Jul 5, 2023
@maxnikulin
Copy link
Author

An example of confusing behavior caused by inability to cancel permissions.request call: https://bugzilla.mozilla.org/1841979 Chrome is affected as well.

@xeenon xeenon added the supportive: safari Supportive from Safari label Jul 6, 2023
@maxnikulin
Copy link
Author

Making all calls cancellable never was my goal. Of course, methods completing almost instantly do not need any modification. E.g. I do not expect any delay for a call of tabs.query. There are corner cases however. I had to spent some time for debugging to realize that webNavigation.getAllFrames was a source of hangs (almost certainly it was Chromium). Currently I can not reproduce it and I consider it as a bug. A workaround is required in such cases rather than modification of API. The subject for change is functions that may postpone execution or may need to wait for result by their nature.

I am familiar only with a part of WebExtensions API, so it would be hard for me to provide the exhaustive list of methods that should be made abortable. Often I am even unsure whether particular one should be considered as
specific for some browser or a part of API that should be standardized.

@maxnikulin
Copy link
Author

I am against reducing of ability to cancel a call to just a timeout. I am open to discussion of alternatives to AbortSignal however. I do not think there is any reasonable timeout for permissions.request by default. Developers should be able to terminate execution of a function when another user event action is fired. I have in mind the following approach

async function extensionAction(execCtx, tab) {
    const havePermissions = await chrome.permissions.request(
        {permissions}, {signal: execCtx.signal});
    // Do something else
}

var abortController;
async function actionListener(tab) {
    abortController?.abort(new Error("New user action"));
    abortController = new AbortController();
    await extensionAction({signal: abortController.signal}, tab);
}

chrome.action.onClicked.addListener(tab => void actionListener(tab));

I have tried a more realistic proof of concept in my extension. It is Firefox-only since it relies on an API method unavailable in Chrome. Certainly I can not make permissions.request cancellable, I have not even tried to prevent simultaneous prompts. The only purpose is to stop execution of code if user decision becomes known after starting of another action.
https://github.com/maxnikulin/alt-copy/blob/5625bbb036b87d3bc80f28a0981f58b717b40465/acp_background.js#L464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or change to an existing feature next manifest version Consider for the next manifest version proposal Proposal for a change or new feature supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

3 participants