1382953 - Can't use browser.permissions.request from about:addons preferences page (win.PopupNotifications is undefined)

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

While testing the Gecko Profiler Addon, I need to set the profilerViewerURL from the preferences page, which the addon calls:

  const granted = await browser.permissions.request({
      origins: [getPermissionFromProfileViewerURL(optionsObj[key])],
  });

But failed immediately with unknown error.

The error from browser toolbox console is:

  15:35:13.472 win.PopupNotifications is undefined 1 ExtensionsUI.jsm:441
	showPermissionsPrompt/< resource:///modules/ExtensionsUI.jsm:441:7
	showPermissionsPrompt resource:///modules/ExtensionsUI.jsm:417:12
	observe resource:///modules/ExtensionsUI.jsm:241:15
	getAPI/request_parent/allow< chrome://extensions/content/ext-permissions.js:57:15
	request_parent chrome://extensions/content/ext-permissions.js:47:31
	InterpretGeneratorResume self-hosted:1276:8
	next self-hosted:1183:9
	request_parent self-hosted:947:17
	call/result< resource://gre/modules/ExtensionParent.jsm:696:57
	withPendingBrowser resource://gre/modules/ExtensionParent.jsm:395:26
	next self-hosted:1183:9
	call resource://gre/modules/ExtensionParent.jsm:695:20
	next self-hosted:1183:9
15:35:13.473 Error: An unexpected error occurred 1 options.js:36:11
	saveOptions moz-extension://6aa65102-9a9c-406d-95f4-81d301e7b452/options.js:36:11


It looks like win.PopupNotifications is undefined for about:addons page.

Comment 1

[Kris Maglione [:kmag]]

I guess we need to map these <browser>s to the top-level tab browser. We can either do that in ExtensionUI, or re-use the code we have elsewhere that special cases about:addons browsers when setting tab IDs.

But this makes me suspect we don't handle this properly for things like popup or sidebar browsers either. I suppose, given that popups are auto-close, we'd run into the normal auto-close issues with the permission prompts there, but sidebars should arguably be able to handle this.

Comment 3

[Markus Stange [:mstange]]

Is there a workaround for this?

Comment 4

[Andrew Swan [:aswan]]

