1681131 - browser.menus.onClicked handler cannot call browser.browserAction.openPopup

Status: RESOLVED FIXED

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

Description

[Emmanuel SELLIER]

Attachment: tb78contextmenutest-main.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 11_0_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.67 Safari/537.36

Steps to reproduce:

https://github.com/esellier/tb78contextmenutest.git

function contextualClickHandler() {
browser.browserAction.setPopup({"popup": "popup1.html"});
browser.browserAction.openPopup();
browser.browserAction.setPopup({popup:null});
}
browser.menus.create({
id: "MENU1",
title: "CLICK ME",
contexts: ["message_list"]
});
browser.menus.onClicked.addListener(contextualClickHandler);

browser.browserAction.onClicked.addListener(()=>{
console.log("Got the click on browserAction button");
browser.browserAction.setPopup({"popup": "popup2.html"});
browser.browserAction.openPopup();
browser.browserAction.setPopup({popup:null});
});

browser.browserAction.setPopup({popup:null});

Actual results:

[firefox/index.js][debug] Firefox stderr: JavaScript error: moz-extension://..../backgroundTest.js, line 4: Error: browserAction.openPopup may only be called from a user input handler

Although the click on context menu is a user interaction

Expected results:

We should be able to open browserAction popup...

Comment 1

[Emmanuel SELLIER]

In ext-menu.js:276, we have :
// This event could have attachments in the info object, and
        // attachments must be transformed for the child process.
        // See ext-compose.js.
        onClicked: new EventManager({
          context,
          name: "menus.onClicked",
          register: fire => {
            let listener = (info, tab) => {
              info = { ...info }; // Create a copy owned by this process.
              if ("attachments" in info) {
                extensions.loadModule("compose");
                info.attachments = info.attachments.map(a =>
                  // eslint-disable-next-line no-undef
                  new ComposeAttachment(context, a).api()
                );
              }
              withHandlingUserInput(context.contentWindow, () =>
                fire.asyncWithoutClone(
                  Cu.cloneInto(info, context.cloneScope, {
                    cloneFunctions: true,
                  }),
                  Cu.cloneInto(tab, context.cloneScope)
                )
              );
            };

            let event = context.childManager.getParentEvent("menus.onClicked");
            event.addListener(listener);
            return () => {
              event.removeListener(listener);
            };
          },
        }).api(),

This is called by ExtensionCommon.jsm:120 :function withHandlingUserInput(window, callable) {
  let handle = getWinUtils(window).setHandlingUserInput(true);
  try {
    return callable();
  } finally {
    handle.destruct();
  }
}

But as the event is fired asynchronously, the handle.destruct() is called right after callable() and before the event is actually (asynchronously) fired (from withHandlingUserInput).handle.destruct() will reset the context and isHandlingUserInput is not true.Then in ExtensionChild.jsm:713, an error is thrown:  callAsyncFunction(args, callback, requireUserInput) {
    if (requireUserInput) {
      let context = this.childApiManager.context;
      if (!getWinUtils(context.contentWindow).isHandlingUserInput) {
        let err = new context.cloneScope.Error(
          ${this.path} may only be called from a user input handler
        );
        return context.wrapPromise(Promise.reject(err), callback);
      }
    }
    return this.childApiManager.callParentAsyncFunction(
      this.path,
      args,
      callback,
      { alreadyLogged: this.alreadyLogged }
    );
  }

If we replace :fire.asyncWithoutClone(
                  Cu.cloneInto(info, context.cloneScope, {
                    cloneFunctions: true,
                  }),
                  Cu.cloneInto(tab, context.cloneScope)
                )
by fire.async( Cu.cloneInto(info, context.cloneScope, { cloneFunctions: true, }), Cu.cloneInto(tab, context.cloneScope) )
the context is cloned and the isHandlingUserInput is kept to true

Comment 2

[John Bieling (:TbSync)]

firefox is using fire.sync for menu events.

This is related to bug 1654102, which is addressing a more global issue, not limited to the menu API.

Comment 3

[John Bieling (:TbSync)]

Filed a bug in toolkit with a patch. Closing this as a duplicate of 1654102.

Comment 4

[John Bieling (:TbSync)]

It looks like we have to fix this without fixing the underlying issue with async calls in an onClicked event handler reported in bug 1654102.

Comment 5

[John Bieling (:TbSync)]

Attachment: bug1681131_fix_user_input_status_in_onClicked_events.patch

Took a while to dig this up. See
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/User_actions

Using fire.async caused the status as a user input handler to be lost. M-C is also using fire.sync.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204889 [details] [diff] [review]
bug1681131_fix_user_input_status_in_onClicked_events.patch

This breaks the info.attachments property, which is the reason all of this code exists in the first place. We really need a fire.syncWithoutClone method or some other way to clone the function.

Comment 7

[Geoff Lankow (:darktrojan)]

Kris, Shane, re: comment 6, would you accept the addition of syncWithoutClone? Any better ideas?

Comment 8

[John Bieling (:TbSync)]

Attachment: bug1681131_fix_user_input_status_in_onClicked_events_v2.patch

So the clone part is needed, because the listener returns a function for the attachment context. And currently there is only an async version for the clone, which kills the user input status.

This patch keeps the user input status if possible and only drops it for the attachment context.

Comment 9

[Shane Caraveo (:mixedpuppy)]

What I understand from the last patch is that the onClicked handler was modified for TB to support some kind of attachments handler, and that is what caused the issue for the user input handling. I probably would have picked a different design, maybe passing an attachment id and adding an attachments api namespace that the extension could use to deal with any internals around that. But I don't know what's under the hood there, thus that input my not be valid. In any case, given the design there, it seems a syncWithoutClone isn't useful.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9205079 [details] [diff] [review]
bug1681131_fix_user_input_status_in_onClicked_events_v2.patch

This seems like a reasonable compromise. It'll get most things working again anyway.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/351c1deadf93
Fix missing user input status in onClicked events. r=darktrojan

Comment 12

[John Bieling (:TbSync)]

Comment on attachment 9205079 [details] [diff] [review]
bug1681131_fix_user_input_status_in_onClicked_events_v2.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I am evaluating.

Comment 13

[Magnus Melin [:mkmelin]]

78 uplift?

Comment 14

[John Bieling (:TbSync)]

Yes. I will prepare patch.

Comment 15

[John Bieling (:TbSync)]

Attachment: Bug1681131_Fix_missing_user_input_status_in_onClicked_events_ESR78.patch

Patch for ESR78.

Note: The change in the schema description has already been uplifted, so only the actual fix for the user input handler is part of this patch.

Comment 16

[John Bieling (:TbSync)]

Comment on attachment 9218919 [details] [diff] [review]
Bug1681131_Fix_missing_user_input_status_in_onClicked_events_ESR78.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Popups from context clicks will not work

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&collapsedPushes=903140%2C903136&revision=8aba19515a31d62eab74c5df4fb177ae8ceb205a

Risk to taking this patch (and alternatives if risky):
Low

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9218919 [details] [diff] [review]
Bug1681131_Fix_missing_user_input_status_in_onClicked_events_ESR78.patch

[Triage Comment]
Approved for esr78

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 78.10.2:
https://hg.mozilla.org/releases/comm-esr78/rev/cd5f8de6b406