Closed Bug 1432083 Opened 6 years ago Closed 4 years ago

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

Categories

(WebExtensions :: Frontend, defect, P3)

57 Branch
defect

Tracking

(firefox75 fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: ramseya, Assigned: saroyanm, Mentored)

References

Details

(Whiteboard: [permission])

Attachments

(3 files)

Attached file 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
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
See Also: → 1382953, 1422605
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.
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
(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).
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.
Product: Toolkit → WebExtensions
See Also: → 1493396
See Also: → 1433292
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.
Whiteboard: [permission]
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)
This bug is blocking some extension from being available on Firefox add-on store: https://github.com/sourcegraph/sourcegraph/issues/760

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.

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

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

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

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?

Flags: needinfo?(andrew.swan)

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

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.

Flags: needinfo?(andrew.swan)
Attached image MhJSfUlC1p.gif

You must go to the add-on options.

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

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.

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.

Flags: needinfo?(andrew.swan)

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

Flags: needinfo?(andrew.swan)

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

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

Flags: needinfo?(andrew.swan)

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

Flags: needinfo?(andrew.swan) → needinfo?(tomica)

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.

Flags: needinfo?(rob)
Flags: needinfo?(lgreco)

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

Mentor: rob
Flags: needinfo?(tomica)
Flags: needinfo?(rob)
Flags: needinfo?(lgreco)
Assignee: nobody → saroyanm

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

(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

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?

(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

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c196f0d65eab
Fixed browser.permissions.request doesn't work in WebExtension popup r=robwu
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be2a7d1a4d0d
Backed out changeset c196f0d65eab for browser chrome failures on browser_extension_sideloading.js.

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

Flags: needinfo?(saroyanm)

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

Flags: needinfo?(saroyanm) → needinfo?(rob)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32fc8dce035e
Fixed browser.permissions.request doesn't work in WebExtension popup r=robwu
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: