955906 - Permissions are not granted for packaged apps

Status: VERIFIED FIXED

(Core :: Permission Manager, defect)

Description

[:gerard-majax]

While debugging Fil@Tours on B2G master running on HTC Desire Z, I noticed that Geolocation was broken. There have been reports of this on v1.3 on ZTE OPEN too.

After doing some binary bisect, I came accross a range of ok/nonok builds:
OK:
  <project name="gecko.git" path="gecko" remote="mozillaorg" revision="8288f8888d8f0feda156b51ef2849bf1a07aa815"/>
  <project name="gaia.git" path="gaia" remote="mozillaorg" revision="d4b9a3d271f0451b4d903a03c2b931b8cc092041"/>

NOK:
  <project name="gecko.git" path="gecko" remote="mozillaorg" revision="3be523c252aeac68f9fa91f962a6d02005f81839"/>
  <project name="gaia.git" path="gaia" remote="mozillaorg" revision="0d57ec2801ae125ec855a19cf956ab118660d694"/>


The git log for gecko shows:
$ git log --oneline 8288f8888d8f0feda156b51ef2849bf1a07aa815..3be523c252aeac68f9fa91f962a6d02005f81839
3be523c Bug 944141 - Temporarily disable all Valgrind suppressions for unintentional problems within Mozilla code, to see which ones are still necessary.
a732a15 Bug 944178 - Back out 6ca43b5171c1 (bug 943451) for breaking urlbar taps
7d81d80 Bug 944133 - Valgrind-on-TBPL: Suppress an intentional leak. NPOTB
117cd83 Bumping gaia.json for 4 gaia-central revision(s) a=gaia-bump
1ef091a Bumping gaia.json for 1 gaia-central revision(s) a=gaia-bump
216fc77 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
af1a999 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
7091171 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
0e14027 Bumping gaia.json for 3 gaia-central revision(s) a=gaia-bump
e703474 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
253a073 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
8984860 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
7f744c9 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump Truncated some number of revisions since the previous bump.
ae4e6d5 Merge m-c to b2g-inbound
d8c0330 Bumping gaia.json for 3 gaia-central revision(s) a=gaia-bump
2420946 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
ac221d1 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
e4ef10e Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
d68e609 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
e3cd73c Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
105aa84 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
db3e2c6 Backed out changeset 3436989a306e (bug 938466) for marionette webapi bustage on a CLOSED TREE
c6acc4f Backed out changeset 8ef36d96a366 (bug 938466) for marionette webapi bustage on a CLOSED TREE
f9e32f8 Backed out changeset abf315caa163 (bug 938466) for marionette webapi bustage on a CLOSED TREE
b3af651 Backed out changeset 274255d4107d (bug 938466) for marionette webapi bustage on a CLOSED TREE
a37cefc Bumping gaia.json for 4 gaia-central revision(s) a=gaia-bump
7333763 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
fffb3de Bumping gaia.json for 4 gaia-central revision(s) a=gaia-bump
0b87a4f Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
e610d49 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
8b7046d Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
a978df8 Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
17c8247 Bug 935361 - Use CheckPermission in the PContentParent Geolocation message handlers. r=dougt
91e4dd3 Bug 938466 - Part 4: Marionette for STK BIP message. r=yoshi
5e3c40a Bug 938466 - Part 3: Modify xpcshell BIP test case. r=yoshi
0e22770 Bug 938466 - Part 2: Modify STK BIP command const. r=yoshi
90cf103 Bug 938466 - Part 1: Correct STK BIP command const. r=yoshi, r=hsinyi
ec2393a Bug 824611 - Part 2: Modify xpcshell for ICCPDUHelper. r=yoshi
a7e8a7d Bug 824611 - Part 1: Add ICC PDU Helper. r=yoshi
7ba509d Bug 943350 - [B2G][DSDS] Gecko needs to properly download MMS for non-active SIM (follow-up fix). r=vicamo
0b34dbb Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
56d412f Bumping gaia.json for 3 gaia-central revision(s) a=gaia-bump
2ff1a9f Bumping gaia.json for 2 gaia-central revision(s) a=gaia-bump
c5cab30 Merge m-c to b2g-inbound
c800eff Bumping gaia.json for 3 gaia-central revision(s) a=gaia-bump

