960709 - Synthesized APK launch attempts to invoke the health provider; permission denial, app fails to launch - java.lang.SecurityException

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Aaron Train [:aaronmt]]

D/GeckoProfile( 2965): Created new profile dir.
I/GeckoApp( 2965): Creating BrowserHealthRecorder.
D/GeckoApp( 2965): OS locale is en_US, app locale is null
D/GeckoHealthRec( 2965): Initializing. Dispatcher is org.mozilla.gecko.util.EventDispatcher@40580398
W/dalvikvm( 2965): threadid=9: thread exiting with uncaught exception (group=0x40015560)
W/ActivityManager(   98): Permission Denial: opening provider org.mozilla.gecko.background.healthreport.HealthReportProvider from ProcessRecord{40893b98 2965:org.mozilla.fennec_myk:org.mozilla.fennec_myk.WebApp0/10031} (pid=2965, uid=10031) requires org.mozilla.fennec_myk.permissions.HEALTH_PROVIDER or org.mozilla.fennec_myk.permissions.HEALTH_PROVIDER
E/GeckoAppShell( 2965): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 10 ("GeckoBackgroundThread")
E/GeckoAppShell( 2965): java.lang.SecurityException: Permission Denial: opening provider org.mozilla.gecko.background.healthreport.HealthReportProvider from ProcessRecord{40893b98 2965:org.mozilla.fennec_myk:org.mozilla.fennec_myk.WebApp0/10031} (pid=2965, uid=10031) requires org.mozilla.fennec_myk.permissions.HEALTH_PROVIDER or org.mozilla.fennec_myk.permissions.HEALTH_PROVIDER
E/GeckoAppShell( 2965): 	at android.os.Parcel.readException(Parcel.java:1322)
E/GeckoAppShell( 2965): 	at android.os.Parcel.readException(Parcel.java:1276)
E/GeckoAppShell( 2965): 	at android.app.ActivityManagerProxy.getContentProvider(ActivityManagerNative.java:1882)
E/GeckoAppShell( 2965): 	at android.app.ActivityThread.getProvider(ActivityThread.java:3347)
E/GeckoAppShell( 2965): 	at android.app.ActivityThread.acquireProvider(ActivityThread.java:3372)
E/GeckoAppShell( 2965): 	at android.app.ContextImpl$ApplicationContentResolver.acquireProvider(ContextImpl.java:1666)
E/GeckoAppShell( 2965): 	at android.content.ContentResolver.acquireProvider(ContentResolver.java:779)
E/GeckoAppShell( 2965): 	at android.content.ContentResolver.acquireContentProviderClient(ContentResolver.java:814)
E/GeckoAppShell( 2965): 	at org.mozilla.gecko.background.healthreport.EnvironmentBuilder.getContentProviderClient(EnvironmentBuilder.java:28)
E/GeckoAppShell( 2965): 	at org.mozilla.gecko.health.BrowserHealthRecorder.<init>(BrowserHealthRecorder.java:259)
E/GeckoAppShell( 2965): 	at org.mozilla.gecko.GeckoApp$16.run(GeckoApp.java:1330)
E/GeckoAppShell( 2965): 	at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoAppShell( 2965): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoAppShell( 2965): 	at android.os.Looper.loop(Looper.java:130)
E/GeckoAppShell( 2965): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Steps to reproduce

i) Install http://dapk.net/application.apk?manifestUrl=https://marketplace.firefox.com/app/0021e1e5-2ca0-43e4-b66f-a52cfbf4ffeb/manifest.webapp

ii) Launch application

Comment 1

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

Can we move the BrowserHealthRecorder to BrowserApp.java?

Comment 2

[Aaron Train [:aaronmt]]

Oh, ignore the fail to run bit - that looks like bug 959244.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Can we move the BrowserHealthRecorder to BrowserApp.java?

Seems doable, though we'd need a listener to update BrowserHealthRecorder's locale data in GeckoApp.setLocale (or else risk inconsistent FHR results by handling the message once in BrowserApp and once in GeckoApp) [1].

Additionally, BHR needs to access to mJavaUiStartupTimer and mGeckoReadyStartupTimer, which should also seems to need a listener to maintain consistency [2] [3].

I'm assuming this is what we want to do?

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?rev=124f393d9f2e#2779
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1615
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#575

Comment 4

[Richard Newman [:rnewman]]

This was the "meh, we'll fix it later" about putting FHR into GeckoApp not BrowserApp.

Comment 5

[Richard Newman [:rnewman]]

Probably the simplest thing to do here is to start BHR "paused", and only hit "play" in BrowserApp (and, indeed, hit "stop" in webapps). As Michael mentioned, FHR has/needs a lot of hooks that would violate some of the encapsulation of GeckoApp.

Comment 6

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

