[System] Asking for more than one permission in a short period of time is broken

VERIFIED FIXED in 2.2 S9 (3apr)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: borjasalguero, Assigned: borjasalguero)

Tracking

({late-l10n})

unspecified
2.2 S9 (3apr)
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
When asking an array of permissions in Gaia in a short period of time, the status of the prompts is broken.

For testing in master, I've added a test app called 'permision_tester' [1] which is asking for the following permission sequentially:

1) device-storage:sdcard: Save a random file
2) geolocation: Just print the current location
3) camera: It takes a random photo
4) device-storage:pictures: Store the photo in the device

With this app you can check the following STRs.

STR 1:
- Open 'permission_tester'
- Accept 'sdcard' permission with "remember me"
- Accept 'geolocation' permission with "remember me"


EXPECTED:
Camera prompt must be shown, and then "device-storage:pictures" prompt must be shown (when storing the photo taken using Camera API)

CURRENTLY:
Camera prompt *is not shown*, and *camera permission is granted with the previous prompt, without asking to the user* (we get access to 'Camera' for free, just accepting the previous one with 'remember me').

STR 2:
- Open 'permission_tester'

EXPECTED:
Every prompt is using the right icon, and all texts are available.

CURRENTLY:
SDCard prompt must show the *right icon*, but actually it's showing a 'camera' icon (icon is always the last prompt requested!). If you move forward accepting the prompts *without 'remember me'*, when reaching camera you will notice that this prompt has no text.

Basically the prompts are corrupted due to the 'queue' of prompts is broken, so when requesting several permissions in a short period of time the app is not going to behave properly.


[1] https://github.com/borjasalguero/permission_tester
(Assignee)

Comment 1

4 years ago
Posted file WIP (obsolete) —
This is just a WIP patch, which is keeping the right status of the queue, so all prompts are shown as they should.
Alive, could you take a look? I would really appreciate it! 谢谢! ;)
Attachment #8579475 - Flags: feedback?(alive)
(Assignee)

Updated

4 years ago
Assignee: nobody → borja.bugzilla
Is this also happening on 2.2?
Keywords: qawanted
(Assignee)

Comment 3

4 years ago
Hi Gregor! I can reproduce the same in 2.2 (both STRs) in my Flame... Should we ask for any specific flag?
Flags: needinfo?(anygregor)
blocking-b2g: --- → 2.2?
Flags: needinfo?(anygregor)
Whiteboard: [systemsfe]
Naoki, do we have any tests for this?
Flags: needinfo?(nhirata.bugzilla)
I can confirm that both issues in comment 0 occur on Flame 3.0 and 2.2.  

Environmental Variables:
Device: Flame 3.0
BuildID: 20150318055339
Gaia: b8051d370ddf4e5bd8e7d8a19fb9eeb5fd6ffb39
Gecko: 41a61514461e
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150318075645
Gaia: 6a58c433c53c25e96e067a9b38bf1cc17307d5b2
Gecko: 70b8707bc5d2
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

I attempted to use the app on 2.1 per our normal branch check work as well but it hangs after the device storage permission pop up.

Environmental Variables:
Device: Flame 2.1
BuildID: 20150317070128
Gaia: 13c85d57f49b4bfd657ff674f2b530c141c94803
Gecko: 2fbd284621e2
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
We have tests for individual permissions; I am not sure about the multiple dialogs in a short time.  

Something to look at is jsmith's web permissions test app.
https://github.com/nhirata/qa-testcase-data/blob/gh-pages/webapi/apps/webpermissionstestapp.manifest

Personally I think it's bad ux for multiple dialogs for various permissions rather than having one permissions dialog for all....

pinging ux to see what they think and how the ux should go for multiple permission asks.
Flags: needinfo?(nhirata.bugzilla) → needinfo?(firefoxos-ux-bugzilla)

Comment 7

4 years ago
I don't believe that all of the permissions *can* get thrown without the prompts. 