The only relevant and maybe related change seems to be:
17c8247 Bug 935361 - Use CheckPermission in the PContentParent Geolocation message handlers. r=dougt

I'm currently investigating and git bisecting to confirm this.

Comment 1

[:gerard-majax]

This, of course, is reproduced after doing some reset-gaia.

Comment 2

[:gerard-majax]

17c82478d7002b9bc4284ddb3342f396b87639d7 is the first bad commit
commit 17c82478d7002b9bc4284ddb3342f396b87639d7
Author: Reuben Morais <[e-mail]>
Date:   Wed Nov 27 02:28:16 2013 -0200

    Bug 935361 - Use CheckPermission in the PContentParent Geolocation message handlers. r=dougt

:040000 040000 b5687b8366486c2c1231ffaa7c1dc31635819fb2 c899745d1772fa79d217838da7b970dd630e9966 M	dom

Comment 3

[:gerard-majax]

Reuben, it seems your patch for bug 935361 is related to this issue.

Comment 4

[:gerard-majax]

Out of GDB I get that we are bailing out at https://hg.mozilla.org/mozilla-central/file/c8d5a871ae32/dom/ipc/AppProcessChecker.cpp#l229:

>   222   // Make sure that `aPermission' is an app permission before checking the origin.
>   223   nsCOMPtr<nsIPrincipal> appPrincipal = GetAppPrincipal(aPrincipal->GetAppId());
>   224   uint32_t appPerm = nsIPermissionManager::UNKNOWN_ACTION;
>   225   nsresult rv = pm->TestExactPermissionFromPrincipal(appPrincipal, aPermission, &appPerm);
>   226   NS_ENSURE_SUCCESS(rv, nsIPermissionManager::UNKNOWN_ACTION);
>   227   if (appPerm == nsIPermissionManager::UNKNOWN_ACTION ||
>   228       appPerm == nsIPermissionManager::DENY_ACTION) {
>   229     return appPerm;
>   230   }

Comment 5

[:gerard-majax]

However, the TestExactPermissionFromPrincipal() computes the correct value.

I/Gecko   ( 4504): nsPermissionManager: BEFORE:: aType=geolocation aPermission=0
I/Gecko   ( 4504): nsPermissionManager: BEFORE:: aType=geolocation aIncludingSession=1
I/Gecko   ( 4504): nsPermissionManager: AFTER:: aType=geolocation aPermission=1
I/Gecko   ( 4504): nsPermissionManager: BEFORE:: aType=geolocation aPermission=0
I/Gecko   ( 4504): nsPermissionManager: BEFORE:: aType=geolocation aIncludingSession=1
I/Gecko   ( 4504): nsPermissionManager: AFTER:: aType=geolocation aPermission=1

[...]

I/Gecko   ( 4504): CheckPermission: IN for aPermission=geolocation
I/Gecko   ( 4504): CheckPermission: Get service for aPermission=geolocation
I/Gecko   ( 4504): CheckPermission: checking for aPermission=geolocation
I/Gecko   ( 4504): CheckPermission: [BEFORE] appPerm=0 for aPermission=geolocation
I/Gecko   ( 4504): CheckPermission: [AFTER] appPerm=0 for aPermission=geolocation
I/Gecko   ( 4504): CheckPermission: out appPerm for aPermission=geolocation

Comment 6

[:gerard-majax]

Reuben, I'm not sure your previous patch is faulty, but rather that it exposes an underlying issue. I also have issues with systemXHR ...

Comment 7

[:gerard-majax]

Also noticed those in logcat:
E/GeckoConsole( 6680): Content JS LOG at app://system.gaiamobile.org/js/permission_manager.js:79 in pm_chromeEventHandler: XXX version < v1.2 does not support new permissions

Comment 8

[Jason Smith [:jsmith]]

