Closed Bug 1024708 Opened 5 years ago Closed 5 years ago

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

Categories

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

33 Branch
All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: garvan, Assigned: nalexander)

References

Details

Attachments

(6 files, 9 obsolete files)

3.32 KB, patch
Details | Diff | Splinter Review
678 bytes, patch
Details | Diff | Splinter Review
2.25 KB, patch
Details | Diff | Splinter Review
158.97 KB, patch
Details | Diff | Splinter Review
7.46 KB, patch
Details | Diff | Splinter Review
4.83 KB, patch
nalexander
: feedback+
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → gkeeley
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?
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.
Target Milestone: --- → Firefox 33
Attached file android-services.mozbuild (obsolete) —
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.
(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?
I'd rather us use the tracking flag instead of specifying a target milestone for nothing that has landed.
Target Milestone: Firefox 33 → ---
(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.
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.
(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.
> 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?
Depends on: 812227
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.
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">
No longer depends on: 812227
Attached patch WIP_stumbler_integration.patch (obsolete) — Splinter Review
Builds ok.
This is missing the proper integration step for stumbler/manifests/*.xml.in

Nick could I bug you for how to do that?
Attachment #8439466 - Attachment is obsolete: true
Attachment #8439480 - Attachment is obsolete: true
Attachment #8439526 - Attachment is obsolete: true
Flags: needinfo?(nalexander)
Flags: needinfo?(nalexander)
Blocks: 1032506
Attached patch WIP_stumbler_integration.patch (obsolete) — Splinter Review
Nick, hopefully this is useful
Attachment #8447222 - Attachment is obsolete: true
OS: All → Android
Hardware: x86 → All
This is the Java code from the MozStumbler git repo.
Attachment #8450494 - Flags: review+
rnewman, you did the last one, so I'll use you again.  Feel free to
rubber-stamp or re-direct.
Attachment #8450495 - Flags: review?(rnewman)
rnewman: ditto, but a slightly different integration, since the
stumbler doesn't (yet) depend on Fennec at all.
Attachment #8450497 - Flags: review?(rnewman)
Attachment #8449933 - Attachment is obsolete: true
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...
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.
Attachment #8450632 - Flags: review?(rnewman)
This is how I launched the passive stumbler.  I can't guarantee
anything is happening, but the service starts and I see log messages.
Attachment #8450634 - Flags: feedback?(gkeeley)
Attachment #8450495 - Flags: review?(rnewman) → review+
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".
Attachment #8450497 - Flags: review?(rnewman) → review+
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?
Attachment #8450632 - Flags: review?(rnewman) → review+
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.
Attachment #8450634 - Flags: feedback?(gkeeley) → feedback+
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)
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).
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.
(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.
(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.
Depends on: 1036508
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
Attachment #8450494 - Attachment is obsolete: true
Attachment #8454204 - Flags: review?(nalexander)
updated list of stumbler build files for added file and changed name of PassiveServiceStarter to PassiveServiceReceiver
Attachment #8450497 - Attachment is obsolete: true
Attachment #8454216 - Flags: review?(nalexander)
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.
Rename *Starter to *Receiver
Attachment #8454216 - Attachment is obsolete: true
Attachment #8454216 - Flags: review?(nalexander)
Attachment #8454506 - Flags: review?(nalexander)
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.
Attachment #8450634 - Attachment is obsolete: true
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.
Attachment #8454506 - Flags: review?(nalexander) → review+
(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 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).
Attachment #8454511 - Flags: feedback+
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.
Attachment #8454204 - Flags: review?(nalexander) → review+
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.
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/gkeeley@mozilla.com-7a3079f76805

This is with the stumbler compile flag on.
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!
Component: General → Build Config & IDE Support
(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.
Blocks: 1038843
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.
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.
Depends on: 1038923
No longer depends on: 1036508
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 33 → mozilla33
You need to log in before you can comment on or make changes to this bug.