1240703 - ScreenshotObserver requires READ_EXTERNAL_STORAGE permission

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

> GeckoScreenshotObserver  E  Failure to process media change:
>                          E  java.lang.SecurityException: Permission Denial: reading com.android.providers.media.MediaProvider uri content://media/externa
>                             l/images/media from pid=6382, uid=10127 requires android.permission.READ_EXTERNAL_STORAGE, or grantUriPermission()
>                          E      at android.os.Parcel.readException(Parcel.java:1620)
>                          E      at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:183)
>                          E      at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:135)
>                          E      at android.content.ContentProviderProxy.query(ContentProviderNative.java:421)
>                          E      at android.content.ContentResolver.query(ContentResolver.java:491)
>                          E      at android.content.ContentResolver.query(ContentResolver.java:434)
>                          E      at org.mozilla.gecko.util.ScreenshotObserver$1.run(ScreenshotObserver.java:88)
>                          E      at android.os.Handler.handleCallback(Handler.java:739)
>                          E      at android.os.Handler.dispatchMessage(Handler.java:95)
>                          E      at android.os.Looper.loop(Looper.java:148)
>                          E      at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

We do not want to prompt the user on app start, just to enable the ScreenshotObserver (Currently only used for sending telemetry). I see two options here:

* Maybe ask somewhere unobtrusively inside the app. For example show a snackbar like "Hey, we need this permission for blabla | Accept" and then prompt.

* Or simpler: We will get this permission whenever the user starts a download (and then of course accepts the permission). So maybe we just enable the ScreenshotObserver only if we already have the permission from somewhere else. This of course leads to lower numbers in telemetry (But prompting probably too).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> We do not want to prompt the user on app start, just to enable the
> ScreenshotObserver (Currently only used for sending telemetry). I see two
> options here:
> 
> * Maybe ask somewhere unobtrusively inside the app. For example show a
> snackbar like "Hey, we need this permission for blabla | Accept" and then
> prompt.
> 
> * Or simpler: We will get this permission whenever the user starts a
> download (and then of course accepts the permission). So maybe we just
> enable the ScreenshotObserver only if we already have the permission from
> somewhere else. This of course leads to lower numbers in telemetry (But
> prompting probably too).

The simple approach is better for now. Waiting for a download is not good in the long term. We have plans to use this code to add weight to URLs that were "screenshotted", so we'll need a better approach soon.

The drop in telemetry data isn't great, but this should be small for now, since this is Android 6 only.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1240703 - Only start ScreenshotObserver if we have the needed permissions. r?nalexander

Review commit: https://reviewboard.mozilla.org/r/31789/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31789/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ac33da1f4a

Comment 5

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8710480 [details]
MozReview Request: Bug 1240703 - Only start ScreenshotObserver if we have the needed permissions. r?nalexander

https://reviewboard.mozilla.org/r/31789/#review28721

I leave it to you to address, decline to address, and potentially request re-review as you see fit.

::: mobile/android/base/java/org/mozilla/gecko/util/ScreenshotObserver.java:72
(Diff revision 1)
> +        if (mediaObserver == null) {

It's odd that the observer itself has not reference to the permissions it requires.  Could you make `start()` do the permission check?

If so, this would have the cost of binding the calling activity to the permissions block.  In the case when you have `doNotPrompt`, can we weaken to requiring a `Context` that is not an `Activity`?

If you feel that the permission check should not be local to the observer, please comment here that it's possible for `start()` to not run; and also comment around the `mediaObserver` member declaration, to save a NPE-chasing headache.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, now that Permissions/PermissionBlock allows abitrary Context objects I'll move this to ScreenshotObserver.start().

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, the Activity parameter wasn't the only problem here: ScreenshotObserver is in the org.mozilla.gecko.util.* package that is compiled without knowledge of all the other packages (here: org.mozilla.gecko.permissions.*).

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/373447b0e1ff2d49f47528dc0399433bf49311e1
Bug 1240703 - Pre: Move ScreenshotObserver to base package. r=me

https://hg.mozilla.org/integration/fx-team/rev/0be45a5671ba5b5513c2e0da21185b5ba2531011
Bug 1240703 - Only start ScreenshotObserver if we have the needed permissions. r=nalexander

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

I decided to move the ScreenshotObserver to the base package for now. But I'd like to post a follow-up (or a mailing list post?) to discuss if we want to keep the utility package separate and mirror the separation in the gradle configuration or get rid of it. That's not the first time this has caused (minor) problems.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

NI self: This might land in Fx47. Consider uplifting.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/373447b0e1ff
https://hg.mozilla.org/mozilla-central/rev/0be45a5671ba

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8710480 [details]
MozReview Request: Bug 1240703 - Only start ScreenshotObserver if we have the needed permissions. r?nalexander

Approval Request Comment

[Feature/regressing bug #]: This is a follow-up from bug 1212830 - Support for Android 6.0 runtime permissions.

[User impact if declined]: We are already catching this error; the user should not notice any problems. However we are logging this as an error. I'd just like to avoid false flag errors. But it wouldn't be problematic to not uplift this.

[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ac33da1f4a

[Risks and why]: Low - Checking permissions instead of letting this bubble up as an error.

[String/UUID change made/needed]: -

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8710480 [details]
MozReview Request: Bug 1240703 - Only start ScreenshotObserver if we have the needed permissions. r?nalexander

Avoids unnecessary error logging, please uplift to aurora

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 46 since this was (small, not user-facing) regression in 46.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc96d328acca
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e005d9f35ea