(In reply to Markus Stange [:mstange] from comment #3)
> Is there a workaround for this?

Its probably not very satisfying but for now, opening the preferences in a separate tab i guess?

Comment 5

[Markus Stange [:mstange]]

That works, thanks! The URL is moz-extension://73bc7cd1-4322-2745-b8cc-9d1a6e0a27b1/options.html for me.

Comment 6

[Andrew Swan [:aswan]]

The uuid in that URL is local to your installation.  But you can put the "open_in_tab" property into options_ui in the extension manifest to automatically have preferences open in a tab:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/options_ui#Syntax

Comment 7

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #1)
> I guess we need to map these <browser>s to the top-level tab browser. We can
> either do that in ExtensionUI, or re-use the code we have elsewhere that
> special cases about:addons browsers when setting tab IDs.
> 
> But this makes me suspect we don't handle this properly for things like
> popup or sidebar browsers either. I suppose, given that popups are
> auto-close, we'd run into the normal auto-close issues with the permission
> prompts there, but sidebars should arguably be able to handle this.

Yeah, agreed.  Between having a full plate and not knowing the popup/sidebar stuff very well, I'm skeptical that I can do this in 57.  But P5 seems too low, bumping it up a wee bit.

Comment 8

[Sébastien Règne]

Attachment: testcase.zip

I attach a test case which permission prompt:
  - doesn't display on Firefox 57.0a1 (2017-09-10) ;
  - works on Firefox 55.0.3.

Steps:
  1. dezip attach file
  2. load tempary add-on ;
  3. go to preference of add-on ;
  4. click on "Remove" button ;
  5. click on "Request" button : permission prompt doesn't display.

Browser console:
// Click on "Remove" button.

19:24:01,361 removed = true 1 script.js:7:9

// Click on "Request" button.

19:24:03,462 win.PopupNotifications is undefined 1 ExtensionsUI.jsm:329
	showPermissionsPrompt/< resource:///modules/ExtensionsUI.jsm:329:7
	showPermissionsPrompt resource:///modules/ExtensionsUI.jsm:305: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:1190:9

19:24:03,476 granted: 1 script.js:18:9

19:24:03,478 Error: An unexpected error occurred
Trace de la pile :
Async*@moz-extension://76b70b4b-8122-4179-a315-4ab599f1c066/script.js:15:5
EventListener.handleEvent*@moz-extension://76b70b4b-8122-4179-a315-4ab599f1c066/script.js:14:1
 1 script.js:19:9
	<anonyme> moz-extension://76b70b4b-8122-4179-a315-4ab599f1c066/script.js:19:9
	(Asynchrone : promise callback)
	<anonyme> moz-extension://76b70b4b-8122-4179-a315-4ab599f1c066/script.js:15:5
	(Asynchrone : EventListener.handleEvent)
	<anonyme> moz-extension://76b70b4b-8122-4179-a315-4ab599f1c066/script.js:14:1

Comment 10

[me]

I get the same message when requesting permissions from background.js + contextMenus.onClicked

Shortened code:

  chrome.contextMenus.onClicked.addListener(() => {
    chrome.permissions.request({
      origins: [
        `https://google.com/*`
      ]
    }, granted => {
      // It never reaches this place
    });
  });



Full code: https://github.com/bfred-it/webext-domain-permission-toggle/blob/05bc4be/webext-domain-permission-toggle.js#L31-L35

Comment 11

[Christoph Zwerschke]

Can confirm this problem with 57.0.2.

When I request permissions
- from a popup: nothing happens, 
- from the about page: nothings happens, and I get that error message (unexpected error occured)
- from a separate tab: it works.

Comment 12

[Christoph Zwerschke]

> Can confirm this problem with 57.0.2

... and also with with 59.0a1 (2017-12-07)

Comment 13

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

(In reply to Sébastien Règne from comment #8)
>   - doesn't display on Firefox 57.0a1 (2017-09-10) ;
>   - works on Firefox 55.0.3.

Thanks for the tip!  This was regressed by bug 1357486.

Comment 14

[jscher2000]

I was using a context menu entry from the browser action button, and that does not work regardless of the site in the active tab (win.PopupNotifications is undefined).

This code example, using a separate moz-extension page in a new tab to collect user input, does still work:

https://github.com/mdn/webextensions-examples/tree/master/permissions

Is there currently ANY more elegant way to use browser.request.permission() or is that the only option? Let's please document the current situation on MDN to reduce developer frustration:

https://developer.mozilla.org/Add-ons/WebExtensions/API/permissions/request

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1382953: Fix permission prompts in about:addons options browsers.

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8966009 [details]
Bug 1382953: Fix permission prompts in about:addons options browsers.

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

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8966009 [details]
Bug 1382953: Fix permission prompts in about:addons options browsers.

https://reviewboard.mozilla.org/r/234752/#review240736

The test looks dicey but I don't have a better option to suggest.

::: browser/modules/ExtensionsUI.jsm:255
(Diff revision 2)
>          histkey = "installLocal";
>        } else {
>          histkey = "installWeb";
>        }
>  
>        this.showPermissionsPrompt(target, strings, icon, histkey)

Pass browser as the first argument here to avoid another lookup inside showPermissionsPrompt?

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/791f0155deb4500a2acbea31711ed1485df2b79f
Bug 1382953: Fix permission prompts in about:addons options browsers. r=aswan

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7054991fa0cc83476b63e1e04f8551a1e75b74a
Bug 1382953: Follow-up: Temporarily disable browser_ext_user_events on debug for being too flaky. r=bustage DONTBUILD CLOSED TREE

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/791f0155deb4
https://hg.mozilla.org/mozilla-central/rev/a7054991fa0c

Comment 21

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

Kris, would this be a safe candidate for uplift? I think it'd be especially good for extension authors & users to be able to rely on considering 60 will be an ESR.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This has missed Fx60 at this point. We could maybe consider it for a dot release if/when it gets nominated.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Also, this will need a rebased patch for release/esr60 if we intend to uplift it. The landed patch doesn't graft cleanly.

Comment 24

[Kris Maglione [:kmag]]

Attachment: Patch rebased for release

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1357486
[User impact if declined]: This issue breaks features of many Chrome extensions, and Firefox extensions developed before OOP was enabled by default, which expect to be able to request optional permissions from their options pages. When the issue is fixed on other channels, not having it on 60 branch is likely to cause additional breakage for ESR users with add-ons developed on release branches.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: I verified myself when it landed, but it hasn't been verified by QA.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low-risk
[Why is the change risky/not risky?]: The changes are minimal and well-understood. They're more likely to unexpectedly fix other unknown issues than to cause new ones. The riskiest changes are to the test file, which shouldn't be a concern for uplift.
[String changes made/needed]: None

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: This bug impacts a very common feature of extensions which use optional permissions, which is likely to cause an increasing amount of breakage for ESR users once the fix is on release branches.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): See above
String or UUID changes made by this patch: None

Comment 25

[Kris Maglione [:kmag]]

Urgh, sorry, missed an ESLint failure. Just so I don't have to update the patch and re-request flags, here's a fixup diff:

https://gist.github.com/bf03c12ecd082c49a98c628243bfc1c8

Comment 26

[Will Bamberg [:wbamberg]]

I'm writing some compat notes for this. It seems that with this fix, options documents embedded in about:addons do now work, but sidebar and popup documents don't, is that right/expected?

Is there a workaround for the sidebar/popup case? I tried using a background page and message-passing, but that fails because it's not recognized as a user input handler.

Comment 27

[Kris Maglione [:kmag]]

(In reply to Will Bamberg [:wbamberg] from comment #26)
> I'm writing some compat notes for this. It seems that with this fix, options
> documents embedded in about:addons do now work, but sidebar and popup
> documents don't, is that right/expected?
> 
> Is there a workaround for the sidebar/popup case? I tried using a background
> page and message-passing, but that fails because it's not recognized as a
> user input handler.

Popup documents are a separate issue. about:addons frames are easy to handle, because they belong to a tab. Popups and sidebars do not, which means they don't have a valid place to anchor the permissions door hanger. For the popup case, even if they did, trying to open the door hanger would cause the popup to close.

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 8975678 [details] [diff] [review]
Patch rebased for release

I'll keep this on the radar for 60.1esr, but it doesn't sound like we need to rush it in a dot release.

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 8975678 [details] [diff] [review]
Patch rebased for release

fix an issue with webextensions, approved for 60.1esr

Ryan, heads-up about comment 25.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/62bc8ca444f9