If there's anything specific that UX should ui-review? in this patch, please re-flag accordingly.
Flags: needinfo?(firefoxos-ux-bugzilla)
Comment on attachment 8579475 [details] [review]
WIP

Much Ttanks for cleaning the permission dialog! I took a quick look but don't see big problem. Fred, please take a look as well.
Attachment #8579475 - Flags: feedback?(gasolin)
Attachment #8579475 - Flags: feedback?(alive)
Attachment #8579475 - Flags: feedback+
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8579475 [details] [review]
WIP

Applied the patch and found following issue

1. open UI tests app
2. go to API/getUserMedia panel
3. press video

The `More info` tag is shown, but it should be hidden
Attachment #8579475 - Flags: feedback?(gasolin)
(Assignee)

Comment 10

4 years ago
I'll update the code in order to fix this issue as well. Once the code will be ready, I'll ask you both for the final review. Thanks!! :)
(Assignee)

Comment 11

4 years ago
(In reply to Fred Lin [:gasolin] from comment #9)
> Comment on attachment 8579475 [details] [review]
> WIP
> 
> Applied the patch and found following issue
> 
> 1. open UI tests app
> 2. go to API/getUserMedia panel
> 3. press video
> 
> The `More info` tag is shown, but it should be hidden

This seems to be fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1144577, so I'll double check after rebasing.
(Assignee)

Comment 12

4 years ago
Posted file Pull Request (obsolete) —
Attachment #8579475 - Attachment is obsolete: true
Attachment #8581756 - Flags: review?(alive)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8579475 [details] [review]
WIP

Hi! This is the final patch. I've updated the code with your suggestions, and I've added some more tests. Could you take a look? 谢谢! Gracias! ;)
Attachment #8579475 - Flags: review?(gasolin)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8581756 [details] [review]
Pull Request

Adding the right Patch to be reviewed :)
Attachment #8581756 - Flags: review?(gasolin)
(Assignee)

Updated

4 years ago
Attachment #8579475 - Flags: review?(gasolin)
Comment on attachment 8581756 [details] [review]
Pull Request

Looks good to me, run on device without error,
Some nits are comment on github, please fix them before landing (and should be passed the treeherder tests).
Attachment #8581756 - Flags: review?(gasolin) → review+
Comment on attachment 8581756 [details] [review]
Pull Request

I am forwarding this review to Fred and since he already r+ this you have my r+ as well. Thank you for working on this!
Attachment #8581756 - Flags: review?(alive)
(Assignee)

Comment 17

4 years ago
The patch is just updated with last bits fixed, waiting for treeherder to merge. Thanks Alive and Fred for the quick review!
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

4 years ago
Comment on attachment 8581756 [details] [review]
Pull Request

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Permission Manager were not handling properly the queue of 'permissions prompt', so the UI was broken and some of the permissions were not handled properly (some of them were accepted even with asking to the user)

[User impact] if declined:
Permission management would be broken, so a malicious app could handle permission without asking to the user (check STR1 in [1] for an example)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1144784#c0

[Testing completed]:
I've added some more unit tests in order to ensure that all scenarios are working as expected, and some manual tests were done as well.

[Risk to taking this patch] (and alternatives if risky):
Minimal risk. Currently we have a regression which is fixed with this patch.

[String changes made]:
Added "perm-camera-appRequest"[1] & "perm-camera-webRequest"[2] in order to handle 'camera' permission properly (there was no string before)

[1]https://github.com/borjasalguero/gaia/blob/fix_permission_manager_wip/shared/locales/permissions/permissions.en-US.properties#L30

[2]https://github.com/borjasalguero/gaia/blob/fix_permission_manager_wip/shared/locales/permissions/permissions.en-US.properties#L31
Attachment #8581756 - Flags: approval-gaia-v2.2?
Target Milestone: --- → 2.2 S9 (3apr)
Reverted for breaking gaia integration tests. (Please try to land with autolander and checkin-needed to prevent this in the future)

Failure: https://treeherder.mozilla.org/logviewer.html#?job_id=1568528&repo=b2g-inbound

