1693829 - menu.onClicked: Allow withHandlingUserInput to be used with async callbacks

Status: RESOLVED WONTFIX

(WebExtensions :: General, defect, P2)

Description

[John Bieling (:TbSync)]

The menu.onClicked event allows to open browser action popus. This works:

function contextualClickHandler() {
    browser.browserAction.openPopup();
}
browser.menus.create({
    id: "MENU1",
    title: "CLICK ME",
    contexts: ["all"]
});
browser.menus.onClicked.addListener(contextualClickHandler);

However, if I want to check something in my local storage before opening the popup, I have to make contextualClickHandler async:

async function contextualClickHandler() {
    let data = await browser.storage.local.get();
    // ... do something with data ...
    browser.browserAction.openPopup();
}
browser.menus.create({
    id: "MENU1",
    title: "CLICK ME",
    contexts: ["all"]
});
browser.menus.onClicked.addListener(contextualClickHandler);

This throws a openPopup() is only allowed to execute in a user input handler context.

The reason for this is probably, that the event is fired as a callback to withHandlingUserInput (https://searchfox.org/mozilla-central/source/browser/components/extensions/child/ext-menus.js#274) and that does not await the callback, which means the handle is destroyed before the popup is opened.

Comment 1

[John Bieling (:TbSync)]

Attachment: Bug 1693829 - Allow withHandlingUserInput to be used with async callbacks. r=mixedpuppy

Comment 2

[Alex Cornestean]

Hello,

I’m from QA and the issue appeared in our triage. Does the issue require we perform manual testing to confirm the bug? If so, could you please provide us with steps to reproduce and possibly a test extension?

Thank you !

Comment 3

[John Bieling (:TbSync)]

Attachment: bug1693829_sync.zip

This version is using a sync event listener for the menus.onClicked event.

Load the add-on by loading the zip file as a temporary add-on (Tools -> Add-ons -> Gear Icon -> Debug add-ons).

After install you can right-click on any content area and you will get a context menu with a "sync test (success)" entry. Click that and it will open the browserAction popup.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1693829_async.zip

This version is using an async event listener for the menus.onClicked event.

Load the add-on by loading the zip file as a temporary add-on (Tools -> Add-ons -> Gear Icon -> Debug add-ons).

After install you can right-click on any content area and you will get a context menu with a "async test (fails)" entry. Click that and the browserAction popup will not open, but instead print this to the error console:

Error: browserAction.openPopup may only be called from a user input handler

Comment 5

[John Bieling (:TbSync)]

Are you able to reproduce the issue?

Comment 6

[Shane Caraveo (:mixedpuppy)]

Moving my review comment over here, with a little more context.

After some discussion and refreshing of memory, I don't think we'd accept this change. The behavior here is intentional that it requires the action to be done during the user event.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/User_actions

Comment 7

[Alex Cornestean]

Hello,

I’m terribly sorry I did not come around to reproduce the issue earlier, but I got caught up with something else.

As per Comment 3 and Comment 4, I did manage to reproduce the issue on the latest Nightly (88.0a1/20210222214056), Beta (86.0/20210222142601) and Release (85.0.2/20210208133944) under Windows 10 x64.
With the sync version of the add-on the pop-up is displayed and the error is not present in the console. With the async version, no pop-up is opened and the mentioned error is logged in the console.

However, in the light of Comment 6, can the issue be closed?

Comment 8

[John Bieling (:TbSync)]

I’m terribly sorry I did not come around to reproduce the issue earlier, but I got caught up with something else.

Don't be sorry, thanks for looking into this!

It would be nice to keep it open until Shane has made a final decision, as I have made an alternative suggestion.

Comment 9

[John Bieling (:TbSync)]

The use-case in question could be solved differently (local caching of preferences) and the async call to local storage is no longer needed. Not nice, but works.

I also got feedback from other developers regarding the Promise.race approach, stating it will cause other issues down the road. So I close this bug. Thank you for your time and your feedback.