1432083 - browser.permissions.request doesn't work in WebExtension popup

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Alan Ramsey]

Attachment: addon.zip

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180104101725

Steps to reproduce:

- Include a browser action or page action popup in a WebExtension

- Put a match pattern in optional_permissions in the manifest

- From the popup, call browser.permissions.request with the match pattern in the origins key



Actual results:

The promise returned from browser.permissions.request never resolves.


Expected results:

The browser asks the user for permission and the promise resolves with true or false as described in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions/request

Comment 1

[Andrew Swan [:aswan]]

We have a collection of related bugs, they're all related to finding the enclosing browser window so we know where to anchor the permission notification.  I don't know off the top of my head exactly how to do it but I suspect there's some relatively easy way to get from the <browser> for an extension popup to the owning browser window.
In the mean time, I also filed bug 1433292 to make the failure mode here clearer.

Comment 2

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

(In reply to Andrew Swan [:aswan] from comment #1)
> We have a collection of related bugs, they're all related to finding the
> enclosing browser window so we know where to anchor the permission
> notification.  I don't know off the top of my head exactly how to do it but
> I suspect there's some relatively easy way to get from the <browser> for an
> extension popup to the owning browser window.
> In the mean time, I also filed bug 1433292 to make the failure mode here
> clearer.

-            let browser = context.pendingEventBrowser || context.xulBrowser;
+            let browser = context.pendingEventBrowser ||
+              windowTracker.topWindow.gBrowser.mCurrentBrowser;

At https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/toolkit/components/extensions/ext-permissions.js#44
seemed to fix it for me, but no idea if that's actually a good idea (I shelved it as "permissionsHackFix" for a reason).

Comment 3

[Andrew Swan [:aswan]]

I don't think topWindow is what we want, this varies from platform to platform but there are scenarios where you can interact with a window that's not on the top -- the notification should happen where the interaction took place, not whatever window happens to be on top.  I think that would also have a race where focus could change between the original user interaction and the time we get around to handling the call to request() in the main process.  And again, the notification should happen in the same window where the original interaction took place.

Comment 5

[Evgeniy Miklyaev]

Just want to add that if you permissions.request({permissions: ["cookies"]}) it works properly. If you permission.request only origins or origins + permissions then promise doesn't resolve as described by Alan.

Comment 6

[Vitaly Zdanevich]

The same issue - I do not see any popup that ask about new permission, while the same codebase works as expected on Chrome. Code example: 

    nodes.listFeed.onclick = function(mouseEvent) {
        chrome.permissions.request({
            'permissions': ['downloads']
        });
    }

In my manifest.json I have:
  "optional_permissions": [
    "downloads"
  ],


Checked on
63.0.3
and
65.0a1 (2018-11-28)

Comment 7

[Zhaolin Yu]

This bug is blocking some extension from being available on Firefox add-on store: https://github.com/sourcegraph/sourcegraph/issues/760

Comment 8

[nero120]

Confirmed in my extension as well. I'm executing:

browser.permissions.request({
  origins: ['http://*/', 'https://*/']
})

from a button click event handler in the browser action pop up. When clicked, the promise is neither resolved nor rejected and no error appears in the console. Fix this please.

Comment 10

[nero120]

(In reply to Andrew Swan [:aswan] (PTO until 3/11) from comment #3)

I don't think topWindow is what we want, this varies from platform to
platform but there are scenarios where you can interact with a window that's
not on the top -- the notification should happen where the interaction took
place, not whatever window happens to be on top. I think that would also
have a race where focus could change between the original user interaction
and the time we get around to handling the call to request() in the main
process. And again, the notification should happen in the same window where
the original interaction took place.

Andrew Swan this bug has been open for over a year! Do you plan on fixing it any time soon?

Comment 11

[Andrew Swan [:aswan]]

(In reply to nero120 from comment #10)

Andrew Swan this bug has been open for over a year! Do you plan on fixing it any time soon?

Bugs are prioritized based on their severity and impact, not how long they have been open. This one has been assigned P3, I have enough other higher priority work to spend any time on it now. If you're interested in contributing and working on a fix, I would be happy to mentor you though...

Comment 12

[nero120]

(In reply to Andrew Swan [:aswan] from comment #11)

(In reply to nero120 from comment #10)

Andrew Swan this bug has been open for over a year! Do you plan on fixing it any time soon?

Bugs are prioritized based on their severity and impact, not how long they have been open. This one has been assigned P3, I have enough other higher priority work to spend any time on it now. If you're interested in contributing and working on a fix, I would be happy to mentor you though...

I'd be happy to fix your bugs if you pay me for it. Otherwise I'd rather spend my free time working on my own open source project, thanks though guy.

Comment 13

[kernp25]

If you test the add-on from here first and then test the add-on from here,
you will see, that permissions.request() is not working anymore.

But if you test the add-on only, then you will see, that permissions.request() works.

Is this a normal behavior?
Should the add-on here not affect the other add-on?

Comment 14

[kernp25]

(In reply to kernp25 from comment #13)

If you test the add-on from here first and then test the add-on from here,
you will see, that permissions.request() is not working anymore.

I got my add-on working again, after i restarted my browser.

Comment 15

[Andrew Swan [:aswan]]

The (second) add-on in comment 13 doesn't do anything when installed. In any case, its not clear to me exactly what you're reporting, if you think you've found a new bug can you please explain clearly a sequence of steps for reproduction, what you expected to happen, and what actually happened.

Comment 16

[kernp25]

Attachment: MhJSfUlC1p.gif

Comment 17

[kernp25]

You must go to the add-on options.

Comment 18

[Vitaly Zdanevich]

Checked right now - not only from popup page permission is not asking but also if I ask permission from background page - also nothing.

Comment 19

[Manvel Saroyan]

This issue is currently blocking Privacy Manager from being supported on the Firefox.
https://github.com/Privacy-Managers/Privacy-Manager/issues/37#issuecomment-571081813

Hope it will be fixed soon.

Comment 20

[Manvel Saroyan]

This ticket is blocking me right now, I'll be happy to try fixing current issue, but I don't have experience contributing code to the actual Mozilla Firefox project and not sure yet what actually causes current issue.

(In reply to Andrew Swan [:aswan] from comment #11)

(In reply to nero120 from comment #10)

Andrew Swan this bug has been open for over a year! Do you plan on fixing it any time soon?

Bugs are prioritized based on their severity and impact, not how long they have been open. This one has been assigned P3, I have enough other higher priority work to spend any time on it now. If you're interested in contributing and working on a fix, I would be happy to mentor you though...

Andrew, are you or anyone else from the Firefox team still up for mentoring fixing current issue? Any initial guidelines or link to them will be much appreciated.

Comment 21

[Andrew Swan [:aswan]]

(In reply to Manvel Saroyan from comment #20)

Andrew, are you or anyone else from the Firefox team still up for mentoring fixing current issue? Any initial guidelines or link to them will be much appreciated.

Hi, I'm no longer working for Mozilla but still happy to help you with this. Current Mozilla employees who might also be able to help are :robwu, :rpl, :mixedpuppy, or :zombie.

For information about how to build and develop Firefox, this page has some good information: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

As for the specifics of this bug, the code in question is here:
https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/toolkit/components/extensions/parent/ext-permissions.js#68

I would suggest the following steps to acquaint yourself with what's happening:

1. Create a test extension that calls browser.permissions.request() from a popup and install it in a test profile
2. Open the browser toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) and set a breakpoint at the line above in ext-permissions.js
3. Take whatever action your extension needs to cause it to call permissions.request()

At this point, the browser will pause and you can inspect the state of the browser at that moment. If you switch to the Inspector tab in the browser toolbox, you can take a look at all the xul that backs the user interface. The important elements here are the xul <browser> elements. The code that displays the permission popup wants to receive the <browser> that holds the current tab contents but at this point all you have in context.xulBrowser is the <browser> that holds the popup. This is a bit of a dark art, you can experiment in the toolbox a bit but you probably want something like: context.xulBrowser.ownerGlobal.gBrowser.selectedBrowser

There's also the question of how to tell if the request is coming from a popup or not, I think you could use something like
if (context.xulBrowser.classList.contains("webextension-popup-browser"))

I think that understanding the relationship between the different elements here and how to navigate around the browser xul structure is the tricky part. Once you figure that out, the code change is likely to be relatively short. The wiki page linked above then has details about how to prepare and submit a patch.

That's a pretty whirlwind overview of everything, please feel free to ask questions about anything that isn't clear. Thanks for your interest in working on this and good luck!

Comment 22

[Manvel Saroyan]

(In reply to Andrew Swan [:aswan] from comment #21)

(In reply to Manvel Saroyan from comment #20)
Hi, I'm no longer working for Mozilla but still happy to help you with this.

Thank you, that's very nice of you.

Current Mozilla employees who might also be able to help are :robwu, :rpl, :mixedpuppy, or :zombie.

Noted.

At this point, the browser will pause and you can inspect the state of the browser at that moment. If you switch to the Inspector tab in the browser toolbox, you can take a look at all the xul that backs the user interface. The important elements here are the xul <browser> elements. The code that displays the permission popup wants to receive the <browser> that holds the current tab contents but at this point all you have in context.xulBrowser is the <browser> that holds the popup. This is a bit of a dark art, you can experiment in the toolbox a bit but you probably want something like: context.xulBrowser.ownerGlobal.gBrowser.selectedBrowser

At this point what I get is that context.xulBrowser returns null, rather than "<browser> that holds the popup". I'll play around to see, how I can identify if the request is coming from popup or whether I'm doing smth wrong, but if you have an idea why it's so, or need more info please let me know.

P.S. using Firefox Dev. edition v73.0b1.

Comment 23

[Manvel Saroyan]

(In reply to Manvel Saroyan from comment #22)

(In reply to Andrew Swan [:aswan] from comment #21)

(In reply to Manvel Saroyan from comment #20)

At this point, the browser will pause and you can inspect the state of the browser at that moment. If you switch to the Inspector tab in the browser toolbox, you can take a look at all the xul that backs the user interface. The important elements here are the xul <browser> elements. The code that displays the permission popup wants to receive the <browser> that holds the current tab contents but at this point all you have in context.xulBrowser is the <browser> that holds the popup. This is a bit of a dark art, you can experiment in the toolbox a bit but you probably want something like: context.xulBrowser.ownerGlobal.gBrowser.selectedBrowser

At this point what I get is that context.xulBrowser returns null, rather than "<browser> that holds the popup".

Not much progress here, currently blocked by this, basically both context.pendingEventBrowser and context.xulBrowser are null and not sure how to retrieve xul <browser> at this point.

Comment 24

[Andrew Swan [:aswan]]

(In reply to Manvel Saroyan from comment #22)

At this point what I get is that context.xulBrowser returns null, rather than "<browser> that holds the popup".

Huh, I'm not sure if this has always been the case or if something changed recently with Fission. zombie is much more up-to-date in this area than I am, lets see if he can shed some light...

Comment 25

[Manvel Saroyan]

Tagging (In reply to Andrew Swan [:aswan] from comment #24)

(In reply to Manvel Saroyan from comment #22)

At this point what I get is that context.xulBrowser returns null, rather than "<browser> that holds the popup".

Huh, I'm not sure if this has always been the case or if something changed recently with Fission. zombie is much more up-to-date in this area than I am, lets see if he can shed some light...

It has been a while since current comment, so I would like to try my luck and request info from :robwu and/or :rpl as well, just in case, before I give up.

Comment 26

[Rob Wu [:robwu]]

(In reply to Manvel Saroyan from comment #22)

At this point what I get is that context.xulBrowser returns null, rather than "<browser> that holds the popup". I'll play around to see, how I can identify if the request is coming from popup or whether I'm doing smth wrong, but if you have an idea why it's so, or need more info please let me know.

context.xulBrowser is null because you are closing the popup while debugging.
Set "Disable Popup Auto-Hide" (an option in the browser's toolbox) to false to prevent the popup from autoclosing, and context.xulBrowser will be non-null.

To debug further, put a breakpoint at https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/browser/modules/ExtensionsUI.jsm#366
and to make sure that the execution does not get stuck there, run the following snippet (either by running it from the console, or by using the snippet as a conditional breakpoint:

this.pendingNotifications.delete(window)

Then, to debug further, set a breakpoint at https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/toolkit/modules/PopupNotifications.jsm#596

You will see that isActiveBrowser is false after executing that line. I stopped debugging here, but this observation explains why the promise never resolves: PopupNotifications thinks that the browser is temporarily inactive, and waits until it regains the active state. That never happens, and the promise appears to be stuck.

To fix this bug, you should do one of the following:

• (if the permission warning should be anchored to the extension button): continue debugging where I left of, and make sure that the permission dialog opens, anchored to the extension button.
• (for consistency with permission.request calls from the background page): Use the active tab of the current window's browser (but reject if not found - https://bugzilla.mozilla.org/show_bug.cgi?id=1433292#c14 ). Basically what one of the first comments in this bug report suggests, except using the window with which the browserAction popup is associated instead of topWindow.

The last bullet point shows the easiest way to fix this bug.

Comment 27

[Manvel Saroyan]

Attachment: Bug 1432083 - Fixed browser.permissions.request doesn't work in WebExtension popup

See -> https://bugzilla.mozilla.org/show_bug.cgi?id=1432083

Comment 28

[Manvel Saroyan]

(In reply to Rob Wu [:robwu] from comment #26)

(In reply to Manvel Saroyan from comment #22)

Thanks Rob Wu.

Sorry for the late reply, it took longer than expected to build Firefox locally.

(for consistency with permission.request calls from the background page): Use the active tab of the current window's browser (but reject if not found - https://bugzilla.mozilla.org/show_bug.cgi?id=1433292#c14 ). Basically what one of the first comments in this bug report suggests, except using the window with which the browserAction popup is associated instead of topWindow.

Thinking that I have understood this comment only partially, but intuitively following your and Andrew's suggestions I have created https://phabricator.services.mozilla.com/D61411, hope it's good start to continue working on this.

Comment 29

[ericdude4]

(In reply to Manvel Saroyan from comment #28)

Hey Manvel, have you had any progress with this?

It's been my experience that the browser.permissions.request({origins: ['https://*/*']}) promise doesn't resolve when called in a popup, extension rendered page (in a separate tab), or in the extension options page.

It seems there is no way to request permissions with this function under any circumstances.

Eric

Comment 30

[Rob Wu [:robwu]]

Hi Eric, as you can see in the patch, there is some work in progress. Some patience would be appreciated.

(In reply to ericdude4 from comment #29)

It's been my experience that the browser.permissions.request({origins: ['https://*/*']}) promise doesn't resolve when called in a popup,

That is this bug.

extension rendered page (in a separate tab), or in the extension options page.

That is unexpected. Could you create a new bug, and attach a test case + video, so we can investigate it?

Comment 31

[ericdude4]

(In reply to Rob Wu [:robwu] from comment #30)

extension rendered page (in a separate tab), or in the extension options page.

That is unexpected. Could you create a new bug, and attach a test case + video, so we can investigate it?

Hi Rob, I made a bug report with a test case and a video here: https://bugzilla.mozilla.org/show_bug.cgi?id=1613796

Hopefully it's something you are able to re-produce!

Eric

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c196f0d65eab
Fixed browser.permissions.request doesn't work in WebExtension popup r=robwu

Comment 33

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be2a7d1a4d0d
Backed out changeset c196f0d65eab for browser chrome failures on browser_extension_sideloading.js.

Comment 34

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c196f0d65eab898c678ac9e1028384f652d0f455&selectedJob=288079256&searchStr=Linux%2Cx64%2Cdebug%2CMochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-11%2CM-fis%28bc11%29

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=288079256&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/be2a7d1a4d0d

[task 2020-02-09T04:56:32.959Z] 04:56:32 INFO - TEST-PASS | browser/base/content/test/webextensions/browser_extension_sideloading.js | Addon 3 should be enabled -
[task 2020-02-09T04:56:32.959Z] 04:56:32 INFO - Expect post install notification for "Test 3"
[task 2020-02-09T04:56:32.960Z] 04:56:32 INFO - Console message: [JavaScript Error: "TypeError: browser.getAttribute is not a function" {file: "resource:///modules/ExtensionsUI.jsm" line: 44}]
[task 2020-02-09T04:56:32.960Z] 04:56:32 INFO - Buffered messages finished
[task 2020-02-09T04:56:32.961Z] 04:56:32 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webextensions/browser_extension_sideloading.js | Test timed out -
[task 2020-02-09T04:56:32.961Z] 04:56:32 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-02-09T04:56:32.962Z] 04:56:32 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webextensions/browser_extension_sideloading.js | No unexamined telemetry after test is finished - Got 3, expected 0
[task 2020-02-09T04:56:32.962Z] 04:56:32 INFO - Stack trace:
[task 2020-02-09T04:56:32.962Z] 04:56:32 INFO - chrome://mochikit/content/browser-test.js:test_is:1320
[task 2020-02-09T04:56:32.963Z] 04:56:32 INFO - chrome://mochitests/content/browser/browser/base/content/test/webextensions/head.js:hookExtensionsTelemetry/<:677
[task 2020-02-09T04:56:32.963Z] 04:56:32 INFO - chrome://mochikit/content/browser-test.js:nextTest:570
[task 2020-02-09T04:56:32.963Z] 04:56:32 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1183
[task 2020-02-09T04:56:32.963Z] 04:56:32 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1130
[task 2020-02-09T04:56:32.964Z] 04:56:32 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:925
[task 2020-02-09T04:56:32.964Z] 04:56:32 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:808
[task 2020-02-09T04:56:32.964Z] 04:56:32 INFO - Not taking screenshot here: see the one that was previously logged

Comment 35

[Manvel Saroyan]

(In reply to Cosmin Sabou [:CosminS] from comment #34)

Bummer, sorry for that. Thinking that I should have run ./mach mochitest -f browser instead of just testing the latest test ./mach mochitest -f browser browser/components/extensions/test/browser/browser_ext_popup_requestPermission.js. (Thought all test are being run for each commit in phabricator).

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c196f0d65eab898c678ac9e1028384f652d0f455&selectedJob=288079256&searchStr=Linux%2Cx64%2Cdebug%2CMochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-11%2CM-fis%28bc11%29

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=288079256&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/be2a7d1a4d0d

[task 2020-02-09T04:56:32.959Z] 04:56:32 INFO - TEST-PASS | browser/base/content/test/webextensions/browser_extension_sideloading.js | Addon 3 should be enabled -
[task 2020-02-09T04:56:32.959Z] 04:56:32 INFO - Expect post install notification for "Test 3"
[task 2020-02-09T04:56:32.960Z] 04:56:32 INFO - Console message: [JavaScript Error: "TypeError: browser.getAttribute is not a function" {file: "resource:///modules/ExtensionsUI.jsm" line: 44}]

That's strange, apparently browser object that is being passed to https://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#40 not always has getAttribute() and/or classlist() methods available.

As a quick fix I'm thinking about:

if (browser.getAttribute && browser.getAttribute("webextension-view-type") == "popup") {
    browser = browser.ownerGlobal.gBrowser.selectedBrowser;
  }

Maybe :robwu knows why "TypeError: browser.getAttribute is not a function" could have happened in the context of browser_extension_sideloading.js and if there is something better we can do. Thanks in advance.

(Still investigating...)

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32fc8dce035e
Fixed browser.permissions.request doesn't work in WebExtension popup r=robwu

Comment 37

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/32fc8dce035e