1433292 - Make browser.permissions.request() reject when it can't find the browser window to anchor the notification

Status: NEW

(WebExtensions :: Frontend, enhancement, P3)

Description

[Andrew Swan [:aswan]]

We have several related bugs where calling browser.permissions.request() from various contexts leads to a situation where we can't find the browser window to place the permission notification.  In these cases, permissions.request() just fails silently, lets fix it to reject in these cases with a useful error message.

Comment 1

[ssirowa]

I would like to work on this bug. Can you please assign it to me?

Comment 2

[Andrew Swan [:aswan]]

Sure, and thanks for your interest!

Have you downloaded and built the Firefox source code locally?  If not, that's a good place to start:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
For this bug, artifact builds will be suitable and they will save you some time, disk space, etc:
https://developer.mozilla.org/en-US/docs/Artifact_builds

Once you're ready to work on the bug, the code in question is here:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-permissions.js#44

In this case the variable browser is a XUL <browser> element and we want to know if it corresponds to a browser tab or to something else (like a popup, sidebar, etc.)  I think the test to add there is something like:
```
if (browser.ownerGlobal.gBrowser && browser.ownerGlobal.gBrowser.getTabForBrowser(browser)) {
}
```

You can start out by testing this manually, once you've got something working we can talk about how to write an automated test.

It looks like you already know how to use the needinfo flag, please feel free to use it for any questions that you have along the way.
Thanks, good luck, and don't hesitate to ask questions!

Comment 3

[Caitlin Neiman [:caitmuenster]]

Hey ssirowa! Just wanted to check in and see how it's going with this bug.

Comment 4

[ssirowa]

I was a bit busy with exams. I will submit the patch by Tuesday.
I apologize for the delay.

Comment 5

[Caitlin Neiman [:caitmuenster]]

Hey ssirowa! Because it's been awhile since we've heard from you, we're going to unassign you from this bug and open it up to other contributors. 

If you'd still like to submit a patch, please feel free to re-assign yourself and submit!

Comment 6

[Amy Chan (:amychan331)]

