Nightly/Aurora updater requires Storage permission

VERIFIED FIXED in Firefox 46

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 46
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We can't update Nightly/Aurora builds until we have the Storage permission.
(Assignee)

Updated

2 years ago
Blocks: 1216537
(Assignee)

Comment 1

2 years ago
There are many possibilities:

1) (From the UpdateService) Show a notification that reminds the user to enable this permission in order to get updates.

2) Show a Snackbar in the app like "Nightly requires the STORAGE permission to download updates | ACCEPT"

3) Download updates to the cache directory of the application if we do not have the STORAGE permission (does not need permissions; but might have less storage space)

I'll implement (1) first now because this will catch all use cases. (2) is nice but we can do this in a follow-up. (3) sounds smart but we should take care to not pollute the application directory - and we still want to have (1) and (2).
(Assignee)

Comment 2

2 years ago
Created attachment 8711042 [details]
MozReview Request: Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r=nalexander

Review commit: https://reviewboard.mozilla.org/r/31959/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31959/
(Assignee)

Comment 3

2 years ago
Created attachment 8711043 [details]
MozReview Request: Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r=nalexander

Review commit: https://reviewboard.mozilla.org/r/31961/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31961/
(Assignee)

Comment 4

2 years ago
Created attachment 8711044 [details]
MozReview Request: Bug 1241887 - (Part 3) Show snackbar to ask for Storage permission if updater is enabled. r?

Review commit: https://reviewboard.mozilla.org/r/31957/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31957/
(Assignee)

Updated

2 years ago
Attachment #8711042 - Flags: review?(nalexander)
(Assignee)

Updated

2 years ago
Attachment #8711043 - Flags: review?(nalexander)
(Assignee)

Updated

2 years ago
Attachment #8711044 - Flags: review?(nalexander)
(Assignee)

Comment 5

2 years ago
Those three patches are built on top of "part 1" from bug 1240711 (doNotPrompt()).
Comment on attachment 8711042 [details]
MozReview Request: Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r=nalexander

https://reviewboard.mozilla.org/r/31959/#review28723

Prefer static checking by changing the API (but I can live with this).

