1024708 - [geolocation-stumbler] Integrate MozStumbler service into Fennec build system

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[:garvan]

Attachment: StumblerAndroidManifest_services.xml.in

Integrate code from https://github.com/mozilla/MozStumbler into Fennec. The code is divided into client and service. The source dir to integrate is src/org/mozilla/mozstumbler/service. 
Plan is to copy the required pieces from github to land in m-c source tree.

Where to place the *.java files? And add list of *.java files to mozbuild, which one?

Manifest integration: mobile/android/base/AndroidManifest.in
--> #include ../services/manifests/StumblerAndroidManifest_services.xml.in

Where to put XML files (mobile/android/base/resources/xml ?): MozStumbler has a prefs xml, authenticator and sync xml. The prefs xml could be coded around, I don't see as necessary. I'd rather not refactor that ATM if I don't have to.

Comment 1

[:garvan]

Nick you mentioned this is similar to a recent search project that was integrated in Fennec. Could you point me in the direction I should go? Thanks.

Comment 2

[Nick Alexander :nalexander [he/him]]

The ticket tracking integrating the Search Activity into Fennec is Bug 1021864.  My intention is to work out the kinks in the process on that work, and then do the same thing here.  What's the time frame for this work landing in tree?

Comment 3

[:garvan]

I set the milestone on the bug. 
I need to be working simultaneously on MozStumbler and Fennec, but just for myself for development over the next 2 weeks. I plan to be landing something 1st wk of July in m-c.  

I did try to do a personal-use integration, adding my code under thirdparty/org/mozilla
I ran into what I assume are classpath problems based on the errors I got. 
If you could help me just mash something in for personal use for now, that would be fine.

Comment 4

[:garvan]

Attachment: android-services.mozbuild

This mozbuild is for personal-use, just an attempt by me to quickly add in my code for experimentation. The lines starting with 'org/mozilla/mozstumbler' are what I added.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Garvan Keeley [:garvank] from comment #3)
> I set the milestone on the bug. 
> I need to be working simultaneously on MozStumbler and Fennec, but just for
> myself for development over the next 2 weeks. I plan to be landing something
> 1st wk of July in m-c.  

Okay, this is reasonable.  That's giving 3-4 weeks for getting something in place; we can make that happne.

> I did try to do a personal-use integration, adding my code under
> thirdparty/org/mozilla
> I ran into what I assume are classpath problems based on the errors I got. 
> If you could help me just mash something in for personal use for now, that
> would be fine.

I should be able to do this.  There's light magic to be done here, which might require upstream changes (but not too significant).  Some questions first: what's the existing Java root package?  The existing Android package name?  Does the package have any dependencies?  Does it have any preprocessed source code or resources?

Comment 6

[Aaron Train [:aaronmt]]

I'd rather us use the tracking flag instead of specifying a target milestone for nothing that has landed.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Garvan Keeley [:garvank] from comment #4)
> Created attachment 8439480 [details]
> android-services.mozbuild
> 
> This mozbuild is for personal-use, just an attempt by me to quickly add in
> my code for experimentation. The lines starting with
> 'org/mozilla/mozstumbler' are what I added.

OK, reading between the lines a little, I dug into this.  This is close to being simple, but:

1) this is using BuildConfig.  Totally reasonable, but not how m-c does anything.  A little light pre-processing is the way m-c does this kind of configuration.  See, for example, AppConstants.java.in.

2) the service requires resources, but only in 2 places -- one, to get the app name; two, to set default preferences.  If we can remove all resource references, this gets a good deal simpler: then it's just building a jar and putting some manifest entries into Fennec.  (We can support resources, it's just trickier.)  For the app name, we can get it using code (I bet), using the preprocessor, or hard-coding.  For the default preference values, I'm not sure what you want to do.  You and ibarlow will have to work out the preference/UI story together before we make a decision about this stuff.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: WIP on landing MozStumbler into mozilla-central.

garvank: this is WIP towards building the stumbler as
$OBJDIR/mozilla/android/stumbler/stumbler.jar.  It won't work due to
the issues I outlined above, but it's how I evaluated what our next
steps should be.

Comment 9

[:garvan]

(In reply to Nick Alexander :nalexander from comment #5)
> Some questions
> first: what's the existing Java root package?  

Can use either org.mozilla.mozstumbler or org.mozilla.mozstumbler.service as the root.
Only the service is needed, but the MozStumbler build system uses the former as a root.

> The existing Android package name?  

org.mozilla.mozstumbler

> Does the package have any dependencies?  

    dependencies {
        compile "org.osmdroid:osmdroid-android:4.1"
        compile 'org.slf4j:slf4j-android:1.7.7'
        compile "com.android.support:support-v4:19.0.+"
    }

> Does it have any preprocessed source code or resources?

3 xml files, prefs, sync+authentication. I haven't looked at pulling these out, but they seem like an annoyance, as without them, this could be a pure .jar lib.

Comment 10

[:garvan]

> 1) this is using BuildConfig.  Totally reasonable, but not how m-c does
> anything.  A little light pre-processing is the way m-c does this kind of
> configuration.  See, for example, AppConstants.java.in.

I think this could be pulled if it simplifies things. I'll look at that.

> 2) the service requires resources, but only in 2 places -- one, to get the
> app name; two, to set default preferences. 

The preferences don't need to be in an XML file. I can work around that.

The authentication.xml and sync.xml I would like to work around using, any tips on pulling those out is appreciated. All the sync docs say these are required. However, without them we could have a pure .jar lib, and less stumbler bits polluting the build system.

> For the app name, we can get it using code (I bet), using
> the preprocessor, or hard-coding. 

I am fine with hard-coding the app name, it won't be localized.

> For the default preference values, I'm
> not sure what you want to do.  You and ibarlow will have to work out the
> preference/UI story together before we make a decision about this stuff.

I'll look at the prefs system more closely, it is mainly there for MozStumbler fanciness. I don't think it is needed. The only 'prefs' the stumbler needs to know are queried from Fennec:
1) Is geolocation sharing on?
2) Can I upload over wifi?

Comment 11

[:garvan]

Updating status to reflect current state on github:

- no longer uses BuildConfig

- sync has been pulled from stumbler service (uses async task for upload). Scheduling of this task is primitive currently, may need more logic

- prefs system no longer uses xml

- removed R.* usage anywhere in the service.

Comment 12

[:garvan]

Build is working.

Currently looking into runtime error:
E/AndroidRuntime(21327): java.lang.RuntimeException: Unable to instantiate receiver org.mozilla.mozstumbler.service.PassiveServiceStarter: java.lang.ClassNotFoundException: Didn't find class "org.mozilla.mozstumbler.service.PassiveServiceStarter" on path: DexPathList[[zip file "/data/app/org.mozilla.fennec_gkeeley-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.mozilla.fennec_gkeeley-1, /vendor/lib, /system/lib]]

PassiveServiceStarter is in that package path:
$ head mobile/android/stumbler/src/service/PassiveServiceStarter.java 
package org.mozilla.mozstumbler.service;

StumblerManifest_services.xml.in has the receiver declared like so:
<receiver android:name="org.mozilla.mozstumbler.service.PassiveServiceStarter">

Comment 13

[:garvan]

Attachment: WIP_stumbler_integration.patch

Builds ok.
This is missing the proper integration step for stumbler/manifests/*.xml.in

Nick could I bug you for how to do that?

Comment 14

[:garvan]

Attachment: WIP_stumbler_integration.patch

Nick, hopefully this is useful

Comment 15

[Nick Alexander :nalexander [he/him]]

Preffed off:

https://tbpl.mozilla.org/?tree=Try&rev=bb1d88953e0a

Preffed on:

https://tbpl.mozilla.org/?tree=Try&rev=aed5bcde93e7

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Land mobile/android/stumbler Java code. r=nalexander

This is the Java code from the MozStumbler git repo.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add build feature flag MOZ_ANDROID_MLS_STUMBLER. r=rnewman

rnewman, you did the last one, so I'll use you again.  Feel free to
rubber-stamp or re-direct.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Build stumbler JAR and integrate into Fennec. r=rnewman

rnewman: ditto, but a slightly different integration, since the
stumbler doesn't (yet) depend on Fennec at all.

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Post: Don't build MLS Stumbler in Fennec by default. r=me

Comment 20

[Nick Alexander :nalexander [he/him]]

A note to myself about landing this stuff: the first patch should probably have garvan's name attached to it.  Although, it is a roll-up of a lot of work...

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Eclipse support. r=rnewman

rnewman: this seems to be hitting a plug-in issue for me.  I see "null
argument:" errors in my Eclipse build log, which I interpret to be
problems with the plug-in.  It has to do with referenced_projects
vs. included_projects, but it shouldn't block this little patch,
especially 'cuz we're build-time preffed off.  I'll dig into it.

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: Example commit showing how to start the Stumbler service based on an existing pref.

This is how I launched the passive stumbler.  I can't guarantee
anything is happening, but the service starts and I see log messages.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 8450497 [details] [diff] [review]
Part 3: Build stumbler JAR and integrate into Fennec. r=rnewman

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

::: mobile/android/stumbler/manifests/StumblerManifest_services.xml.in
@@ +2,5 @@
> +  android:name="org.mozilla.mozstumbler.service.StumblerService"
> +  android:label="stumbler">
> +</service>
> +
> +<receiver android:name="org.mozilla.mozstumbler.service.PassiveServiceStarter">

Unrelated: normally we call these classes "*Receiver", not "ServiceStarter".

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8450632 [details] [diff] [review]
Part 4: Eclipse support. r=rnewman

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

::: mobile/android/base/moz.build
@@ +640,5 @@
>      omnijar.add_classpathentry(d, TOPSRCDIR + '/mobile/android/' + d, dstdir=d)
>  
>  # The omnijar is included in the Fennec APK (although it's empty,
>  # having no resources, assets, or Java code).
> +main.included_projects += ['../' + omnijar.name]

That explains the red cross in my library settings…

@@ +652,5 @@
>      main.included_projects += ['../' + crashreporter.name]
> +
> +if CONFIG['MOZ_ANDROID_MLS_STUMBLER']:
> +    main.included_projects += ['../FennecStumbler']
> +    main.referenced_projects += ['../FennecStumbler']

Does this need to be both included and referenced?

Comment 25

[:garvan]

Comment on attachment 8450634 [details] [diff] [review]
Example commit showing how to start the Stumbler service based on an existing pref.

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

This is great thanks! I can either store the value of this pref on the stumbler side (preferred), or I can have the stumbler call out to Fennec to check this pref.

Comment 26

[:garvan]

The stumbler service is a single-threaded design, so android.os.StrictMode$StrictModeDiskReadViolation are a-plenty. And they should be, stumbler can't be performing disk and DB access on the main thread.

Solutions: 
1) run as a separate process (easiest, preferred by me, not popular with others)
2) refactor to put file I/O on secondary thread (time-consuming, regression risk)
3) get stumbler service on a secondary thread (need to research how to move a persistent service to a thread)

Comment 27

[Richard Newman [:rnewman]]

Take a look at how IntentService does things.

http://developer.android.com/reference/android/app/IntentService.html
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/app/IntentService.java

It automatically handles incoming intents on a worker thread.

Feel free to explore the process approach if you can quantify perf characteristics (e.g., memory usage), and don't have any need for process-sharing with Fennec (such as reading/writing the same SharedPreferences file -- you'll need to communicate through intents).

Comment 28

[:garvan]

I'll go with looking at the IntentService model, modified for a persistent service. 
I don't want to introduce any uncertainties/unknowns, I'll stay away from the secondary process approach.

Comment 29

[:garvan]

(In reply to Richard Newman [:rnewman] from comment #27)
> Take a look at how IntentService does things.
> 
The modified IntentService works great so far, this is the approach I am taking.

Comment 30

[:garvan]

(In reply to Nick Alexander :nalexander from comment #22)
> Created attachment 8450634 [details] [diff] [review]
> Example commit showing how to start the Stumbler service based on an
> existing pref.

I am storing the state of this pref in the service to know whether it is enabled or not. One more piece of info I am looking at sending along with the pref is the MLS api key.

Comment 31

[:garvan]

Attachment: Part-1-Land-stumbler-Java-code.patch

This code has another round of refactoring, most important is the service is now on a thread. The PersistentIntentService.java is the Android code for IntentService with one line changed. I kept the Android license intact, and added a note regarding my change. 

Full details on this refactor are here: https://github.com/mozilla/MozStumbler/pull/704

Comment 32

[:garvan]

Attachment: Part 3: Build stumbler JAR and integrate into Fennec.

updated list of stumbler build files for added file and changed name of PassiveServiceStarter to PassiveServiceReceiver

Comment 33

[:garvan]

Functionally, this is working well at this point.

Just realized I forgot to add code to pass the mozilla api key, easiest way is to do so along with the startup pref. The code will work without the key. I can do this before or after this code lands. 

The other important TODO is bug 1036514, getting an about:config pref to turn on/off logging. It is hard-coded to on for now.

Comment 34

[:garvan]

Attachment: Part 3: Build stumbler JAR and integrate into Fennec.

Rename *Starter to *Receiver

Comment 35

[:garvan]

Attachment: Bug-1024708---Example-commit-showing-how-to-start-.patch

Renamed *Starter to *Receiver. 

At this point I should do a try server run to make sure I am not missing anything else. I am PTO today, will do that Monday.

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8454506 [details] [diff] [review]
Part 3: Build stumbler JAR and integrate into Fennec.

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

This is fine, but an interdiff or follow-up diff to be squashed is easier to review and to build upon.

Comment 37

[Nick Alexander :nalexander [he/him]]

(In reply to Garvan Keeley [:garvank] from comment #35)
> Created attachment 8454511 [details] [diff] [review]
> Bug-1024708---Example-commit-showing-how-to-start-.patch
> 
> Renamed *Starter to *Receiver. 
> 
> At this point I should do a try server run to make sure I am not missing
> anything else. I am PTO today, will do that Monday.

Am I supposed to be reviewing this?  Who reviewed the code you already landed into MozStumbler?  'cuz there are a few things in your PR that look odd.

Comment 38

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8454511 [details] [diff] [review]
Bug-1024708---Example-commit-showing-how-to-start-.patch

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

This is fine, but not yet finished.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +850,5 @@
> +     * <code>PREFS_STUMBLER_ENABLED</code> pref.
> +     */
> +    public static void broadcastStumblerPref(final Context context, final boolean value) {
> +        broadcastPrefAction(context,
> +                            AppConstants.ANDROID_PACKAGE_NAME + ".STUMBLER_PREF",

Factor this out into its own variable.

@@ +853,5 @@
> +        broadcastPrefAction(context,
> +                            AppConstants.ANDROID_PACKAGE_NAME + ".STUMBLER_PREF",
> +                            PREFS_GEO_REPORTING,
> +                            value);
> +    }

This will need a little tweaking for your additional data, right?  You want to fish the value of  MOZ_MOZILLA_API_KEY and add it to the intent, correct?

So export CONFIG['MOZ_MOZILLA_API_KEY'] to DEFINES in m/a/b/moz.build (lots of examples), and add it to AppConstants.java.in.

As for a debug flag, that should be a boolean (computed here), based on MOZILLA_OFFICIAL or similar.  (Also in AppConstants, IIRC).

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8454204 [details] [diff] [review]
Part-1-Land-stumbler-Java-code.patch

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

This is a rubber-stamp, but maybe it shouldn't be: who is reviewing the code you land in the MozStumbler repo?  I had been assuming that dought, cpeterson, or other members of that team.  If the answer is nobody, then we need to agree on a review strategy for this code bomb.

Comment 40

[:garvan]

There has been no code review on my landed code, so that will be needed. There are 3 automated functional tests that I added, that are designed to hit a large swath of code paths.  
During the refactor there have been 4 releases with the refactored code (0.20.6 to 0.20.9): https://github.com/mozilla/MozStumbler/releases

Important design notes for the reviewer:

PassiveServiceReceiver is the service starter (by boot intent, or by Fennec startup). This then installs a passive GPS listener. When any other app requests a GPS, we receive it and inspect it to ensure it is not assisted by GLS (it is 'pure' GPS). GPS requests must be a few minutes and 30 metres apart, or they are considered too frequent.

If GPS is accepted, then run a wifi and cell scan, and record the results as a single row in the db. 

If the power state of the device is low, the service goes idle, will ignore GPS messages.

Upload: see StumblerService.java, mPassiveUploadTimer.schedule(). This has a flag indicating if the db is empty (rather than touch the db). If the db is not empty, schedule a check every few mins to upload the stored data. Uploads 50 rows gzipped at a time, in a loop, until the db is empty (or error). Only upload if power state is good, wifi is on, and screen is on. The 'screen is on' check was there under the assumption that, on device idle, having the timer check every 3 minutes would have no cost (that all the power cost would be the upload operation). That assumption is wrong, even a 3 min timer doing nothing can keep the CPU awake, which is bad. 
- Data Volumes: with logging on you will see the current upload package size, and the total for the session. I am seeing 1 kb every few minutes while stumbling with google maps open and tracking. 
- Clearing old data: currently set to 2 weeks, could be shorter. Deletes *all* records if the oldest record is > 2 weeks old.

Nick, as mentioned, this upload process should be changed to a scheduled intent, so as not to require a timer.
Also, we should have a hard limit on the number of rows in the DB.

Some Known Issues:

- lack of junit tests.

- ContentResolverInterface.java: during the refactor, in order to not use android sync for upload, this class was inserted as a refactoring proxy, to keep the MozStumbler client functional during refactor, and for A/B testing of the refactor. The client and the service use different concrete implementations of this, this should change so that the client uses the same concrete implementation, the code is now stable. This is an issue, as the stumbler code gets QA'd by the community using MozStumbler.
-- To explain this code further: ServerContentResolver.java + DbCommunicator.java together are a drop-in replacement for Sync Service. The sync service remains in the MozStumbler client (for the reasons mentioned above).

- UploadReports.java: rather than using sql queries to get batches, it uses some weird getMax/Min id of data in the table. I don't understand this, I left it as-is.

- Network code is repeated in a few places, it looks like AbstractCommunicator is an effort to make a reusable component for this, but it should be async callback-based, rather than used through inheritance (has-a rather than is-a), so it can be used anywhere, and for more than one type of communication in a class.

Comment 41

[:garvan]

More work is going in, but the current try server results:

https://tbpl.mozilla.org/?tree=Try&rev=7a3079f76805
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7a3079f76805

This is with the stumbler compile flag on.

Comment 42

[Richard Newman [:rnewman]]

I'm happy to be (one of) the reviewers.

Probably best to file a separate bug for that. This one's up to 42 comments already!

Comment 43

[:garvan]

(In reply to Nick Alexander :nalexander from comment #36)
> Comment on attachment 8454506 [details] [diff] [review]
> Part 3: Build stumbler JAR and integrate into Fennec.
> Review of attachment 8454506 [details] [diff] [review]:
> -----------------------------------------------------------------
> This is fine, but an interdiff or follow-up diff to be squashed is easier to
> review and to build upon.

Good point, will do that in future. The changes were to add:
'java/org/mozilla/mozstumbler/service/PersistentIntentService.java'

And to rename the *Starter to *Receiver.

Comment 44

[Nick Alexander :nalexander [he/him]]

I'm going to land a dummy version of this -- still preffed off -- so that we can move on to the landing review and integration bugs.

Comment 45

[Nick Alexander :nalexander [he/him]]

Place holder landed, compile-time preffed off.

remote:   https://hg.mozilla.org/integration/fx-team/rev/bf9c873c60f1
remote:   https://hg.mozilla.org/integration/fx-team/rev/18a81fe5e78b
remote:   https://hg.mozilla.org/integration/fx-team/rev/298f6155fc60
remote:   https://hg.mozilla.org/integration/fx-team/rev/84a78562c01a

Comment 46

[Nick Alexander :nalexander [he/him]]

Richard Newman [:rnewman] from comment #24)
> Comment on attachment 8450632 [details] [diff] [review]
> Part 4: Eclipse support. r=rnewman
> 
> Review of attachment 8450632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/moz.build
> @@ +640,5 @@
> >      omnijar.add_classpathentry(d, TOPSRCDIR + '/mobile/android/' + d, dstdir=d)
> >  
> >  # The omnijar is included in the Fennec APK (although it's empty,
> >  # having no resources, assets, or Java code).
> > +main.included_projects += ['../' + omnijar.name]
> 
> That explains the red cross in my library settings…

Probably, yes.

> @@ +652,5 @@
> >      main.included_projects += ['../' + crashreporter.name]
> > +
> > +if CONFIG['MOZ_ANDROID_MLS_STUMBLER']:
> > +    main.included_projects += ['../FennecStumbler']
> > +    main.referenced_projects += ['../FennecStumbler']
> 
> Does this need to be both included and referenced?

IIRC, included means the code is available to the main project (in a Java JAR-y type of way); referenced means that the Android library is compiled with the main project, and influences dependencies in a way that I haven't quite figured out.  I was erring on the side of fewer references (to avoid an odd Eclipse error about missing APK files), but that appears to cause dependency issues that I also can't explain.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf9c873c60f1
https://hg.mozilla.org/mozilla-central/rev/18a81fe5e78b
https://hg.mozilla.org/mozilla-central/rev/298f6155fc60
https://hg.mozilla.org/mozilla-central/rev/84a78562c01a