951785 - [B2G][GPS] Geolocation requests coming from a browser tab don't complete

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Salem Elkabule]

Description:
The user is unable to see the location icon from the status bar after turning geolocation on in the setting page and going to  maps.google.com to make sure location is being used

Repro Steps:
1) Updated Buri 1.3 to Build ID: 20131218004002
2) Set phone to use GPS from the setting page
3) Open the browser
4) Go to https://maps.google.com/ or Here maps
5) Select "share" from the dialog to use your location

Actual:
The GPS icon does not appear in the status bar

Expected:
Status bar indicates GPS is being used

Environmental Variables
Device: buri 1.3 moz ril
Build ID: 20131218004002
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/369bdbff6c38
Gaia: a99b23e73fe5630a877004746d0e7fcec1b6d653
Platform Version: 28.0a2

Notes:
Repro frequency: 100%
Test Suite Name: GPS
Link to failed test case: https://moztrap.mozilla.org/manage/case/2125/

Comment 1

[Salem Elkabule]

This issue is not reproduce on buri 1.2 

Environmental Variables
Device: buri 1.2 moz ril
Build ID: 20131213004002
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a2b69b561d9b
Gaia: 1aca7c4860e39b1a9969807d335dcf9f070ea9b3
Platform Version: 26.0

Comment 2

[gbennett]

This was reproducing in the past in bug https://bugzilla.mozilla.org/show_bug.cgi?id=905520 which is still unconfirmed and has a regression window of it's own. Going to see if I can find the most recent regression window.

Comment 3

[Jason Smith [:jsmith]]