Can I work with this bug? Also, I notice the link above (https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-permissions.js#44) is now 404. I did a search, and I am wondering if it is moved to https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-permissions.js.

Comment 7

[Tomislav Jovanovic :zombie]

Hi Amy,

Yeah you can work on this, thanks.  And you are correct, that file recently moved, and you've got the right one.

If this is your first time trying to code on firefox, check out this (and linked) pages for instructions:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

If you have any questions, feel free to need-info me or Andrew.

Comment 8

[Andrew Swan [:aswan]]

Amy, thanks for your interest.  The patches that recently landed for bug 1382953 may have made this bug irrelevant, so a good first step would be to see if you can reproduce it at all.  That would entail writing a test extension that uses the permissions.request() api from various types of events (context menu clicks, sidebars, options pages, etc) and seeing if it fails to show a prompt to the user in any of those cases.

As Tom said, please feel free to use needinfo if you have more questions (click the "Need more information" box at the bottom of this page when adding a comment and type either :aswan or :zombie into the text box there)

Comment 9

[Amy Chan (:amychan331)]

I wrote a test extension that use permission.request() in normal pages, context menus, options pages, and sidebars. The first three worked and prompted the user for permission, but when I request permission in the sidebar, I got an "JavaScript error: moz-extension://5714f483-cd0c-dc45-882c-930b5b97849b/sidebar/panel.js, line 16: TypeError: browser.tabs is undefined" error in my terminal's console. Is that the intended results and this bug is irrelevant, or is a different result desired?

Comment 10

[Andrew Swan [:aswan]]

Can you share the full code for your test extension somewhere?

Comment 11

[Amy Chan (:amychan331)]

I haven't really done much with extension before, so all I did was use mdn's own extension demo examples (https://github.com/mdn/webextensions-examples) and added link that request permission - I requested history permission in this case. I put the code in https://github.com/amychan331/testing-webextension. Directory extension-permission combines mdn's permissions and menu-demo examples, while the extension-options-sidebar directory combines the annotate-page and favourite-colour.

Comment 12

[Andrew Swan [:aswan]]

When I load your sample extension, I don't get the browser.tabs error you reported in comment 9 but I do get an error since the code to work back from the sidebar page where extension is running to the surrounding chrome window doesn't do the right thing.

That code is here:
https://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#29-34

Its a little crude but an expedient fix would be to add an additional check such as
`if (browser.getAttribute("id") == "webext-panels-browser")`

But first we have to get past the browser.tabs error, what Firefox version are you running?

Comment 13

[Amy Chan (:amychan331)]

Interesting - now that I try it again, I am getting the error from ExtensionsUI.jsm too. The error message is now "JavaScript error: resource:///modules/ExtensionsUI.jsm, line 400: TypeError: window.PopupNotifications is null". I wonder if I did something and saved it without realizing it?

As for the Firefox version, I am using Nightly, 61.0a1.

Comment 14

[Rob Wu [:robwu]]

Sidebar bug was reported in bug 1493396.

I just tested permissions.request on Nightly (same version as that other bug):
- Background page: Anchored to current tab.
- Popup: Silent failure, no errors in console.
- Sidebar: Rejected - bug 1493396
- Options page: untested, supposedly works after bug 1382953

Comment 15

[Rob Wu [:robwu]]

Bug for popup is here: bug 1432083

Comment 16

[Arica Chakraborty]

Hey, I would like to work on this bug.

Comment 17

[Caitlin Neiman [:caitmuenster]]

Hey Arica, go for it! If you haven't already done so, you might want to look at the onboarding documentation here: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

If you need any help, please need-info :aswan. 

Happy bug fixing!

Comment 18

[Arica Chakraborty]

Thanks,i'm going through the available material to set-up my environment and get started.

Comment 19

[Caitlin Neiman [:caitmuenster]]

Hey Arica, how's it going with this bug?

Comment 20

[Suhail Singh Bains]

Hi,
I would like to work on this bug. Can you please assign it to me!
Thanks

Comment 21

[Suhail Singh Bains]

Attachment: Bug 1433292 - Make Browser.permissions.request() reject when it can't find the browser window to anchor the notification, r=aswan

Comment 22

[Suhail Singh Bains]

Hi
The code has to be changed in getTabBrowser function, if I am not wrong.
Please correct me if I am missing something.
Thanks

Comment 23

[Andrew Swan [:aswan]]

Do you mean getTabForBrowser() ? If so, I'm not sure what you're suggesting, what would you change in that function?

Comment 24

[Caitlin Neiman [:caitmuenster]]

Hey Suhail, how's it going with this bug? Can we help unblock you?

Comment 25

[Suhail Singh Bains]

Attachment: Bug 1433292 - Browser.permissions.request() reject when it can't find the browser window to anchor the notification, r=aswan

Comment 26

[Suhail Singh Bains]

Attachment: Bug 1433292 - Browser.permissions.request() reject when it can't find the browser window to anchor the notification, r=aswan

Comment 27

[Suhail Singh Bains]

Attachment: Bug 1433292 - Browser.permissions.request() reject when it can't find the browser window to anchor the notification, r=aswan

Comment 28

[Caitlin Neiman [:caitmuenster]]

Hey Suhail, it looks like you're getting close! Aswan added some comments in Phabricator -- want to take a look at those and finish this off?

Comment 29

[Mark Striemer [:mstriemer]]

I'll help mentor this patch now. Let me know if you have any questions.

Comment 30

[Caitlin Neiman [:caitmuenster]]

Per conversation over IRC with Suhail, this bug is being re-opened for other contributors. If you would like to work on this, please submit a patch via Phabricator; once your patch has been submitted, we will assign you to the bug.

Comment 31

[Ajitesh13]

Hey, is this bug still open ? Can i work in this bug ?

Comment 32

[Thomas Oberndörfer]

Not sure if this is related, but in my case browser.permissions is undefined in a certain context (iframe with src="moz-extension://...").

Comment 33

[Andrew Swan [:aswan]]

(In reply to Thomas Oberndörfer from comment #32)

Not sure if this is related, but in my case browser.permissions is undefined in a certain context (iframe with src="moz-extension://...").

That is an unrelated bug: bug 1443253

Comment 34

[Naimat Oyewale]

Hi Rob and Luca. I am an outreachy intern, can I work on this bug please?

Comment 35

[Caitlin Neiman [:caitmuenster]]

Hi Naimat, feel free to work on this bug! We'll assign it to you once you have a PR. Here's a link to our onboarding documentation to help you get started: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Comment 36

[Shulammite]

Hello Rob Wu, I am an Outreachy applicant and I'd like to work on this please

Comment 37

[Ankush Dua]

Hi, Is someone working on this?, If not I would love to take on this issue :)

Comment 38

[Ankush Dua]

Hi, Just a friendly reminder for this comment#37

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

No contributor is currently working on this issue at the moment, redirecting the needinfo to me to look if previous contributor got stuck because of some lack of guidance details.

Comment 40

[Rob Wu [:robwu]]

https://phabricator.services.mozilla.com/D98574 is already working towards a fix that may also fix this bug.

bug 1493396 is the other remaining bug (from the list in comment 14).

Comment 41

[kernp25]

Should i fix viewType == "background" in this bug, but still check gBrowser.getTabForBrowser(browser) and reject with an error message?

Comment 42

[kernp25]

Created bug 1763915.