Backout: https://github.com/mozilla-b2g/gaia/commit/1e2fde597d599404c926f9ce73ce15d34598c2db
Status: RESOLVED → REOPENED
Flags: needinfo?(borja.bugzilla)
Resolution: FIXED → ---
Comment on attachment 8581756 [details] [review]
Pull Request

Clearing approval until this sticks.
Attachment #8581756 - Flags: approval-gaia-v2.2?
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Duplicate of this bug: 1113439
(Assignee)

Comment 27

4 years ago
Comment on attachment 8582469 [details] [review]
[gaia] borjasalguero:fix_permission_manager_wip > mozilla-b2g:master

Hi Alive,
In order to use the 'autolander', I would need a 'suggested' reviewer giving the r+. As Fred does not appear in the list, I would ask you in order to follow the steps described by Kevin, and avoid any possible issue with integration tests. Now 'treeherder' is all green [1], so let's wait to your review for merging it properly!

@Kevin: Sorry for the noise generated, I did not realized that the `checkin-needed` flag was tied to asking review to a 'suggested reviewer', that's why I merged manually. I'll keep an eye on this next time! Thanks!


[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=56d83f2bb607fb07c438f1e8c495978c671653ca
Flags: needinfo?(borja.bugzilla)
Attachment #8582469 - Flags: review?(alive)
Comment on attachment 8582469 [details] [review]
[gaia] borjasalguero:fix_permission_manager_wip > mozilla-b2g:master

No worries. Looking at this again, this actually looks like a bug with autolander. Since Alive is a peer, and forwarded the review - we should allow it :) 

For now I'll go ahead and add my review here so you can leverage autolander if you wish. Thanks!
Attachment #8582469 - Flags: review?(alive) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
(Assignee)

Updated

4 years ago
Attachment #8581756 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

4 years ago
Comment on attachment 8582469 [details] [review]
[gaia] borjasalguero:fix_permission_manager_wip > mozilla-b2g:master

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Permission Manager were not handling properly the queue of 'permissions prompt', so the UI was broken and some of the permissions were not handled properly (some of them were accepted even with asking to the user)

[User impact] if declined:
Permission management would be broken, so a malicious app could handle permission without asking to the user (check STR1 in [1] for an example)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1144784#c0

[Testing completed]:
I've added some more unit tests in order to ensure that all scenarios are working as expected, and some manual tests were done as well.

[Risk to taking this patch] (and alternatives if risky):
Minimal risk. Currently we have a regression which is fixed with this patch.

[String changes made]:
Added "perm-camera-appRequest"[1] & "perm-camera-webRequest"[2] in order to handle 'camera' permission properly (there was no string before)

[1]https://github.com/borjasalguero/gaia/blob/fix_permission_manager_wip/shared/locales/permissions/permissions.en-US.properties#L30

[2]https://github.com/borjasalguero/gaia/blob/fix_permission_manager_wip/shared/locales/permissions/permissions.en-US.properties#L31
Attachment #8582469 - Flags: approval-gaia-v2.2?
Depends on: 1144577
Duplicate of this bug: 1144642
Keywords: late-l10n
Attachment #8582469 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Just adding that we need bug 1144577 to be uplifted to 2.2 before landing this patch in 2.2 branch.
In the future, it would be good if said patch was nominated for approval in that case.
flame
eng
master
Gecko-a2b428e
Gaia-3970bcf

STR 1:
- Open 'permission_tester'
- Accept 'sdcard' permission with "remember me"
- Accept 'geolocation' permission with "remember me"

CURRENTLY:
Camera prompt must be shown, and then "device-storage:pictures" prompt must be shown (when storing the photo taken using Camera API)

STR 2:
- Open 'permission_tester'

CURRENTLY:
Every prompt is using the right icon, and all texts are available.
Tested and working:
flame
eng
v2.2
Gecko-6ea0017
Gaia-e59ac06
Status: RESOLVED → VERIFIED
(Assignee)

Updated

4 years ago
Blocks: 1152917
No longer depends on: 1152917
You need to log in before you can comment on or make changes to this bug.