713062 - Incorrectly using testExactPermssion to check some permission types

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P4)

Description

[:Margaret Leibovic]

Fennec is using testExactPermission for all permission types, but other consumers in gecko are using testPermission for most permission types. For some types (like "popup" and "geolocation") it only matters if we are consistent within /mobile, since that's the only place the permission is set/read, but other permissions (like "offline-app") are read in other places (like /dom).

On desktop, the only permission that uses testExactPermission is geolocation, and I feel like we should just be consistent with desktop, unless there's some compelling reason not to be. At the very least we should make sure we're doing the right thing for permissions that other modules are using.

Comment 1

[:Margaret Leibovic]

This bug is really old, and I don't think we actually care about fixing it. We can reopen if someone ever complains about us being too strict on checking permissions against subdomains.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I think consistency across Gecko/Fennec (and to a lesser degree, Fennec/Firefox) is important. Seems like a good first bug, potentially?

Comment 3

[:Margaret Leibovic]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> I think consistency across Gecko/Fennec (and to a lesser degree,
> Fennec/Firefox) is important. Seems like a good first bug, potentially?

I'm worried this might be tricky for a first bug, since it will involve auditing all the places we use testExactPermission/testPermission in our code, but you're right that I can make it into a mentor bug.

Doing some cursory MXRing, it does look like we should just change everything to use testPermssion, with the exception of geolocation, but I would like whoever takes this bug to do a more thorough investigation.

Comment 4

[Andrey]

MXR looks up just three occurences of testExactPermission and one of testPermission. Grep through whole source tree gets slightly more - I need to understand why. Anyway, the number of entries looks small. Since this activity assumes less or more wide code base investigation and does not seem as to be urgent, I'd like to own it, if you have no objections. For now I yank it.

Comment 5

[:Margaret Leibovic]

Andrey, thanks for offering to take this bug. Let me know if you have any questions about how to proceed!

Comment 6

[Andrey]

MXR shows there are 3 occurences of testExactPermission under mobile/ branch:
1. mobile/android/chrome/content/browser.js
    line 5820 -- let result = Services.perms.testExactPermission(BrowserApp.selectedBrowser.currentURI, "popup"); 

2. mobile/android/chrome/content/OfflineApps.js
    line 15 -- if (Services.perms.testExactPermission(currentURI, "offline-app") != Services.perms.UNKNOWN_ACTION) 

3. mobile/android/chrome/content/PermissionsHelper.js
    line 140 -- return Services.perms.testExactPermission(aURI, aType); 

I did not care about the real meaning yet, just checked for the consistence. And here is what I can see:

1. mobile/android/chrome/content/browser.js
This is in PopupBlockerObserver.onUpdatePageReport(event), which also is defined in
browser/metro/base/content/browser.js and does the same test.

2. mobile/android/chrome/content/OfflineApps.js
OfflineApps.offlineAppRequested(contentWindow)
The same is in
    browser/base/content/browser.js
    browser/metro/base/content/helperui/OfflineApps.js

3. mobile/android/chrome/content/PermissionsHelper.js
It's a "helper" function getPermission, that's supposed to be widely used for other types, but it clearly states:

    if (aType == "geolocation")
        return Services.perms.testExactPermission(aURI, aType);
    return Services.perms.testPermission(aURI, aType);

so not a concern though.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Last activity was almost one year ago. Resetting assignee.

Comment 8

[:Margaret Leibovic]

Let's just close this, I don't think it's worth fixing this.