Closed Bug 1654102 Opened 5 years ago Closed 4 years ago

Persist user input handler context through asynchronous functions

Categories

(Thunderbird :: Add-Ons: Extensions API, defect)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jon, Assigned: TbSync)

References

Details

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Listeners attached to event managers like compose.onBeforeSend are allowed to do things like browser.composeAction.openPopup(), because onBeforeSend is an "inputHandling" event manager, and openPopup() is only allowed to execute in a user input handler context.

If you try calling openPopup() from an asynchronous callback that is invoked within that original context, it no longer knows it's in a user input handler. Fair enough, because it really isn't, and maybe shouldn't be. Those callbacks live in their own context (https://tc39.es/ecma262/#sec-async-functions-abstract-operations-async-function-start)
**I'll come back to this point in a moment.

I assume that the await syntax is designed under the hood to dump the remainder of the call stack into a new anonymous function, and attach it as a listener to the "awaited" code in question.

But, if I understand it correctly, await should merge its handler context back into the parent context after the awaited line, and proceed as if everything were carrying on in a synchronous fashion.

So, after an await call, the rest of the function should still behave as a user input handler, and things like openPopup should work as usual.

** For that matter, it would be great if the inputHandling flag carried through to callbacks and anonymous functions. I can't really tell from the spec if that is technically correct, but it seems logical to me.

Actual results:

If you call openPopup() in a user input handler function, it works at the beginning of the function, but not if you call it after an await, or inside of an anonymous function.

Expected results:

Ideally, listeners should act as closures in which things like inputHanlding state remain fixed. That should be the case after await statements, inside anonymous functions, and maybe in callbacks.

This is a problem because, for example, if you want to check a user preference from browser.local.storage, you need some amount of asynchronicity.

Component: Untriaged → Add-Ons: Extensions API

I re-created this bug in toolkit, with a Firefox use-case. I did copy most of your example code :-)
https://bugzilla.mozilla.org/show_bug.cgi?id=1693829

The fix I proposed there also fixes this for our menu API and for our compose.onBeforeSend.

Assignee: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
See Also: → 1693829

I got pointed to this document, which explicitly states, that waiting for a promise kills the user input status:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/User_actions

Discussion is here:
https://phabricator.services.mozilla.com/D105790

Ugh. I suppose it's true that extensions could hold onto a user input context indefinitely, but isn't that something that could easily be noticed in code review, such that a misbehaving extension would not make it onto ATN?

In particular, checking local storage before acting on a user input is such an obvious and useful code pattern that it seems pretty drastic to forbid it. Is there any way that the storage.local API could implement an exception to this, somehow restoring context, while other APIs do not? This is pretty low priority, so if diverging from Firefox is challenging it's probably not worth the effort.

In any case, it would be great to alert devs about that behavior in the Thunderbird docs, similar to what Firefox seems to be doing. I didn't even think to search for this because it's so non-intuitive that a basic pattern like async/await would behave so differently under one very particular circumstance.

Pure WebExtension (without Experiments) do not get reviewed, they are post-reviewed in case somebody got suspicious. This is one of the goals of WebExtensions.

I think there are ways to get this working. Will continue to investigate.

(In reply to John Bieling (:TbSync) from comment #5)

I think there are ways to get this working. Will continue to investigate.

Whilst it might be possible, I'd be concerned that there'd be various ways that could break. For example, your suggestion of a timer in the phabricator revision could mean that you'd get intermittent failures if your task was fine on an ssd drive, but marginal and inconsistent on hdd drive or if there's other mass processing going on (e.g. from another extension).

I think the first async point hit is a consistent way of measuring, and there's other ways extensions should be able to handle it. Checking preferences is storage something that can easily be managed with a cache, and browser.storage.* already has listeners available.

Firefox extensions, although maybe not as complex, haven't needed it either I guess.

Hence until there's a more complex use case that can't potentially be worked around, I'm not sure it is worth trying out something for this.

The implementation I have in mind would use a pref to set the timeout, so we can have a) different settings for Firefox and Thunderbird and b) "disable" the timeout for tests (if we really run into such issues). Since the feedback talked about "indefinite" holding of the user input status, I do not see a need to set a tight timeout at all, we just have to protect from the "indefinite" case. But I have no idea what they think of that approach.

Hence until there's a more complex use case that can't potentially be worked around, I'm not sure it is worth trying out something for this.

I am torn. We tell add-on developers to shift towards async programming, and then we do something like this and force them to find workarounds. This just does not feel right. There could be other use-cases independent of storage, where add-on developers need to make async requests.

@Jonathan P-H : Can you work around this limitation by setting up a listener for storage changes and keep track of the current value of the preference in question and keep a local cache, so you can access the current value synchronously?

(In reply to John Bieling (:TbSync) from comment #7)

The implementation I have in mind would use a pref to set the timeout, so we can have a) different settings for Firefox and Thunderbird and b) "disable" the timeout for tests (if we really run into such issues). Since the feedback talked about "indefinite" holding of the user input status, I do not see a need to set a tight timeout at all, we just have to protect from the "indefinite" case. But I have no idea what they think of that approach.

Timeouts are always tricky though, and how do you determine what's acceptable for the user? Is it fine, that the user clicks a button and then 2 seconds later some UI pop-ups up?

Hence until there's a more complex use case that can't potentially be worked around, I'm not sure it is worth trying out something for this.

I am torn. We tell add-on developers to shift towards async programming, and then we do something like this and force them to find workarounds. This just does not feel right. There could be other use-cases independent of storage, where add-on developers need to make async requests.

I understand the desire, though I think simplicity is better for user security. Add-on authors also need to consider responsiveness - waiting more than a couple of hundred ms (maybe less) is going to feel like yank, so dealing with handling responses as sync will automatically help them to consider and resolve that.

To be clear, I'm not saying we shouldn't ever do this, and I understand the sync vs async issue, but I think it would help to have a more compelling real life use case.

This also means I have to reopen bug 1681131 as that fails because we call the event handler for onClick async, which removes the status even if the user is not using an async callback.

Ok, sorry about that, I suspect using sync for those is a better thing overall.

(In reply to John Bieling (:TbSync) from comment #7)

@Jonathan P-H : Can you work around this limitation by setting up a listener for storage changes and keep track of the current value of the preference in question and keep a local cache, so you can access the current value synchronously?

Yes, I already have this working. It's not a big deal now that I know what's going on, but tracking down what was happening when I first encountered it was frustrating at the time. The local cache solution is also a little messier than I'd like, considering accessing a single variable from storage.local would usually just be one line.

FWIW, I also think that a timeout sounds dangerous. Besides the race condition that @standard8 described, someone might just naively try to hang on to the user input status for a long time with what seems like a legitimate reason at the time, and a timeout would make this behavior even more idiosyncratic.

At the very least, there should be a console warning printed whenever an extension drops its user Input status through one of these mechanisms. It would be really helpful tracking down unexpected behavior.

It looks like this is settled then. I will update the docs and close this bug as WONTFIX.

I do not know if we can get the log entry, as the status is removed after the async function returns early while encountering its first await and I do not think we can detect that. If at all, I will do that in bug 1693829.

You need to log in before you can comment on or make changes to this bug.