Closed Bug 1369577 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox56 verified)

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- verified

People

(Reporter: wbamberg, Assigned: aswan)

References

Details

(Whiteboard: [permissions], triaged)

Attachments

(4 files)

Attached file 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.
Priority: -- → P2
Whiteboard: [permissions], triaged
downloads.open() will have this same problem once bug 1369782 lands.
See Also: → 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)
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
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
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: nobody → aswan
Flags: needinfo?(kmaglione+bmo)
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)
Attachment #8878159 - Flags: review?(kmaglione+bmo)
Attachment #8878160 - Flags: review?(kmaglione+bmo)
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 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 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+
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
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
https://hg.mozilla.org/mozilla-central/rev/a149344cbbf6
https://hg.mozilla.org/mozilla-central/rev/0c16e7a1a10f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(aswan)
Attached image 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)
Depends on: 1378990
Filed bug 1378990 to fix the issue described in the previous comment.
Flags: needinfo?(aswan)
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: