Can't call permissions.request() from a browser action or context menu click

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P2
normal
VERIFIED FIXED
3 months ago
a month ago

People

(Reporter: wbamberg, Assigned: aswan)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [permissions], triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

3 months ago
Created attachment 8873660 [details]
perms.zip

I've attached an add-on that adds a browser action and a context menu item. They both try to request a permission using permissions.request(). When clicked, they fail with:

Error: May only request permissions from a user input handler
I'm adding a similar check to downloads.open() and noticed that a click handler in a browser action popup also isn't being registered as a user input handler.

Updated

2 months ago
Priority: -- → P2
Whiteboard: [permissions], triaged
downloads.open() will have this same problem once bug 1369782 lands.
See Also: → bug 1369782
I'm guessing we'll need to find some more windows to check if they're handling user input. This seems to make sense to me for a popup but I'm not sure about the context menu and especially the browser action.

@aswan have you thought about this at all? Do you have any ideas of where to get started on this?
Flags: needinfo?(aswan)
(Assignee)

Comment 4

2 months ago
I haven't had a chance to dig into this but I can try to take a look this afternoon...
Flags: needinfo?(aswan)
I haven't investigated yet, I simply noticed it when working on something else, but I was going to look into what E10SUtils.wrapHandlingUserInput does, and whether it would be helpful for the same/similar issues with bug 1341126
(Assignee)

Comment 6

2 months ago
I think wrapHandlingUserInput does just what we need for:
browser.browserAction.onClicked
browser.pageAction.onClicked
browser.commands.onCommand
browser.contextMenus.onClicked

I'll try to get a patch together
(Assignee)

Comment 7

2 months ago
Ugh, I have the patch to call setHandlingUserInput() during those events but now things fall apart since the implementation of permissions needs to find the <browser> element where the click originated in order to show the permission notification (since it is anchored to the addon icon in the location bar).  We could do some hacky thing to find the foreground window but that sounds racy to me.  The alternative is actually propagating the <browser> element where the events listed in comment 6 occur and stashing it somewhere where the permissions code can pick it up.  That also sounds clumsy.
Kris, show me the way here...
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

2 months ago
Assignee: nobody → aswan
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
Comment on attachment 8878160 [details]
Bug 1369577 Part 2 Propagate isHandlingUserInput for browserAction, pageAction, and menus

(see comments in the commit message)
Attachment #8878160 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8878159 - Flags: review?(kmaglione+bmo)
Attachment #8878160 - Flags: review?(kmaglione+bmo)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8878159 [details]
Bug 1369577 Part 1 Rename SingletonEventManager to EventManager

https://reviewboard.mozilla.org/r/149538/#review158182

::: toolkit/components/extensions/ExtensionChild.jsm:53
(Diff revision 2)
>  const {
>    LocalAPIImplementation,
>    LocaleData,
>    NoCloneSpreadArgs,
>    SchemaAPIInterface,
> -  SingletonEventManager,
> +  EventManager,

Nit: Please keep sorted.

::: toolkit/components/extensions/ExtensionCommon.jsm:1516
(Diff revision 2)
>    LocalAPIImplementation,
>    LocaleData,
>    NoCloneSpreadArgs,
>    SchemaAPIInterface,
>    SchemaAPIManager,
> -  SingletonEventManager,
> +  EventManager,

Please keep sorted.

::: toolkit/components/extensions/ext-webNavigation.js:150
(Diff revision 2)
>    };
>  
> -  return SingletonEventManager.call(this, context, name, register);
> +  return EventManager.call(this, context, name, register);
>  }
>  
> -WebNavigationEventManager.prototype = Object.create(SingletonEventManager.prototype);
> +WebNavigationEventManager.prototype = Object.create(EventManager.prototype);

