Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Description • 7 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 1 • 7 years ago

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.

	Markus Stange [:mstange]
	Comment 3 • 7 years ago

Is there a workaround for this?

	Andrew Swan [:aswan]
	Comment 4 • 7 years ago

(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?

	Markus Stange [:mstange]
	Comment 5 • 7 years ago

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

	Andrew Swan [:aswan]
	Comment 6 • 7 years ago

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

	Andrew Swan [:aswan]
	Comment 7 • 7 years ago

(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.

	Sébastien Règne
	Comment 8 • 7 years ago

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

	me
	Comment 10 • 7 years ago

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

	Christoph Zwerschke
	Comment 11 • 7 years ago

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.

	Christoph Zwerschke
	Comment 12 • 7 years ago

> Can confirm this problem with 57.0.2

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

	Ian Moody [:Kwan] (UTC+1)
	Comment 13 • 6 years ago

(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.

	jscher2000
	Comment 14 • 6 years ago

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

	Andrew Swan [:aswan]
	Comment 17 • 6 years ago
mozreview-review

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?

	Kris Maglione [:kmag] Assignee
	Comment 18 • 6 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 19 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 20 • 6 years ago
bugherder

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

	Ian Moody [:Kwan] (UTC+1)
	Comment 21 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 24 • 6 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 25 • 6 years ago

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

	Will Bamberg [:wbamberg]
	Comment 26 • 6 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 27 • 6 years ago

(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.

	Julien Cristau [:jcristau] (back April 22)
	Comment 28 • 6 years ago

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.

	Julien Cristau [:jcristau] (back April 22)
	Comment 29 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 6 years ago
bugherder uplift

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