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)
WebExtensions
General
Tracking
(firefox56 verified)
VERIFIED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | verified |
People
(Reporter: wbamberg, Assigned: aswan)
References
Details
(Whiteboard: [permissions], triaged)
Attachments
(4 files)
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
Comment 1•7 years ago
|
||
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•7 years ago
|
Priority: -- → P2
Whiteboard: [permissions], triaged
Comment 2•7 years ago
|
||
downloads.open() will have this same problem once bug 1369782 lands.
Comment 3•7 years ago
|
||
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?
Updated•7 years ago
|
Flags: needinfo?(aswan)
Assignee | ||
Comment 4•7 years ago
|
||
I haven't had a chance to dig into this but I can try to take a look this afternoon...
Flags: needinfo?(aswan)
Comment 5•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → aswan
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years 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•7 years ago
|
Attachment #8878159 -
Flags: review?(kmaglione+bmo)
Attachment #8878160 -
Flags: review?(kmaglione+bmo)
Comment 13•7 years 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•7 years 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•7 years 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•7 years 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
Comment 22•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a149344cbbf6 https://hg.mozilla.org/mozilla-central/rev/0c16e7a1a10f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(aswan)
Comment 28•7 years ago
|
||
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 | ||
Comment 29•7 years ago
|
||
Filed bug 1378990 to fix the issue described in the previous comment.
Flags: needinfo?(aswan)
Comment 30•7 years 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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•