::: mobile/android/base/java/org/mozilla/gecko/permissions/PermissionBlock.java:27
(Diff revision 1)
> -    /* package-private */ PermissionBlock(Activity activity, PermissionsHelper helper) {
> +    /* package-private */ PermissionBlock(Context context, PermissionsHelper helper) {

Oh hey, I was just talking about this.

::: mobile/android/base/java/org/mozilla/gecko/permissions/PermissionBlock.java:73
(Diff revision 1)
> +        if (!doNotPrompt && !(context instanceof Activity)) {

Consider making the constructor protected and exposing to static factory methods, to get this check statically...
Attachment #8711042 - Flags: review?(nalexander) → review+
Attachment #8711043 - Flags: review?(nalexander) → review+
Comment on attachment 8711043 [details]
MozReview Request: Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r=nalexander

https://reviewboard.mozilla.org/r/31961/#review28725

Sure.

::: mobile/android/base/locales/en-US/android_strings.dtd:670
(Diff revision 1)
> +     if the app requires a runtime permission to downloads updates (Nightly/Aurora).-->

Can you clarify (Nightly/Aurora)?  Do you mean: "Currently, the updater only sees remotely advertised updates in the Nightly and Aurora channels."

Also, is opening files for writing that triggers the permission?  Because Beta and Release still have a "stubby" updater service that runs, it just never gets served updates.  I don't know if that service downloads and writes the (empty) XML that the Balrog update server serves...
Attachment #8711044 - Flags: review?(nalexander)
Comment on attachment 8711044 [details]
MozReview Request: Bug 1241887 - (Part 3) Show snackbar to ask for Storage permission if updater is enabled. r?

https://reviewboard.mozilla.org/r/31957/#review28729

I don't think this is right.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:761
(Diff revision 1)
> +        if (UpdateServiceHelper.isUpdaterEnabled()) {

It appears that this will show the snackbar every time Fennec runs, regardless of whether we have the permission already.  Explain why that's not the case.

That's likely an issue for Beta and Release, too, since for historical reasons the updater is on and enabled, but isn't served updates.
(Assignee)

Comment 9

2 years ago
(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8711044 [details]
> MozReview Request: Bug 1241887 - (Part 3) Show snackbar to ask for Storage
> permission if updater is enabled. r?
> 
> https://reviewboard.mozilla.org/r/31957/#review28729
> 
> I don't think this is right.

Oh, yeah, you are right. Something went wrong here.. I'll break the snackbar part off and move it to a separate bug to get this here out (This is rather annoying for fresh installs of Nightly - without permissions).
(Assignee)

Updated

2 years ago
Depends on: 1241999
(Assignee)

Comment 10

2 years ago
(In reply to Nick Alexander :nalexander from comment #6)
> Consider making the constructor protected and exposing to static factory
> methods, to get this check statically...

I can't fully follow here: Do you mean like having two entry points? One for Activity and one for Context? Doesn't this mean I need to add another flag (boolean?) to remember what Context I have here. And what about random places in the app where I have just a Context. I would need to explicitly cast it to Activity first so that the right factory method is used?

As you said you can live with the current version I'm going to land this part, to unblock our Nightly (and Monday Aurora) users. But I'm still interested in improving the API later. :)
(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> (In reply to Nick Alexander :nalexander from comment #6)
> > Consider making the constructor protected and exposing to static factory
> > methods, to get this check statically...
> 
> I can't fully follow here: Do you mean like having two entry points? One for
> Activity and one for Context? Doesn't this mean I need to add another flag
> (boolean?) to remember what Context I have here. And what about random
> places in the app where I have just a Context. I would need to explicitly
> cast it to Activity first so that the right factory method is used?

Yeah, two static entry points, perhaps like:

Permissions.waitForGrant(Activity)...
Permissions.doNotWaitForGrant(Context)...

You might need an internal flag, or two slightly different implementing classes, yes.  My concern is that you're enforcing at run time a thing that can be enforced at compile time.

> As you said you can live with the current version I'm going to land this
> part, to unblock our Nightly (and Monday Aurora) users. But I'm still
> interested in improving the API later. :)

Roger this.
(Assignee)

Comment 12

2 years ago
:nalexander from comment #7)
> Because Beta and Release still have a "stubby" updater service that runs, it just never gets served updates.(In reply to Nick Alexander 

Oh, then I'll move the actual permission check later in the code - after we actually found an update online and before we start to download it. This way Release and Beta can still look for updates but do not get the notification unless we start serving updates for them.
(Assignee)

Comment 13

2 years ago
Comment on attachment 8711042 [details]
MozReview Request: Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31959/diff/1-2/
Attachment #8711042 - Attachment description: MozReview Request: Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r? → MozReview Request: Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r=nalexander
(Assignee)

Comment 14

2 years ago
Comment on attachment 8711043 [details]
MozReview Request: Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31961/diff/1-2/
Attachment #8711043 - Attachment description: MozReview Request: Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r? → MozReview Request: Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r=nalexander
(Assignee)

Updated

2 years ago
Attachment #8711044 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/61b658cb7701178abb7c4b201eacaac4f0ae6978
Bug 1241887 - (Part 1) Allow to use Permission class with every Context object. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/a5f1af611fd6e5a7691041106736305e5d6e9c11
Bug 1241887 - (Part 2) UpdateService: Show notification if permission is missing. r=nalexander

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61b658cb7701
https://hg.mozilla.org/mozilla-central/rev/a5f1af611fd6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 17

2 years ago
Verified as fixed on Nightly 47.0a1 (2016-01-27), updating to Nightly 47.0a1 (2016-01-28)
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
You need to log in before you can comment on or make changes to this bug.