Closed Bug 1241887 Opened 4 years ago Closed 4 years ago

Nightly/Aurora updater requires Storage permission

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox46 --- verified

People

(Reporter: sebastian, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We can't update Nightly/Aurora builds until we have the Storage permission.
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).
Attachment #8711042 - Flags: review?(nalexander)
Attachment #8711043 - Flags: review?(nalexander)
Attachment #8711044 - Flags: review?(nalexander)
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.
(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).
Depends on: 1241999
(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.
: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 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
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
Attachment #8711044 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/61b658cb7701
https://hg.mozilla.org/mozilla-central/rev/a5f1af611fd6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Verified as fixed on Nightly 47.0a1 (2016-01-27), updating to Nightly 47.0a1 (2016-01-28)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.