Could you be more specific on what you mean by broken? Are you getting the permission prompt? Are you getting a location outside when requesting a geolocation permission?

Comment 9

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #8)
> Could you be more specific on what you mean by broken? Are you getting the
> permission prompt? Are you getting a location outside when requesting a
> geolocation permission?

Sorry, I thought it was clear:
 - I get the prompt
 - I allow the app
 - Then no location is found, no geolocation icon is present in status bar, and no gps request is sent to the low level stack

Comment 10

[:gerard-majax]

Forgot to mention that in the Settings, Permissions section, I can confirm that the Geolocation permission has been granted.

Comment 11

[Reuben Morais [:reuben]]

Hm, this is odd. You're probably right that it's exposing an underlying issue. I'll investigate.

Comment 12

[Jason Smith [:jsmith]]

Turns out there's already a bug on this. Will 1.3+ the dupe.

Comment 13

[an]

I don't think this is a duplicate of bug 951785 which refers to the *icon* of geolocation, not to the functionality of GPS. 
This bug is about the fact that on my zte open with v1.3 GPS simply doesn't work, with or without icon. I've tried several maps apps outdoors and none of them were able to use GPS, please make a note of this.
Geolocation worked with v1.0 that was originally on the phone.

Comment 14

[:gerard-majax]

This is clearly NOT a dupe, referring to https://bugzilla.mozilla.org/show_bug.cgi?id=951785#c3 which states that bug 951785 is about the icon itself.

I can confirm that there is NO location found at all, no request being retriggered at all.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> This is clearly NOT a dupe, referring to
> https://bugzilla.mozilla.org/show_bug.cgi?id=951785#c3 which states that bug
> 951785 is about the icon itself.
> 
> I can confirm that there is NO location found at all, no request being
> retriggered at all.

The root cause is exactly the same. comment 9 confirmed what the end-user would notice, which matches what is seen in bug 951785 in reference to the icon. The cause therefore is the same, so re-duping this.

