Closed Bug 960709 Opened 6 years ago Closed 6 years ago

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

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
fennec - ---

People

(Reporter: aaronmt, Assigned: mcomella)

References

Details

(Keywords: reproducible)

Attachments

(5 files, 3 obsolete files)

18.05 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
3.69 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
14.23 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
6.14 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
1.75 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
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
Can we move the BrowserHealthRecorder to BrowserApp.java?
Oh, ignore the fail to run bit - that looks like bug 959244.
(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
Flags: needinfo?(mark.finkle)
tracking-fennec: ? → -
This was the "meh, we'll fix it later" about putting FHR into GeckoApp not BrowserApp.
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.
(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
Flags: needinfo?(mark.finkle)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Creates the interface and factors out the appropriate components. MVP, so it's missing some methods which should be in the interface for correctness.
Attachment #8369772 - Flags: review?(rnewman)
Applies part 1 to GeckoApp and BrowserApp. Part 3 will add the missing methods to the interface for correctness.
Attachment #8369774 - Flags: review?(rnewman)
Speaking of correctness, I added the @Override annotation to BrowserApp.createHealthRecorder.
Attachment #8369806 - Flags: review?(rnewman)
Attachment #8369774 - Attachment is obsolete: true
Attachment #8369774 - Flags: review?(rnewman)
Comment on attachment 8369772 [details] [diff] [review]
Part 1: Factor out HealthRecorder interface.

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

I love it.
Attachment #8369772 - Flags: review?(rnewman) → review+
Attachment #8369806 - Flags: review?(rnewman) → review+
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…
Attachment #8369794 - Flags: review?(rnewman) → review+
Hardware: ARM → All
Attachment #8369806 - Attachment is obsolete: true
(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).
Attachment #8370510 - Flags: review?(rnewman)
Comment on attachment 8370503 [details] [diff] [review]
Part 2: Use HealthRecorder interface in GeckoApp.

Move r+.
Attachment #8370503 - Flags: review+
Attachment #8370510 - Flags: review?(rnewman) → review+
Fixes a bug where the locale would not change if HealthRecorder was not yet initialized, as mentioned on IRC.
Attachment #8370521 - Flags: review?(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 ;)
Attachment #8370521 - Flags: review?(rnewman) → review+
Attachment #8370521 - Attachment is obsolete: true
Comment on attachment 8370538 [details] [diff] [review]
Part 5: Allow locale change even if HealthRecorder is not yet initialized.

Move r+
Attachment #8370538 - Flags: review+
You need to log in before you can comment on or make changes to this bug.