Ick.

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:63
(Diff revision 2)
>    let fireSingleton;
> -  let onSingleton = new SingletonEventManager(context, "onSingleton", fire => {
> +  let onSingleton = new EventManager(context, "onSingleton", fire => {

These names don't really make sense anymore.
Attachment #8878159 - Flags: review?(kmaglione+bmo) → review+

Comment 14

2 months ago
mozreview-review
Comment on attachment 8878160 [details]
Bug 1369577 Part 2 Propagate isHandlingUserInput for browserAction, pageAction, and menus

https://reviewboard.mozilla.org/r/149540/#review158190

::: browser/components/extensions/ext-c-menus.js:163
(Diff revision 2)
>            return context.childManager.callParentAsyncFunction("menusInternal.removeAll", []);
>          },
>  
>          onClicked: new EventManager(context, "menus.onClicked", fire => {
>            let listener = (info, tab) => {
> -            fire.async(info, tab);
> +            let handle = context.domWindowUtils.setHandlingUserInput(true);

Can we add a helper method for this, and give it a closure? That would also remove the need to add the missing try-finally clause here.

::: browser/components/extensions/test/browser/browser_ext_user_events.js:44
(Diff revision 2)
> +    if (result.errmsg) {
> +      dump(`ERROR ${result.errmsg}\n`);
> +    }

Please make this something like `is(result.errmsg, undefined, "Should have no error message")`... or at least use info() rather than dump()

::: toolkit/components/extensions/ExtensionChild.jsm:766
(Diff revision 2)
>            let args = data.args.deserialize(this.context.cloneScope);
>  
> +          let handle;
> +          try {
> +            if (data.handlingUserInput) {
> +              handle = this.context.domWindowUtils.setHandlingUserInput(true);

Same here. Please add a helper function to handle this.

::: toolkit/components/extensions/ExtensionCommon.jsm:420
(Diff revision 2)
> +defineLazyGetter(BaseContext.prototype, "domWindowUtils", function() {
> +  return this.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                           .getInterface(Ci.nsIDOMWindowUtils);
> +});

Please use `ExtensionUtils.getWinUtils`.

Also, if we make this a property of `context`, it needs to be nulled out when `contextWindow` is nulled out.

::: toolkit/components/extensions/ExtensionParent.jsm:334
(Diff revision 2)
> +    } catch (err) {
> +      throw err;

No need for this.

::: toolkit/components/extensions/ExtensionParent.jsm:632
(Diff revision 2)
> +      let result, saveBrowser;
> +      try {
> +        saveBrowser = context.pendingEventBrowser;
> +        context.pendingEventBrowser = pendingEventBrowser;
> +        result = fun(...args);
> +      } finally {
> +        context.pendingEventBrowser = saveBrowser;
> +      }

This is duplicating all of the logic in `withPendingBrowser`. Please just have that method save the original browser, and then this becomes:

    let result = context.withPendingBrowser(
        pendingEventBrowser,
        () => fun(...args));
Attachment #8878160 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 months ago
mozreview-review
Comment on attachment 8878160 [details]
Bug 1369577 Part 2 Propagate isHandlingUserInput for browserAction, pageAction, and menus

https://reviewboard.mozilla.org/r/149540/#review158576

::: toolkit/components/extensions/ExtensionParent.jsm:331
(Diff revision 3)
> +
>      apiManager.emit("proxy-context-load", this);
>    }
>  
> +  withPendingBrowser(browser, callable) {
> +    let saveBrowser = this.pendingEventBrowser;

Nit: `savedBrowser`
Attachment #8878160 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26fd6d0a1254
Part 1 Rename SingletonEventManager to EventManager r=kmag
https://hg.mozilla.org/integration/autoland/rev/dfb376de5c23
Part 2 Propagate isHandlingUserInput for browserAction, pageAction, and menus r=kmag
Backed out for failing xpcshell's test_ext_simple.js and test_ext_startup_cache.js on Android:

https://hg.mozilla.org/integration/autoland/rev/a7ea11bae1ce47fb94c6e62696b474f492131101
https://hg.mozilla.org/integration/autoland/rev/fef1e9cda18c114f3264d2833277dfc6f8ce5758

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dfb376de5c23bb6f9598bb49cde95d7519a9740a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110911092&repo=autoland

[task 2017-06-29T22:42:24.386864Z] 22:42:24     INFO -  TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js
[task 2017-06-29T22:42:32.991971Z] 22:42:32  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | xpcshell return code: 0
[task 2017-06-29T22:42:32.992437Z] 22:42:32     INFO -  TEST-INFO took 8605ms
[task 2017-06-29T22:42:32.992701Z] 22:42:32     INFO -  >>>>>>>
[task 2017-06-29T22:42:32.993049Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell
[task 2017-06-29T22:42:32.993796Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_simple.js"]; -e const _TEST_NAME = "xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js" -e _execute_test(); quit(0);
[task 2017-06-29T22:42:32.993865Z] 22:42:32     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-06-29T22:42:32.994145Z] 22:42:32     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-06-29T22:42:32.994401Z] 22:42:32     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-06-29T22:42:32.994676Z] 22:42:32     INFO -  running event loop
[task 2017-06-29T22:42:32.994957Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | Starting test_simple
[task 2017-06-29T22:42:32.995250Z] 22:42:32     INFO -  (xpcshell/head.js) | test test_simple pending (2)
[task 2017-06-29T22:42:32.995517Z] 22:42:32     INFO -  "Extension attached"
[task 2017-06-29T22:42:32.995798Z] 22:42:32     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-06-29T22:42:32.999203Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | Extension error: SingletonEventManager is not defined chrome://browser/content/ext-utils.js:208 :: @chrome://browser/content/ext-utils.js:208:1
[task 2017-06-29T22:42:32.999317Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | lazyInit/promise<@resource://gre/modules/ExtensionParent.jsm:111:9
[task 2017-06-29T22:42:32.999427Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | _do_main@/storage/sdcard/tests/xpc/head.js:220:3
[task 2017-06-29T22:42:32.999511Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | _execute_test@/storage/sdcard/tests/xpc/head.js:549:5
[task 2017-06-29T22:42:32.999602Z] 22:42:32     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_simple.js | @-e:1:1
[task 2017-06-29T22:42:32.999683Z] 22:42:32     INFO -  Unexpected exception ReferenceError: SingletonEventManager is not defined at chrome://browser/content/ext-utils.js:208
[task 2017-06-29T22:42:32.999762Z] 22:42:32     INFO -  @chrome://browser/content/ext-utils.js:208:1
[task 2017-06-29T22:42:32.999825Z] 22:42:32     INFO -  lazyInit/promise<@resource://gre/modules/ExtensionParent.jsm:111:9
[task 2017-06-29T22:42:32.999879Z] 22:42:32     INFO -  _do_main@/storage/sdcard/tests/xpc/head.js:220:3
[task 2017-06-29T22:42:32.999958Z] 22:42:32     INFO -  _execute_test@/storage/sdcard/tests/xpc/head.js:549:5
[task 2017-06-29T22:42:33Z] 22:42:32     INFO -  @-e:1:1
[task 2017-06-29T22:42:33.000040Z] 22:42:32     INFO -  exiting test
[task 2017-06-29T22:42:33.000142Z] 22:42:32     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "SingletonEventManager is not defined" {file: "chrome://browser/content/ext-utils.js" line: 208}]
[task 2017-06-29T22:42:33.000198Z] 22:42:32     INFO -  @chrome://browser/content/ext-utils.js:208:1
[task 2017-06-29T22:42:33.000279Z] 22:42:32     INFO -  lazyInit/promise<@resource://gre/modules/ExtensionParent.jsm:111:9
[task 2017-06-29T22:42:33.000334Z] 22:42:32     INFO -  _do_main@/storage/sdcard/tests/xpc/head.js:220:3
[task 2017-06-29T22:42:33.000388Z] 22:42:32     INFO -  _execute_test@/storage/sdcard/tests/xpc/head.js:549:5
[task 2017-06-29T22:42:33.000451Z] 22:42:32     INFO -  @-e:1:1
[task 2017-06-29T22:42:33.000516Z] 22:42:33     INFO -  "
[task 2017-06-29T22:42:33.000840Z] 22:42:33     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:22226"]"
[task 2017-06-29T22:42:33.001134Z] 22:42:33     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "UnknownError" {file: "resource://gre/modules/IndexedDB.jsm" line: 259}]"
[task 2017-06-29T22:42:33.001462Z] 22:42:33     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "[object DOMError]" {file: "resource://gre/modules/ExtensionParent.jsm" line: 1361}]
[task 2017-06-29T22:42:33.001817Z] 22:42:33     INFO -  get@resource://gre/modules/ExtensionParent.jsm:1361:7
[task 2017-06-29T22:42:33.002220Z] 22:42:33     INFO -  _execute_test@/storage/sdcard/tests/xpc/head.js:630:5
[task 2017-06-29T22:42:33.002857Z] 22:42:33     INFO -  @-e:1:1
[task 2017-06-29T22:42:33.003207Z] 22:42:33     INFO -  "
Flags: needinfo?(aswan)
Android chrome tests: https://treeherder.mozilla.org/logviewer.html#?job_id=110911079&repo=autoland
> mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getTitle_setTitle.html | startup failed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

2 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a149344cbbf6
Part 1 Rename SingletonEventManager to EventManager r=kmag
https://hg.mozilla.org/integration/autoland/rev/0c16e7a1a10f
Part 2 Propagate isHandlingUserInput for browserAction, pageAction, and menus r=kmag

Comment 27

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a149344cbbf6
https://hg.mozilla.org/mozilla-central/rev/0c16e7a1a10f
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 months ago
Flags: needinfo?(aswan)

Comment 28

2 months ago
Created attachment 8883020 [details]
2017-07-03.gif

I can reproduce this issue on Firefox 56.0a1 (2017-06-30) under Wind 7 64-bit and Ubuntu 12.04 64-bit. 
 
On the latest Firefox 56.0a1 (2017-07-03) under Wind 7 64-bit and Ubuntu 12.04 64-bit I can see another error: “win.PopupNotifications is undefined  ExtensionsUI.jsm:438”.
 
Is this error expected? since default_popup is not declared?
 
Please see the attached file.
Flags: needinfo?(aswan)
(Assignee)

Updated

a month ago
Depends on: 1378990
(Assignee)

Comment 29

a month ago
Filed bug 1378990 to fix the issue described in the previous comment.
Flags: needinfo?(aswan)

Comment 30

a month ago
This issue is verified as fixed on Firefox 56.0a1 (2017-07-10) under Wind 7 64-bit and Ubuntu 12.04 64-bit, after the fix of Bug 1378990, the error from the comment 28 is no longer displayed and permissions.request() from a browser action or context menu click, it is working.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.