Comment 16

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #15)
> (In reply to Alexandre LISSY :gerard-majax from comment #14)
> > This is clearly NOT a dupe, referring to
> > https://bugzilla.mozilla.org/show_bug.cgi?id=951785#c3 which states that bug
> > 951785 is about the icon itself.
> > 
> > I can confirm that there is NO location found at all, no request being
> > retriggered at all.
> 
> The root cause is exactly the same. comment 9 confirmed what the end-user
> would notice, which matches what is seen in bug 951785 in reference to the
> icon. The cause therefore is the same, so re-duping this.
> 
> *** This bug has been marked as a duplicate of bug 951785 ***

Great, what is the root cause then ?

Comment 17

[Jason Smith [:jsmith]]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #15)
> > (In reply to Alexandre LISSY :gerard-majax from comment #14)
> > > This is clearly NOT a dupe, referring to
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=951785#c3 which states that bug
> > > 951785 is about the icon itself.
> > > 
> > > I can confirm that there is NO location found at all, no request being
> > > retriggered at all.
> > 
> > The root cause is exactly the same. comment 9 confirmed what the end-user
> > would notice, which matches what is seen in bug 951785 in reference to the
> > icon. The cause therefore is the same, so re-duping this.
> > 
> > *** This bug has been marked as a duplicate of bug 951785 ***
> 
> Great, what is the root cause then ?

The root cause is the fact there isn't a request going out at all as you said above. If that doesn't happen, then the icon won't appear.

Comment 18

[:gerard-majax]

Okay, then we agree. Sorry to be picky, but why marking this bug as a duplicate while it is the one containing most informations: git bisect, first bad commit, etc. ?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> Okay, then we agree. Sorry to be picky, but why marking this bug as a
> duplicate while it is the one containing most informations: git bisect,
> first bad commit, etc. ?

I'll flip the dupe then.

Comment 22

[:gerard-majax]

I have geolocation that works on Twitter, though.

Comment 23

[Andre Natal]

On unagi on my  app (https://marketplace.firefox.com/app/voicity) I get the permission prompt but dont receive any coordinate. FF for Android works perfect.

Comment 24

[:gerard-majax]

Attachment: Debug

With the attached changes, I am getting the following output:

Running Twitter, tapping to send a message, allowing geolocation:
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: will read permission for geolocation.
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: has read permission for geolocation: 1

Running HERE Maps, allowing geolocation:
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 3
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: will read permission for geolocation.
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: has read permission for geolocation: 0
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: will read permission for geolocation.
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: has read permission for geolocation: 0
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: permission for geolocation: 1
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: will read permission for geolocation.
> I/Gecko   ( 1610): nsPermissionManager::CommonTestPermission: checking permission for geolocation
> I/Gecko   ( 1610): ContentParent::RecvAddGeolocationListener: has read permission for geolocation: 0

Comment 25

[:gerard-majax]

Okay, in the HERE Maps case I see that we are bailing out in http://hg.mozilla.org/mozilla-central/annotate/611698b4a246/extensions/cookie/nsPermissionManager.cpp#l1160 but not with Twitter.

Comment 26

[:gerard-majax]

GetPermissionHashKey() returns NULL.

Comment 27

[:gerard-majax]

With Twitter:
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved host for geolocation: mobile.twitter.com (0xbec1ef70)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved typeIndex for geolocation: 30 (0xbec1ef70)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved appId for geolocation: 1026 (0xbec1ef70)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved isInBrowserElement for geolocation: 0 (0xbec1ef70)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved entry for geolocation: 0x44c251b8 (0xbec1ef70)

With HERE Maps:
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved host for geolocation: marketplace.firefox.com (0xbec1ef74)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved typeIndex for geolocation: 30 (0xbec1ef74)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved appId for geolocation: 1027 (0xbec1ef74)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved isInBrowserElement for geolocation: 0 (0xbec1ef74)
> I/Gecko   ( 6640): nsPermissionManager::CommonTestPermission: retrieved entry for geolocation: 0x0 (0xbec1ef74)

Comment 28

[:gerard-majax]

According to Vivien, the manifestURL property of both apps in webapps.json explain this behavior:

For HERE Maps:
    "manifestURL": "https://marketplace.firefox.com/app/7eccfd71-2765-458d-983f-078580b46a11/manifest.webapp?feature_profile=3ebdd4ff37b6.46.3"

For Twitter:
    "manifestURL": "https://mobile.twitter.com/cache/twitter.webapp"

Comment 29

[:gerard-majax]

I can get geolocation permission working by changing appPrincipal to aPrincipal in the call at http://hg.mozilla.org/mozilla-central/annotate/611698b4a246/dom/ipc/AppProcessChecker.cpp#l225

Reuben, do you know if it's normal ?

Comment 30

[:gerard-majax]

After checking with Vivien, http://hg.mozilla.org/mozilla-central/annotate/611698b4a246/dom/ipc/AppProcessChecker.cpp#l187 getting the manifestURL in the host is consistent with the code: when we create the principal, it's getting initialized in GetAppPrincipal() with the value of the ManifestURL.

Comment 31

[Reuben Morais [:reuben]]

Attachment: Use app origin instead of manifest URL to get the app principal

Comment 32

[:gerard-majax]

(In reply to Reuben Morais [:reuben] from comment #31)
> Created attachment 8366352 [details] [diff] [review]
> Use app origin instead of manifest URL to get the app principal

Thanks, this matches what I had in mind but no time to write yesterday :). I'm building with the patch and I'll test once in the office and let you know.

Comment 33

[:gerard-majax]

Just tested, I do confirm that it fixes the geolocation permission in HERE Maps :)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a6150326f5f

Comment 35

[Jason Smith [:jsmith]]

Reuben - Can we get a mochitest written in a followup for this bug? This is a pretty serious regression, so I'd like to ensure that this doesn't happen again.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a6150326f5f

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80924242551d

Comment 38

[Reuben Morais [:reuben]]

Bug 965977.

Comment 39

[Jason Smith [:jsmith]]

Sanity tests with some of the PROMPT_ACTION permission showed this was working well on trunk.