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

VERIFIED FIXED in Firefox 30, Firefox OS v1.3

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: selkabule, Assigned: reuben)

Tracking

(Blocks: 1 bug, {regression, verifyme})

28 Branch
mozilla30
ARM
Gonk (Firefox OS)
regression, verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox30 fixed, b2g-v1.2 unaffected, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 verified)

Details

(Whiteboard: burirun1.3-1, [systemsfe], URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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/
(Reporter)

Comment 1

5 years ago
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

Updated

5 years ago
blocking-b2g: --- → 1.3?

Updated

5 years ago
QA Contact: gbennett

Comment 2

5 years ago
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.
(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

5 years ago
(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

5 years ago
.: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
Keywords: regressionwindow-wanted

Updated

5 years ago
Duplicate of this bug: 955906

Updated

5 years ago
Blocks: 935361
blocking-b2g: 1.3? → 1.3+
Component: Gaia::System → Geolocation
Depends on: 956031
Product: Firefox OS → Core
Version: unspecified → 28 Branch

Updated

5 years ago
Duplicate of this bug: 955906
Does this happen in 3rd party apps to? Or just content loaded via browser?
(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.

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 955906
Looks like this isn't fixed per bug 968045.
Status: RESOLVED → REOPENED
No longer depends on: 956031
Resolution: DUPLICATE → ---

Updated

5 years ago
Duplicate of this bug: 968045
Kau-Ru - Can you investigate this?
Flags: needinfo?(kchen)
Shouldn't we needinfo Fabrice instead, given the fact he was the original author of the Gecko patch? (bug 774580)
Flags: needinfo?(fabrice)
(Assignee)

Updated

5 years ago
Assignee: nobody → reuben.bmo
Summary: [B2G][GPS] Location icon does not show in the status bar → [B2G][GPS] Geolocation requests coming from a browser tab don't complete

Updated

5 years ago
Flags: needinfo?(kchen)
Flags: needinfo?(fabrice)
Whiteboard: burirun1.3-1 → burirun1.3-1, [systemsfe]

Updated

5 years ago
Duplicate of this bug: 965191
(Assignee)

Comment 16

5 years ago
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?
Flags: needinfo?(jonas)

Updated

5 years ago
Duplicate of this bug: 974976
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.
Flags: needinfo?(jonas)
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.
(Assignee)

Comment 20

5 years ago
Created attachment 8379402 [details] [diff] [review]
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
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #8379402 - Flags: review?(jonas)
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]:
https://hg.mozilla.org/mozilla-central/rev/d422bf4841af
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

5 years ago
Duplicate of this bug: 975674
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.
(Assignee)

Comment 27

5 years ago
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
Attachment #8379402 - Flags: approval-mozilla-b2g28?
(Assignee)

Updated

5 years ago
Blocks: 975751
Keywords: verifyme
(Reporter)

Comment 28

5 years ago
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.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Attachment #8379402 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+

Updated

5 years ago
status-firefox30: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/bde435b7b7f1
status-b2g-v1.3: affected → fixed
status-b2g-v1.4: --- → fixed

Updated

5 years ago
Duplicate of this bug: 965191
status-b2g-v1.3T: --- → fixed

Comment 31

5 years ago
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
status-b2g-v1.3: fixed → verified
status-b2g-v1.4: fixed → verified
You need to log in before you can comment on or make changes to this bug.