1241887 - Nightly/Aurora updater requires Storage permission

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We can't update Nightly/Aurora builds until we have the Storage permission.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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).

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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/

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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/

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Those three patches are built on top of "part 1" from bug 1240711 (doNotPrompt()).

Comment 6

[Nick Alexander :nalexander [he/him]]

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...

Comment 7

[Nick Alexander :nalexander [he/him]]

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...

Comment 8

[Nick Alexander :nalexander [he/him]]

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.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(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).

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(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. :)

Comment 11

[Nick Alexander :nalexander [he/him]]

(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.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

: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.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

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/

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

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/

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/61b658cb7701
https://hg.mozilla.org/mozilla-central/rev/a5f1af611fd6

Comment 17

[u421692]

Verified as fixed on Nightly 47.0a1 (2016-01-27), updating to Nightly 47.0a1 (2016-01-28)