(In reply to gbennett from comment #2)
> This was reproducing in the past in bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=905520 which is still
> unconfirmed and has a regression window of it's own. Going to see if I can
> find the most recent regression window.

That bug is unrelated to this issue - this is talking about the geolocation indicator, not the fact that you can't find your location.

Comment 4

[gbennett]

(In reply to Jason Smith [:jsmith] from comment #3)
> That bug is unrelated to this issue - this is talking about the geolocation
> indicator, not the fact that you can't find your location.

Ah ok. Thanks for the heads up.

Comment 5

[gbennett]

.:Last Working Build:.
Environmental Variables:
Device: Buri 1.3 mozRIL
BuildID: 20131127040203
Gaia: d4b9a3d271f0451b4d903a03c2b931b8cc092041
Gecko: 6ecf0c4dfcbe
Version: 28.0a1
RIL Version:
Firmware Version: 20131115

.:First Broken Build:.
Environmental Variables:
Device: Buri 1.3 mozRIL
BuildID: 20131128040201
Gaia: 0d57ec2801ae125ec855a19cf956ab118660d694
Gecko: a5e7f611546f
Version: 28.0a1
RIL Version:
Firmware Version: 20131115

Comment 8

[Dietrich Ayala (:dietrich)]

Does this happen in 3rd party apps to? Or just content loaded via browser?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Does this happen in 3rd party apps to? Or just content loaded via browser?

Based on knowing the dupe, this will happen everywhere. It looks like geolocation requests aren't being sent out.

Comment 11

[Jason Smith [:jsmith]]

Looks like this isn't fixed per bug 968045.

Comment 13

[Jason Smith [:jsmith]]

Kau-Ru - Can you investigate this?

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Shouldn't we needinfo Fabrice instead, given the fact he was the original author of the Gecko patch? (bug 774580)

Comment 16

[Reuben Morais [:reuben]]

When the page tries to use the API, we have

  app  = PROMPT_ACTION
  page = UNKNOWN_ACTION

We then create and show the permission prompt, which sets |page = ALLOW_ACTION| when you choose "Share" or "Allow" or whatever. Then the request gets sent to the parent, which calls mozilla::CheckPermission to validate the principal. At this point, we have:

  app  = PROMPT_ACTION
  page = ALLOW_ACTION

Which is what we want. The problem is that CheckPermission is conservative when the app permission differs from the page permission, it always chooses the less permissive value. In this case, it chooses PROMPT over ALLOW: https://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp?rev=9a6150326f5f&mark=244,245,249,250#226

Jonas, is it OK to prefer ALLOW over PROMPT in this specific case? Or should we special case requests coming from a browser element?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, here's what I suggest:

When we determine if we should put up a prompt:

if page.principal.isInBrowserElement==true
  if perm(app.principal)!=ALLOW/PROMPT || perm(page.principal)==DENY
    deny
  if perm(page.principal)==ALLOW
    allow
  if perm(page.principal)==UNKNOWN/PROMPT
    prompt. Whatever answer user gives, save only for the page principal
  <are there cases not covered?>

if page.principal.isInBrowserElement==false
  if perm(app.principal)!=ALLOW/PROMPT || perm(page.principal)==DENY
    deny
  if perm(app.principal)==ALLOW && perm(page.principal)==ALLOW
    allow
  if perm(app.principal)==PROMPT || perm(page.principal)==PROMPT/UNKNOWN
    prompt. If user says 'allow' save for both page and app principals. If user says 'deny' only save
            for app principal
  <are there cases not covered?>


When we're in CheckPermission:

if page.principal.isInBrowserElement==true
  if perm(app.principal)!=ALLOW/PROMPT || perm(page.principal)==DENY
    deny
  if perm(page.principal)==ALLOW
    allow
  if perm(page.principal)==UNKNOWN/PROMPT
    deny
  <are there cases not covered?>

if page.principal.isInBrowserElement==false
  if perm(app.principal)!=ALLOW/PROMPT || perm(page.principal)==DENY
    deny
  if perm(app.principal)==ALLOW && perm(page.principal)==ALLOW
    allow
  if perm(app.principal)==PROMPT || perm(page.principal)==PROMPT/UNKNOWN
    deny
  <are there cases not covered?>


I *think* that gives somewhat consistent behavior.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The goal of the rules were:

1. For non-browser content, treat a "allow" anywhere as "allow for the app" and "deny" as "deny for the app".
2. When granting access to the app, don't automatically grant access to any off-app-origin pages (other than the one that asked of course).
3. For browser content, use the same principals as in the browser. I.e. only allow or deny for the asking origin.
4. Setting to "deny" in the settings UI should deny everywhere. 
5. Setting to "prompt" in the settings UI should prompt everywhere in non-browser content.
6. Setting to "allow" in the settings UI should allow for the app origin itself, but not automatically for any other origins.
7. Enable us to add settings UI which shows permission granted/denied to individual browser-content origins.

I don't think we can build a settings UI that explains everything without also exposing per-browser-content-origin permissions.

Comment 20

[Reuben Morais [:reuben]]

Attachment: For browser content, consider the permission of the requesting origin, not the requesting app (unless explicitly denied)

Thanks for the discussion and feedback. I turned some of the comments into code comments because they're excellent :)

We don't really have coverage for these edge cases, but here's a Try run anyway: https://tbpl.mozilla.org/?tree=Try&rev=7d80e2900e7b

Comment 21

[Reuben Morais [:reuben]]

For who people not following on IRC, the code that decides whether or not to prompt (ContentPermissionPrompt.js) already matches the rules in comment 18, that's why the patch only changes CheckPermission.

(In reply to Jonas Sicking (:sicking) from comment #18)
> When we determine if we should put up a prompt:
> if page.principal.isInBrowserElement==true
>   if perm(page.principal)==UNKNOWN/PROMPT
>     prompt. Whatever answer user gives, save only for the page principal

> When we're in CheckPermission:
> if page.principal.isInBrowserElement==false
>   if perm(app.principal)==PROMPT || perm(page.principal)==PROMPT/UNKNOWN
>     deny

We've also concluded that there's no need to special case this rule for CheckPermission; both ContentPermissionPrompt.js and CheckPermission can do the same thing. Callers of CheckPermission will likely handle PROMPT as DENY, but not necessarily.

Comment 22

[Reuben Morais [:reuben]]

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

Comment 23

[Preeti Raghunath(:Preeti)]

Once landed on m-c please request approval:

Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: 
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d422bf4841af

Comment 26

[Jason Smith [:jsmith]]

Note - this only resolves the browser case. I've reopened bug 974976 to fix the apps case here as well, as that doesn't work either.

Comment 27

[Reuben Morais [:reuben]]

Comment on attachment 8379402 [details] [diff] [review]
For browser content, consider the permission of the requesting origin, not the requesting app (unless explicitly denied)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 935361
User impact if declined: Geolocation doesn't work in the Browser app
Testing completed: Manually tested. This is also part of moztrap so is also tested in QA runs.
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 28

[Salem Elkabule]

This issue is fixed on buri v 1.4 Mozilla RIL

Environmental Variables
Device: Buri 1.4 MOZ RIL
Build ID: 20140224040201
Gecko: https://hg.mozilla.org/mozilla-central/rev/31113754db3b
Gaia: ffb527b84594396ed611edf0a8a5a130d60a742f
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

The Status bar indicates GPS is being used.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/bde435b7b7f1

Comment 31

[Mike Livingston]

Location icon in status bar appears correctly in 1.3, and 1.4

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140525024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: 94e367cf6c94
Version: 28.0
Firmware Version: v1.2-device.cfg

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg