1219016 - Create service for running Gecko

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Create an Android service that's capable of running Gecko in headless mode.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't use GeckoThread state to detect GeckoApp relaunching (v1)

Right now we use the GeckoThread state to detect whether GeckoApp is
first launching or is being restored after being destroyed. However,
because of headless mode, GeckoThread could already be running on
GeckoApp's first launch, so we need a separate way to detect
relaunching.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Store application context in GeckoAppShell (v1)

This patch adds separate setter and getter for the application context
in GeckoAppShell. The existing getContext method is misused for both
application and activity contexts, so new methods are added to improve
consistency.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use application context where appropriate (v1)

This patch makes code use the application context from GeckoAppShell
instead of the activity context. Enough changes are made to let headless
mode work.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't use context class name to detect webapp process (V1)

The code in WebappsUpdateTimer.js tried to detect a webapp process by
using the context class name, which is extremely fragile. This patch
makes it detect the current Gecko profile instead.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add GeckoService for headless mode (v1)

This patch adds a Service that is able to launch Gecko in a headless state. To
test it in a local build, add "android:exported=true" to GeckoService in
AndroidManifest.xml.in, make sure the Fennec process is not running, and run
something like

> adb shell am startservice -n org.mozilla.fennec_$USER/org.mozilla.gecko.GeckoService

You should then see Gecko output in the logcat.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8679705 [details] [diff] [review]
Use application context where appropriate (v1)

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

Everything about this patch is 'wrong', in that you shouldn't need to do this, and you shouldn't use the application context in any case you can avoid it.

Some old Stack Overflow wisdom is in the top-voted answer here:

http://stackoverflow.com/questions/7298731/when-to-call-activity-context-or-application-context

This is the expedient solution, but we put a bunch of effort into eliminating uses of GeckoAppShell's context code in general, and IMO this is a step in the wrong direction.

I'd also like Sebastian to take a look at the RestrictedProfiles change.