(In reply to Richard Newman [:rnewman] from comment #5)
> Probably the simplest thing to do here is to start BHR "paused", and only
> hit "play" in BrowserApp (and, indeed, hit "stop" in webapps). As Michael
> mentioned, FHR has/needs a lot of hooks that would violate some of the
> encapsulation of GeckoApp.

This seems reasonable for the short term

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Factor out HealthRecorder interface.

Creates the interface and factors out the appropriate components. MVP, so it's missing some methods which should be in the interface for correctness.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Use HealthRecorder interface in GeckoApp.

Applies part 1 to GeckoApp and BrowserApp. Part 3 will add the missing methods to the interface for correctness.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Add methods to HealthRecorder interface for correctness.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Use HealthRecorder interface in GeckoApp.

Speaking of correctness, I added the @Override annotation to BrowserApp.createHealthRecorder.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8369772 [details] [diff] [review]
Part 1: Factor out HealthRecorder interface.

Review of attachment 8369772 [details] [diff] [review]:
-----------------------------------------------------------------

I love it.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8369794 [details] [diff] [review]
Part 3: Add methods to HealthRecorder interface for correctness.

Review of attachment 8369794 [details] [diff] [review]:
-----------------------------------------------------------------

1. Test this, in case we're both mistaken about Java method dispatch :D
2. Add an `isEnabled` method that callers can use to check whether they should bother building expensive argument values?

::: mobile/android/base/health/StubbedHealthRecorder.java
@@ +13,5 @@
>   * StubbedHealthRecorder is an implementation of HealthRecorder that does (you guessed it!)
>   * nothing.
>   */
>  public class StubbedHealthRecorder implements HealthRecorder {
> +    public SessionInformation getCurrentSession() { return null; }

Turns out this is never called anyway…

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Use HealthRecorder interface in GeckoApp.

Removed getCurrentSession.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Use HealthRecorder.isEnabled to make StubbedHealthRecorder use more efficient.

(In reply to Richard Newman [:rnewman] from comment #12)
> 1. Test this, in case we're both mistaken about Java method dispatch :D

Already done by adding Log statements to the `createHealthRecorder` methods
specifying which class it's called from. I ran my browser, ran the SoundCloud
app (downloaded via the marketplace) and it worked correctly. However,
SoundCloud would open the application chooser and run on a browser already
installed on the device, rather than by it's own (I'm guessing the APK wasn't
synthesized?) so I could never repro this bug, but from the debug output,
it should be fixed.

> 2. Add an `isEnabled` method that callers can use to check whether they
> should bother building expensive argument values?

This patch. Unfortunately, the only expensive operation we could avoid was
starting a runnable - everything else seems to be necessary
(`SessionInformation.wasStopped` and the associated pref is used in
`wasKilled()` by Telemetry).

> > ::: mobile/android/base/health/StubbedHealthRecorder.java > @@ +13,5 @@
> >   * StubbedHealthRecorder is an implementation of HealthRecorder that does (you guessed it!)
> >   * nothing.
> >   */
> >  public class StubbedHealthRecorder implements HealthRecorder {
> > +    public SessionInformation getCurrentSession() { return null; }
> > Turns out this is never called anyway…

Removed (comment 13).

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8370503 [details] [diff] [review]
Part 2: Use HealthRecorder interface in GeckoApp.

Move r+.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Allow locale change even if HealthRecorder is not yet initialized.

Fixes a bug where the locale would not change if HealthRecorder was not yet initialized, as mentioned on IRC.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8370521 [details] [diff] [review]
Part 5: Allow locale change even if HealthRecorder is not yet initialized.

Review of attachment 8370521 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with compilation error fixed and comment added.

::: mobile/android/base/GeckoApp.java
@@ +2785,5 @@
>              return;
>          }
>  
>          final HealthRecorder rec = mHealthRecorder;
> +        if (rec != null) {

Worth commenting here that if the health recorder isn't initialized yet (which it should be), the locale change won't trigger a session transition, and subsequent events will be recorded in an environment with the wrong locale.

@@ +2792,3 @@
>          }
>  
>          final boolean startNewSession = true;

Betcha this patch doesn't build ;)

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Allow locale change even if HealthRecorder is not yet initialized.

How embarrassing. x_x

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8370538 [details] [diff] [review]
Part 5: Allow locale change even if HealthRecorder is not yet initialized.

Move r+

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/e4a442b68a43
remote:   https://hg.mozilla.org/integration/fx-team/rev/52ed9fc26da5
remote:   https://hg.mozilla.org/integration/fx-team/rev/236794ee5dd4
remote:   https://hg.mozilla.org/integration/fx-team/rev/229735e9e5ab
remote:   https://hg.mozilla.org/integration/fx-team/rev/50c164585e25

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4a442b68a43
https://hg.mozilla.org/mozilla-central/rev/52ed9fc26da5
https://hg.mozilla.org/mozilla-central/rev/236794ee5dd4
https://hg.mozilla.org/mozilla-central/rev/229735e9e5ab
https://hg.mozilla.org/mozilla-central/rev/50c164585e25