1422605 - win.PopupNotifications is undefined when requesting optional permissions from context menu click

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[gojko]

Attachment: Screen Shot 2017-12-03 at 19.11.33.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

- Firefox 57.0.1 (64 bit) on MacOS 10.13.1. In an extension, requesting to access additional permissions from a context menu handler. 

browser.permissions.request({permissions:["clipboardRead", "clipboardWrite"]}, function () {/*callback*/})


Actual results:

code throws an exception 

win.PopupNotifications is undefined  ExtensionsUI.jsm:343
	showPermissionsPrompt/< resource:///modules/ExtensionsUI.jsm:343:7
	showPermissionsPrompt resource:///modules/ExtensionsUI.jsm:319:12
	observe resource:///modules/ExtensionsUI.jsm:241:15
	getAPI/request/allow< chrome://extensions/content/ext-permissions.js:55:15
	request chrome://extensions/content/ext-permissions.js:45:31
	next self-hosted:1188:9
	request self-hosted:952:17
	call/result< resource://gre/modules/ExtensionParent.jsm:739:57
	withPendingBrowser resource://gre/modules/ExtensionParent.jsm:407:26
	InterpretGeneratorResume self-hosted:1281:8
	next self-hosted:1188:9
	call resource://gre/modules/ExtensionParent.jsm:738:20
	InterpretGeneratorResume self-hosted:1281:8
	next self-hosted:1188:9
Unchecked lastError value: Error: An unexpected error occurred  ExtensionCommon.jsm:407
	withLastError resource://gre/modules/ExtensionCommon.jsm:407:9
	wrapPromise/< resource://gre/modules/ExtensionCommon.jsm:460:11


Expected results:

a dialog asking for permissions should show up, as per https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions/request

Comment 1

[Andrew Swan [:aswan]]

Can you attach a complete extension that exhibits this problem?  Or alternatively, how did you create the context menu handler?

Comment 2

[gojko]

Hi,