::: mobile/android/base/GeckoAppShell.java
@@ +1407,5 @@
>      }
>  
>      @WrapForJNI
>      public static float getDensity() {
> +        return getApplicationContext().getResources().getDisplayMetrics().density;

I have a sneaking horrible suspicion that we want an activity context in some of these situations…

@@ +1463,5 @@
>      }
>  
>      private static Vibrator vibrator() {
>          LayerView layerView = getLayerView();
> +        return (Vibrator) getApplicationContext().getSystemService(Context.VIBRATOR_SERVICE);

This makes `layerView` redundant, no?

@@ +2629,5 @@
>  
>      // Don't fail silently, tell the user that we weren't able to share the image
>      private static final void showImageShareFailureToast() {
> +        Toast toast = Toast.makeText(
> +                getApplicationContext(),

Toasts are supposed to use an activity context.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Richard Newman [:rnewman] from comment #6)
> Comment on attachment 8679705 [details] [diff] [review]
> Use application context where appropriate (v1)
> 
> Review of attachment 8679705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Everything about this patch is 'wrong', in that you shouldn't need to do
> this, and you shouldn't use the application context in any case you can
> avoid it.
>
> Some old Stack Overflow wisdom is in the top-voted answer here:
> 
> http://stackoverflow.com/questions/7298731/when-to-call-activity-context-or-
> application-context
> 
> This is the expedient solution, but we put a bunch of effort into
> eliminating uses of GeckoAppShell's context code in general, and IMO this is
> a step in the wrong direction.
> 
> I'd also like Sebastian to take a look at the RestrictedProfiles change.

For front-end code, I agree that there is almost always a more appropriate Context you can use, as explained by that StackOverflow post.

However, for back-end code, the situation is completely different. Gecko has the same lifetime as the process itself, and for Gecko background tasks that require a Context (accessing the alarm manager, for example), it is almost certainly more appropriate to use the application context rather than another context. In fact, there may not be another context when Fennec is not in the foreground.

That StackOverflow answer seems to make the argument that people shouldn't use application context because a lot of beginners don't know how to use it. That would be a pretty weak argument IMO.

> ::: mobile/android/base/GeckoAppShell.java
> @@ +1407,5 @@
> >      }
> >  
> >      @WrapForJNI
> >      public static float getDensity() {
> > +        return getApplicationContext().getResources().getDisplayMetrics().density;
> 
> I have a sneaking horrible suspicion that we want an activity context in
> some of these situations…

I did go through each change to make sure it made sense to be using an application context (for example, getting a system service). I could have overlooked some though.

> @@ +1463,5 @@
> >      }
> >  
> >      private static Vibrator vibrator() {
> >          LayerView layerView = getLayerView();
> > +        return (Vibrator) getApplicationContext().getSystemService(Context.VIBRATOR_SERVICE);
> 
> This makes `layerView` redundant, no?

Good catch.

> @@ +2629,5 @@
> >  
> >      // Don't fail silently, tell the user that we weren't able to share the image
> >      private static final void showImageShareFailureToast() {
> > +        Toast toast = Toast.makeText(
> > +                getApplicationContext(),
> 
> Toasts are supposed to use an activity context.

Toasts can use either application or activity contexts, as explained by the second answer in your StackOverflow question.

Comment 8

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

Comment on attachment 8679706 [details] [diff] [review]
Don't use context class name to detect webapp process (V1)

Good change

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8679705 [details] [diff] [review]
Use application context where appropriate (v1)

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

r+ for the restricted profiles related changes. We are using the Context for two things: Accessing Android's UserManager and calling GeckoProfile.get(). For the UserManager the context does not matter and for GeckoProfile.get().. it seems like you changed other calls to that in this patch as well - so I guess that's okay.

::: mobile/android/base/GeckoAppShell.java
@@ +1650,5 @@
>  
>          int[] result = new int[attrsAppearance.length];
>  
>          final ContextThemeWrapper contextThemeWrapper =
> +            new ContextThemeWrapper(getApplicationContext(), android.R.style.TextAppearance);

This /could/ yield different results depending on what theme is assigned to the application (context) or activity (context) in this situation.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8679704 [details] [diff] [review]
Store application context in GeckoAppShell (v1)

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

We should just call getApplicationContext() on whatever Context we have (service, activity, etc).

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> Comment on attachment 8679704 [details] [diff] [review]
> Store application context in GeckoAppShell (v1)
> 
> Review of attachment 8679704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should just call getApplicationContext() on whatever Context we have
> (service, activity, etc).

We don't have a Context when it's a JNI call from Gecko, and if we're storing a Context for Gecko to use, we should store the application context instead of a service/activity context, which has shorter lifetime than Gecko.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8679704 [details] [diff] [review]
Store application context in GeckoAppShell (v1)

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

Fair enough I guess

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

rnewman: thoughts on comment 7?

Comment 14

[Richard Newman [:rnewman]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)
> rnewman: thoughts on comment 7?

303 mcomella :D

Comment 15

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

I'll get back to this tomorrow but worth noting that I have no previous knowledge and will be largely looking this up. Sebastian, if you know anything off-hand about application context vs. activity context and how that might apply to a long-lived headless Gecko Service, feel free to chime in. Otherwise, feel free to clear it immediately – we both don't need to spend time on this. :)

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #15)
> Sebastian, if you know anything off-hand about application context vs. activity context and how
> that might apply to a long-lived headless Gecko Service, feel free to chime
> in.

My own guidelines are - in a nutshell:

* Everything that needs to handle UI, themes, layouts, dialogs, .. (everything that happens inside an activity) most likely needs or requires an activity context. Some of those things you just can't create/call with an application context (e.g. dialog) but some of them work fine but might have undesirable results (e.g. LayoutInflater with application context). Toasts are an exception from that.

* For system features it mostly does not matter: Sending broadcasts, getting system services (with at least the exception of LAYOUT_INFLATER_SERVICE), ...

* The application context (Actually the Application object) lives as long as the process lives. So you can keep (static) references to it. You don't want to do that with an Activity (context) because this means you are keeping the activity and all resources in memory even if this (instance of) activity is not visible anymore.


It heavily depends on the calls made inside GeckoAppShell. I guess in a perfect world GeckoView would bubble up calls to its activity if needed and background code uses the application context (or the service context) because it does not touch UI code.

Briefly looking at the patch:

* Calls to getSystemService() for getting LocationManager, AlarmManager, SensorManager, ActivityManager should yield the same result with both contexts.

* For the calls to get the density, or use the density (via WindowManager, getResources(), bitmap decoding): I'm not 100% sure if there could be multiple windows with different densities and we actually want to get the values of the window of the current activity.

* For Intents: For broadcasts it does not matter. For activities: They can only be started in a new task from the application context.

I think the patch is mostly safe (I only did a quick scan) but let's discuss actual lines of code that might or might not be problematic.

Comment 17

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

Sorry for the delay. I don't think I can provide much more over Sebastian's answer, but I did find this old mobile-firefox-dev post [1] on the issue with a linked article [2], that has a similar message to Sebastian.

[1]: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-February/000518.html
[2]: https://possiblemobile.com/2013/06/context/

Comment 18

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

OK, let's get this landed and watch for context-related bustage. We are still early in the cycle.

Jim - Can you do a simple post to mobile-firefox-dev to give a heads up?

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f2bffacca04c
https://hg.mozilla.org/integration/fx-team/rev/9dbbf81894ca
https://hg.mozilla.org/integration/fx-team/rev/d42f4c84b136
https://hg.mozilla.org/integration/fx-team/rev/8ea35a788f3a
https://hg.mozilla.org/integration/fx-team/rev/da972c7c5cf4

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2bffacca04c
https://hg.mozilla.org/mozilla-central/rev/9dbbf81894ca
https://hg.mozilla.org/mozilla-central/rev/d42f4c84b136
https://hg.mozilla.org/mozilla-central/rev/8ea35a788f3a
https://hg.mozilla.org/mozilla-central/rev/da972c7c5cf4