the whole extension is on Github (https://github.com/gojko/bugmagnet). The offending menu item is this one https://bugmagnet.org/assets/operational-mode.png

I'm currently disabling the request for Firefox using this line (https://github.com/gojko/bugmagnet/blob/446d985e0b0e96ceff9644b82a504dbebc739ad7/src/main/extension.js#L13). To enable it for firefox, remove !isFireFox and run `npm run pack-extension` to package it into the `pack` dir. The error will show then.

The context menu item gets created using `chrome.contextMenus.create`, and the handler is added using `chrome.contextMenus.onClicked.addListener((info, tab) => {})`. the onclick parameter to `chrome.contextMenus.create` was not recognised as a user interaction in firefox so it was throwing an error that it cannot ask for additional permissions (chrome actually allows this, and I think FF should too, but this is a separate issue).

If this is too complicated, let me know and I will create a minimal extension example with just this item.

Comment 3

[gojko]

There's a typo in the previous comment, not sure how to edit it, sorry. Instead of just removing `!isFirefox`, set it to `true` for the menu item to show up.

Comment 4

[Andrew Swan [:aswan]]

Huh, I'm not sure if this is a regression or if it never worked.  In browser_ext_user_events.js we test that we properly check for isHandlingUserInput, but not that we actually have a valid browser.
The problem is around here:
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/browser/components/extensions/ext-menus.js#695

We try to pull the browser from the tab object, but tab is not an actual <tab>, but the extension-facing converted object from:
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/browser/components/extensions/ext-menus.js#242

The obvious fix would be to defer all that conversion code from the native event listener until right before we fire the event.  zombie, you know the menus code better than I do, any reason not to do that?

Comment 5

[100の人]

This error also occurs in an Option page integrated into the browser’s add-ons manager. I saw it in Firefox 57.0.2 and Nightly 59.0a1 (2017-12-15)

Example add-on: <https://github.com/esperecyan/firefox-bug1422605>

Thrown exceptions (Nightly):

win.PopupNotifications is undefined                       ExtensionsUI.jsm:343
    showPermissionsPrompt/<         resource:///modules/ExtensionsUI.jsm:343:7
    showPermissionsPrompt          resource:///modules/ExtensionsUI.jsm:319:12
    observe                        resource:///modules/ExtensionsUI.jsm:241:15
    getAPI/request/allow< chrome://extensions/content/ext-permissions.js:55:15
    request               chrome://extensions/content/ext-permissions.js:45:31
    next                                                    self-hosted:1184:9

Error: An unexpected error occurred Async*@moz-extension://c40ac611-35ab-40c6-9bb4-eb8f9ab1f758/options.es:6:21
    EventListener.handleEvent*@moz-extension://c40ac611-35ab-40c6-9bb4-eb8f9ab1f758/options.es:4:1
    @moz-extension://c40ac611-35ab-40c6-9bb4-eb8f9ab1f758/options.es:1:2

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

I also see it with regular mouse clicks and keyboard activation of <input>s in an options page.
In that case the problem seems like it might be
https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/toolkit/components/extensions/ext-permissions.js#44
The immediate cause is that context.xulBrowser is the inner options browser, not the main browser it needs to be to show a notification (pendingEventBrowser is null).  But maybe the actual intervention needs to happen earlier in the code (like in comment #4) so pendingEventBrowser isn't null (context.xulBrowser is the browser for the hidden background extension page in the context menu case).

Not sure if that should be a separate bug or not, but as 100の人 pointed out it has the same error message (which is how I ended up here).

Comment 7

[Andrew Swan [:aswan]]

The issues in options pages are bug 1382953

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Andrew Swan [:aswan] from comment #4)
> Huh, I'm not sure if this is a regression or if it never worked.  In
> browser_ext_user_events.js we test that we properly check for
> isHandlingUserInput, but not that we actually have a valid browser.

Hmm, I can't find, do we even have a full integration test for the whole optional permission prompt flow?


> The problem is around here:
> https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/browser/components/extensions/ext-menus.js#695

Uh, not sure how I missed to notice this is not a NativeTab in bug 1409433, I sorta remember checking that. :\


> The obvious fix would be to defer all that conversion code from the native
> event listener until right before we fire the event.  zombie, you know the
> menus code better than I do, any reason not to do that?

Yeah, seems reasonable, I'll do that.

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: Bug 1422605 - Fix permissions.request() from menus.onClicked listeners

Review commit: https://reviewboard.mozilla.org/r/232898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232898/

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8964119 [details]
Bug 1422605 - Fix permissions.request() from menus.onClicked listeners

https://reviewboard.mozilla.org/r/232898/#review238878

Nice, thank you!

Comment 11

[Tomislav Jovanovic :zombie]

Comment on attachment 8964119 [details]
Bug 1422605 - Fix permissions.request() from menus.onClicked listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232898/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1ef30cd61e9
Fix permissions.request() from menus.onClicked listeners r=aswan

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c1ef30cd61e9

Comment 14

[Ian Moody [:Kwan] (UTC+0)]

Hey Tomislav, would this be safe for uplift? Given 60 is an ESR it'd be nice to have I think.

Comment 15

[Tomislav Jovanovic :zombie]

I think this is safe enough to uplift, but I'm not sure that's reason enough to ask for uplift.  Then again, I don't claim to understand the criteria.

Comment 17

[marius.santa]

Attachment: prmission error.gif

I used the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1454627 to reproduce this issue and I still get the error on the latest FF build 61.0a1 (20180425220031). The second option "Test 2" does not display any error even on the build it was reported on. Please provide more steps or validation that this is the issue so that I can reopen this bug.

Comment 18

[eight04]

I tested the repro on Firefox Dev 61.0b1. If the request is called from browser.menus.onClicked handler, it works. If the request is called from createProperties.onclick, it throws.

Comment 19

[Tomislav Jovanovic :zombie]

Thanks for the reports, I reopened bug 1454627 and will fix there.

Comment 20

[marius.santa]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1422605#c18 and https://bugzilla.mozilla.org/show_bug.cgi?id=1422605#c19 this issue depends on https://bugzilla.mozilla.org/show_bug.cgi?id=1454627.