Shouldn't be too hard – let's do this before v3 (bug 1243585).

This gets more complicated because we have to retrieve the search engines from Gecko. Since we don't want to wait for Gecko, the first time we open Firefox, the default search engine will not be available and we will not be able to provide a value for `defaultSearchEngine`. My proposal would be to omit the attribute in these cases. How does that sound, Georg? Other notes: * If we need to do some analyses with defaultSearchEngine and are not okay omitting the field, my implementation hooks into the only Gecko->Java search engine data listener and will store the data if the search engines are ever viewed. Therefore, we can assume these users still have the default search engine for their locale & FF version (though this can be somewhat fragile if the search engines are ever accessed in a different way). * In order to store the search engine data, I'm devising something similar to the ProfileInformationCache from FHR which stores values on disk from the Gecko profile when Gecko is available and provides access to them when Gecko is not

I had a passing thought that instead of accessing this from Gecko, we could just read it from the profile, like profile creation date & clientID. However, I'm okay waiting for Gecko for the following reasons: * The search engine file format is more likely to change than the file format for the two values we're currently reading from disk * The default search engine is less mission-critical than the other values we retrieve * As per comment 1, given the current implementation, we can assume what the default search engine is based on locale & FF version (then again, it's possible an add-on could change the search engines without Java ever being updated... But we probably don't care about that edge case) If we wanted to pursue the read-from-disk approach, MattN said florian might know how frequently the value changes.

re comment 2, MattN pointed out we use a non-standard version of lz4 to write this file (see bug 1209390) so it'd take a lot of work to read it directly – let's not. :)

The default search engine attribute may be omitted if we haven't been able to retrieve the value from Gecko yet. Note that with this implementation, we hook into the only Gecko->Java search engines message so the user can't have changed the search engines without us having seen it (except perhaps in an addon, but that's a huge edge case) so if the field is omitted, we can assume what the default search engine is based on Firefox locale & version. Review commit: https://reviewboard.mozilla.org/r/35711/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35711/

Includes an Android-specific note that the default search engine may be omitted. Review commit: https://reviewboard.mozilla.org/r/35713/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35713/

Implementation in comment 4 and comment 5 is as described in comment 1.

Any reason this doesn't just call SearchEngineManager.getEngine()?

(In reply to Michael Comella (:mcomella) from comment #1) > This gets more complicated because we have to retrieve the search engines > from Gecko. Since we don't want to wait for Gecko, the first time we open > Firefox, the default search engine will not be available and we will not be > able to provide a value for `defaultSearchEngine`. My proposal would be to > omit the attribute in these cases. I would suggest not omitting the field and instead submitting it with a `null` value. This matches what we do on Desktop for similar situations; we don't have to make that field optional and also have a signal for "no data yet".

(In reply to Richard Newman [:rnewman] from comment #7) > Any reason this doesn't just call SearchEngineManager.getEngine()? Richard beat me to it - we do have a Java search service implementation that mirrors the default in Gecko. We store that value in SharedPreferences.

With Richard's proposal in comment 7, my concern was that the callback that SearchEngineManager.getEngine() will not be called immediately so we will not upload the telemetry immediately on startup, as is one of our telemetry priorities. I spoke to rnewman, who said that the distribution callbacks (which SearchEngineManager.getEngine waits for) will get called shortly after the Distribution system is inited, which currently occurs in BrowserApp.onCreate [1]. We should add some comments to ensure this method is not removed and this remains the case. That being said, if we don't want to wait on the Distribution code, we can use the implementation from comment 5 (at the cost of adding a lot of extra code we have to maintain). [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#705

Running the getEngine callback locally, it appears to be called instantaneously on my N7. With rnewman's vouch, seems reasonable to me.

I just ran into a crash with my implementation when opening fennec -> 3-dot settings -> search -> change default engine -> hit back some number of times: 02-23 17:56:44.841 E/GeckoCrashHandler(10535): java.lang.NullPointerException 02-23 17:56:44.841 E/GeckoCrashHandler(10535): at org.mozilla.gecko.search.SearchEngineManager$1.run(SearchEngineManager.java:148) The crash in question is in SearchEngineManager.runCallback: ThreadUtils.postToUiThread(new Runnable() { @Override public void run() { // Cache engine for future calls to getEngine. SearchEngineManager.this.engine = engine; 148 callback.execute(engine); } }); It looks like this might have been called from the preference change listener part of this code, which calls out to the changeCallback member, which only gets set via `setChangeCallback`. Since I never call that, it's null.

Since we're running on a background thread & dealing with callbacks, we also have to be concerned about the Activity lifecycle – will the Context/Activity still be alive when the callback returns? Looking into it...

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://reviewboard.mozilla.org/r/35713/#review32869 See comment 8.

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Michael says this isn't ready for review yet.

We want to reuse this code for the main Activity. Review commit: https://reviewboard.mozilla.org/r/38563/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38563/

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35711/diff/1-2/

This may occur if setChangeCallback is never called. Review commit: https://reviewboard.mozilla.org/r/38565/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38565/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35713/diff/1-2/

Comment on attachment 8727641 [details] MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander https://reviewboard.mozilla.org/r/38563/#review35183 If it builds for you with |mach build|, it works for me. ::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:106 (Diff revision 1) > + // distribution engine called twice for over the air engines Huh? Make this a full sentence. Do you mean, "It is possible for the engine to be called twice for engines shipped over the air? In this case, we skip the {first,second} invocation?"

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://reviewboard.mozilla.org/r/35713/#review35313 ::: toolkit/components/telemetry/docs/core-ping.rst:41 (Diff revision 2) > + "defaultSearch": <string>, // default search engine, e.g. "yahoo". > + // This field may be `null`. Let's add a bit documentation below on when this can be `null`. We should probably add headings for the `device` and `defaultSearch` field sections, see `main-ping.rst` for an example. Trivial nit: "Default" with upper-case D.

> + "defaultSearch": <string>, // default search engine, e.g. "yahoo". Is this the value of the default search engine field ("Yahoo"): https://dxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/chrome/region.properties#15 or the identifier of the default search engine ("yahoo"): https://dxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/searchplugins/list.txt#5 ?

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman https://reviewboard.mozilla.org/r/35711/#review35471 f+: concurrency. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:702 (Diff revision 2) > + // The telemetry core ping needs to upload as quickly as possible. It relies on the Distribution // We want to upload the telemetry core ping as soon after startup as possible. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1428 (Diff revision 2) > + searchEngineManager.destroy(); And null it out for sanity. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4023 (Diff revision 2) > + searchEngineManager.getEngine(new UploadTelemetryCallback(getContext(), seq, profile.getName(), profile.getDir())); Nit: prefer explicit `this.` `uploadTelemetry` is called in a background runnable. `onDestroy` might have been called by now, leaving you with a destroyed (and perhaps nilled out) search engine manager. You have a few options: * Don't clean up the search engine manager in `onDestroy`. * Capture it outside the runnable, find whatever you need to find, and pass that as an argument to `uploadTelemetry`. * Check here whether you have a value. You'll need to make `searchEngineManager` a thread-safe reference of some kind. Concurrency hard is. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4033 (Diff revision 2) > + contextWeakReference = new WeakReference<>(context); Prefer `this.contextWeakReference = …`. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4048 (Diff revision 2) > + i.putExtra(TelemetryConstants.EXTRA_DEFAULT_SEARCH_ENGINE, engine.getIdentifier()); This answers my documentation question. Please make sure the documentation is clear :)

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret https://reviewboard.mozilla.org/r/38565/#review35583

https://reviewboard.mozilla.org/r/38563/#review35183 > Huh? Make this a full sentence. Do you mean, "It is possible for the engine to be called twice for engines shipped over the air? In this case, we skip the {first,second} invocation?" Oops! Didn't know I changed that. I think this is something Richard told me off-hand but I'm not sure this is the place for the comment even if it's true so I'm going to just drop it.

Since I'm adding the defaultSearch field, do I need to bump the ping format version here too, Georg?

https://reviewboard.mozilla.org/r/35711/#review35471 > Nit: prefer explicit `this.` > > `uploadTelemetry` is called in a background runnable. `onDestroy` might have been called by now, leaving you with a destroyed (and perhaps nilled out) search engine manager. > > You have a few options: > > * Don't clean up the search engine manager in `onDestroy`. > * Capture it outside the runnable, find whatever you need to find, and pass that as an argument to `uploadTelemetry`. > * Check here whether you have a value. You'll need to make `searchEngineManager` a thread-safe reference of some kind. > > Concurrency hard is. > Don't clean up the search engine manager in onDestroy. Can't do this – the `SearchEngineManager` has a reference to context so if the Activity is ever destroyed and the process isn't dying, we'll leak the context. > Check here whether you have a value. You'll need to make searchEngineManager a thread-safe reference of some kind. Locking (i.e. the thread-safe reference you mentioned) greatly increases the complexity so let's try to avoid that. > Capture it outside the runnable, find whatever you need to find, and pass that as an argument to uploadTelemetry. It changes the code flow a little bit, but I think this is the simplest, given that it just uses our standard repetoire. Let's go for it!

Comment on attachment 8727641 [details] MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38563/diff/1-2/

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35711/diff/2-3/

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38565/diff/1-2/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35713/diff/2-3/

Review commit: https://reviewboard.mozilla.org/r/39345/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39345/

(In reply to Michael Comella (:mcomella) from comment #26) > Since I'm adding the defaultSearch field, do I need to bump the ping format > version here too, Georg? Yes, we should do that now. We should have a simple version history in core-ping.rst so we can see what version added what (e.g. just "version 2: addded ``defaultSearch``").

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://reviewboard.mozilla.org/r/35713/#review36349 ::: toolkit/components/telemetry/docs/core-ping.rst:28 (Diff revision 3) > ping”, not total for the whole application lifetime. > > Structure:: > > { > "v": 1, // ping format version We should bump this. ::: toolkit/components/telemetry/docs/core-ping.rst:47 (Diff revision 3) > + // e.g. "yahoo". > > "experiments": [<string>, …], // Optional, array of identifiers > // for the active experiments > } > Let's add a simple version history, e.g. just: ``` versions -------- * version 2: added ``defaultSearch`` * version 1: initial version ```

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35711/diff/3-4/

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38565/diff/2-3/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35713/diff/3-4/

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39345/diff/1-2/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://reviewboard.mozilla.org/r/35713/#review36567 Thanks, this looks good!

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://reviewboard.mozilla.org/r/35713/#review37103 ::: toolkit/components/telemetry/docs/core-ping.rst:64 (Diff revision 4) > > +defaultSearch > +~~~~~~~~~~~~~ > +On Android, this field may be ``null``. To get the engine, we rely on > +``SearchEngineManager#getDefaultEngine``, which searches in several places in > +order to find the search engine name: You use `name` throughout here, but we record the identifier.

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian https://reviewboard.mozilla.org/r/39345/#review37105 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1082 (Diff revision 2) > - // > + // > - // So we're left with onStart/onStop. > + // So we're left with onStart/onStop. > - uploadTelemetry(profile); > - } > - }); > + // > + // Also, we use the SearchEngineManager on the UIThread so it doesn't > + // get destroyed while we're trying to access it. > + searchEngineManager.getEngine(new UploadTelemetryCallback(this)); I think a better solution is to check whether SEM has been destroyed at point of use. Make `getEngine` throw `IllegalStateException` or similar… or have it simply not call the callback? I'm pretty sure we don't want to initialize the SEM -- complete with disk access -- on the UI thread during onStart!

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman https://reviewboard.mozilla.org/r/35711/#review37191 Clearing review -- needs flattening.

I don't see any disk access when initializing the SEM so I think it's okay to initialize on the main thread. I opt-ed to change the `destroy` method into an `unregisterListeners` method and not actually null out the SearchEngineManager state (making those variables final where possible). This means we'll have an internally consistent object even during callbacks, so `unregisterListeners` can be called while the callbacks are running. I felt this was a simpler solution than the one we discussed on IRC (with the imaginary object and the real object). While it's not strictly necessary, I also added WeakReferences to the SEM in the callbacks so we can GC ASAP if the Activity (and thus the SEM) gets GC'd. This is important since we hold a reference to Context which can be a rather large object. Furthermore, I added some related thread annotations where I felt they were useful.

Comment on attachment 8727641 [details] MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38563/diff/2-3/

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35711/diff/4-5/

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38565/diff/3-4/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35713/diff/4-5/

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39345/diff/2-3/

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian https://reviewboard.mozilla.org/r/39345/#review38685 ::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:71 (Diff revision 3) > // This should go through GeckoInterface to get the UA, but the search activity > // doesn't use a GeckoView yet. Until it does, get the UA directly. > private static final String USER_AGENT = HardwareUtils.isTablet() ? > AppConstants.USER_AGENT_FENNEC_TABLET : AppConstants.USER_AGENT_FENNEC_MOBILE; > > - private Context context; > + private final Context context; What you've done is made SEM the immutable object we talked about. Which is good, assuming that it doesn't cause the `context` -- which might be an `Activity` -- to leak!

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian This seems fine to me, but I'm distracted, so let's get a second set of eyes.

(In reply to Richard Newman [:rnewman] from comment #49) > What you've done is made SEM the immutable object we talked about. Which is > good, assuming that it doesn't cause the `context` -- which might be an > `Activity` -- to leak! Ideally, we wouldn't keep a reference to Context (I filed bug 1259519). I'll add some comments to the reference to SEM in BrowserApp/SearchActivity and to the SEM javadoc that it holds a reference to Context so we shouldn't leak it.

Comment on attachment 8727641 [details] MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38563/diff/3-4/

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35711/diff/5-6/

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38565/diff/4-5/

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35713/diff/5-6/

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39345/diff/3-4/

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman https://reviewboard.mozilla.org/r/35711/#review39319

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian https://reviewboard.mozilla.org/r/39345/#review39491 r+ LGTM. Just some thoughts. :) ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:290 (Diff revision 4) > > private final DynamicToolbar mDynamicToolbar = new DynamicToolbar(); > private final ScreenshotObserver mScreenshotObserver = new ScreenshotObserver(); > > - private SearchEngineManager searchEngineManager; > + @NonNull > + private SearchEngineManager searchEngineManager; // Contains reference to Context - DO NOT LEAK! You have good intentions here but is this comment helpful? There are potentially a lot of objects here holding references to BrowserApp. Does adding a comment prevent leaking them? Will I read this comment before I pass searchEngineManager to some other object? I haven't looked at the whole patch series though. :) ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3956 (Diff revision 4) > mDynamicToolbar.setTemporarilyVisible(false, VisibilityTransition.IMMEDIATE); > } > > - private void uploadTelemetry(final GeckoProfile profile) { > - if (!TelemetryUploadService.isUploadEnabledByProfileConfig(this, profile)) { > + @WorkerThread // synchronous SharedPrefs write. > + private static void uploadTelemetry(final Context context, final GeckoProfile profile, > + final org.mozilla.gecko.search.SearchEngine defaultEngine) { Oh, do we have conflicting "SearchEngine" classes here? This is a bit messy. :( ::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:130 (Diff revision 4) > * > * When <code>distributionNotFound</code> is called, > * {@link org.mozilla.gecko.distribution.Distribution#exists()} will return > * false. In the other two callbacks, it will return true. > */ > + @WorkerThread Hm. Adding this annotation to an interface is a bit contradictory: Shouldn't "the method should only be called on a worker thread" be an implementation detail and an interface is not an implementation? I assume this is needed because the checks do not follow the call stack. I wish the check would be fixed/improved rather than those annotations being propagated through the code base. ::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:48 (Diff revision 4) > import java.util.Locale; > > +/** > + * This class is not thread-safe, except where otherwise noted. > + * > + * This class contains a reference to {@link Context} - DO NOT LEAK! What makes this class's context special? Isn't this a generic guideline instead of a specific tip? Do not "leak" anything. ::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:154 (Diff revision 4) > private void runCallback(final SearchEngine engine, @Nullable final SearchEngineCallback callback) { > - ThreadUtils.postToUiThread(new Runnable() { > + ThreadUtils.postToUiThread(new RunCallbackUiThreadRunnable(this, engine, callback)); > + } > + > + // Static is not strictly necessary but the outer class has a reference to Context so we should GC ASAP. > + private static class RunCallbackUiThreadRunnable implements Runnable { This is an interesting way to decouple the callback from this class. Maybe this could even be a nice helper (abstract) class for other UI callbacks we have all over the app.

https://reviewboard.mozilla.org/r/39345/#review39491 > You have good intentions here but is this comment helpful? There are potentially a lot of objects here holding references to BrowserApp. Does adding a comment prevent leaking them? Will I read this comment before I pass searchEngineManager to some other object? I haven't looked at the whole patch series though. :) I think leaking `Context`, `Activity` instances, or anything holding a reference to any of them (e.g. `Views`) is the biggest concern because they hold a reference to basically everything so the memory use basically doubles any time the Activity is recreated. Maintaining an unused reference to a self-contained Object typically won't make the same impact. As such, when I use `Context`s or `View`s, I'm extra careful about the references I keep in order to prevent impactful leaks (as opposed to using other Objects). This comment is to inspire the dev to do the same thing and remind them that we should be careful about memory leaks. That being said, we should try to avoid holding references to `Context` or `Activity` altogether -- I filed a bug 1259519 for SearchEngineManager. > Hm. Adding this annotation to an interface is a bit contradictory: Shouldn't "the method should only be called on a worker thread" be an implementation detail and an interface is not an implementation? > > I assume this is needed because the checks do not follow the call stack. I wish the check would be fixed/improved rather than those annotations being propagated through the code base. > Hm. Adding this annotation to an interface is a bit contradictory: Shouldn't "the method should only be called on a worker thread" be an implementation detail and an interface is not an implementation? With regards to programming construct semantics, that's a really good point! However... > I assume this is needed because the checks do not follow the call stack. I wish the check would be fixed/improved rather than those annotations being propagated through the code base. I agree that it'd be better if the implementation were improved. Given the situation and that we're unlikely to ever call these callbacks on the UiThread, the annotations should help us catch bugs so I'm okay with it. > What makes this class's context special? Isn't this a generic guideline instead of a specific tip? Do not "leak" anything. As I mention above, leaking `Context`s is worse than most other things. The comment serves as a reminder. You may be right that it's excessive but if it doesn't harm readability, I'm okay with it. > This is an interesting way to decouple the callback from this class. Maybe this could even be a nice helper (abstract) class for other UI callbacks we have all over the app. Filed bug 1260549.

NI self to request uplift.

https://hg.mozilla.org/integration/fx-team/rev/aafa9cc405de7f17f05763ecbfe3278729a701ed Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander https://hg.mozilla.org/integration/fx-team/rev/6ecd4b60ef8ad9fe5517eff40ba4810ca6069a52 Bug 1249288 - Add default search engine to core ping. r=rnewman https://hg.mozilla.org/integration/fx-team/rev/afb10f688cdd975aa0986cc7e943fd1a4dc83ea4 Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret https://hg.mozilla.org/integration/fx-team/rev/0568d580c4a79282dd99b3dd8e4013136e1eb794 Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman https://hg.mozilla.org/integration/fx-team/rev/ee11a508dd472945960264b5ca777dccc5961bdf Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian

https://hg.mozilla.org/mozilla-central/rev/aafa9cc405de https://hg.mozilla.org/mozilla-central/rev/6ecd4b60ef8a https://hg.mozilla.org/mozilla-central/rev/afb10f688cdd https://hg.mozilla.org/mozilla-central/rev/0568d580c4a7 https://hg.mozilla.org/mozilla-central/rev/ee11a508dd47

Alessio, can you verify the defaultSearch field is appearing on nightly builds?

Sure, I'd wait a couple of days to get some data though, to be safe. Leaving the ni? around so this stays on my radar.

It is showing up on Nightly: https://gist.github.com/georgf/083dac118168b543c71f7ca011a3d4e8 The engine value distribution is (in percent): [(u'baidu', 3.492), (u'google', 65.977), (u'duckduckgo', 2.427), (u'wikipedia-es', 0.026), (u'bing', 0.911), (u'amazondotcom', 0.005), (u'yahoo', 22.704), (None, 0.846), (u'yahoo-france', 0.076), (u'yandex-ru', 3.534), (u'wikipedia-de', 0.003)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #65) > (None, 0.846), Is that an expected level or too high?

(In reply to Georg Fritzsche [:gfritzsche] from comment #66) > (In reply to Georg Fritzsche [:gfritzsche] from comment #65) > > (None, 0.846), > > Is that an expected level or too high? I should mention that those are the "null" defaultSearch values (maps to None in Python spark jobs).

(In reply to Georg Fritzsche [:gfritzsche] from comment #66) > (In reply to Georg Fritzsche [:gfritzsche] from comment #65) > > (None, 0.846), > > Is that an expected level or too high? Richard? For a recap, [1] (which I copied from the javadoc) should indicate when this field could be null. [1]: https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/core-ping.html#defaultsearch

Seems reasonable that almost 1% of users will install a third-party search engine and set it as default.

Shouldn't the 3rd party ones show up too? That was my understanding from the docs. If not, it might be nice to differentiate between "can't retrieve engine" and "some 3rd party engine" in a follow-up bug.

(In reply to Georg Fritzsche [:gfritzsche] from comment #70) > Shouldn't the 3rd party ones show up too? That was my understanding from the > docs. > If not, it might be nice to differentiate between "can't retrieve engine" > and "some 3rd party engine" in a follow-up bug. Tested locally – we actually crash with 3rd party engines set as the default (oops!). Filed bug 1263758 for the fix and bug 1263761 to update the docs. Finkle suggested the possibility that the defaultSearch engine is null when we first start up but gets populated shortly afterward – for these clients with null engines, would it be possible to run a query to see if these clients engines get populated on the next ping? fwiw, I'm unfamiliar with the search engine retrieval code and due to locales & distributions callbacks, it's non-trivial to identify all of the cases where the return value could be `null`. How significant is null for ~1% [1] of these pings? If insignificant, I think my time is better spent continuing to develop the other telemetry features rather than digging into this at the moment – would you agree? We could also try to get another team member to analyze the engine retrieval code. [1]: I'd expect that number might grow with my custom engine fix

If we expect the 1% to be mostly bug 1263758, then i think we can wait for that fix and re-validate. In the mean-time i can check how the defaultSearch==null distributes over the client history and then we'll see if there might be other issues to check into. I'll leave the needinfo open for this.

(In reply to Georg Fritzsche [:gfritzsche] from comment #72) > In the mean-time i can check how the defaultSearch==null distributes over > the client history and then we'll see if there might be other issues to > check into. The affected clients only have `null` values for `defaultSearch` values: https://gist.github.com/georgf/234b7c861cc78824071d0ed9cf7a6aa2

Worth calling out that there are only 6 affected clients, i upated the gist to be more clear about that. Possibly because 3rd party search providers are rare on Nightly?

(In reply to Georg Fritzsche [:gfritzsche] from comment #72) > If we expect the 1% to be mostly bug 1263758, then i think we can wait for > that fix and re-validate. I meant to say that we'll continue to have the 1% PLUS another set of nulls for people who have a non-built-in search engine. These numbers weren't included before because we were crashing (bug 1263758). I don't know where the original 1% comes from. In any case, I'm going to flag this for uplift because it's better to have the numbers that we can look at carefully than to not have them at all.

I filed bug 1263983 to investigate the null issues.

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman This request applies for all patches landed in this bug. This must be uplifted with bug 1263758. Approval Request Comment [Feature/regressing bug #]: Telemetry implementation [User impact if declined]: We'll have to wait another release cycle in order to get default search engine information. [Describe test coverage new/current, TreeHerder]: Tested locally, in Nightly for 13 days. [Risks and why]: Medium. The code to add the defaultSearch engine to the ping is fairly straight-forward, but I needed to add some code to the SearchEngineManager to let it be accessed during the Android lifecycle by any thread. This code is traditionally tricky and can lead to memory leaks or NullPointerExceptions – I was careful to do my best to avoid these issues. We didn't add locks so there's no possibility of deadlocking and we removed the object destruction, so there's no possibility of calling a deleted object. In the worst case, we get corrupted data. This code runs often (every time fennec is opened) so it's unlikely there are remaining issues. [String/UUID change made/needed]: None

Hi Mike, have we verified whether this telemetry data is showing up from Nightly channel? I just want to make sure we have a good "working" implementation before uplifting to Aurora 47. Thanks!

(In reply to Ritu Kothari (:ritu) from comment #78) > Hi Mike, have we verified whether this telemetry data is showing up from > Nightly channel? I just want to make sure we have a good "working" > implementation before uplifting to Aurora 47. Thanks! Hi Ritu - Comment 65 shows data coming into Nightly. Mike identifed an issue and fixed it in bug 1263758 (needs uplift too). Bug 1263983 was opened to followup and investigate any other sources of "null" search engines. We should be good to go for now.

(In reply to Mark Finkle (:mfinkle) from comment #79) > (In reply to Ritu Kothari (:ritu) from comment #78) > > Hi Mike, have we verified whether this telemetry data is showing up from > > Nightly channel? I just want to make sure we have a good "working" > > implementation before uplifting to Aurora 47. Thanks! > > Hi Ritu - Comment 65 shows data coming into Nightly. Mike identifed an issue > and fixed it in bug 1263758 (needs uplift too). Bug 1263983 was opened to > followup and investigate any other sources of "null" search engines. > > We should be good to go for now. Thanks guys for the due diligence! Will A+ uplifts in both these bugs now.

Comment on attachment 8721558 [details] MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman Fennec default search engine telemetry related, telemetry data was verified on Nightly, Aurora47+

Comment on attachment 8721559 [details] MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman [Triage Comment]

Comment on attachment 8727641 [details] MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander [Triage Comment]

Comment on attachment 8727642 [details] MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret [Triage Comment]

Comment on attachment 8729290 [details] MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian [Triage Comment]

has problems with uplifting to aurora: grafting 336726:aafa9cc405de "Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander" merging mobile/android/base/moz.build merging mobile/android/search/java/org/mozilla/search/PostSearchFragment.java merging mobile/android/search/java/org/mozilla/search/SearchActivity.java merging mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java warning: conflicts while merging mobile/android/search/java/org/mozilla/search/PostSearchFragment.java! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use hg resolve and hg graft --continue)

Finkle reminded me that I HAVE THE POWER~~~! remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/616c474808cc remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/130280323942 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/c046a7887ffa remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/a5801fb8ba64 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/98b3c3a9c273

(In reply to Michael Comella (:mcomella) from comment #92) > Finkle reminded me that I HAVE THE POWER~~~! > > remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/616c474808cc > remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/130280323942 > remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/c046a7887ffa > remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/a5801fb8ba64 > remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/98b3c3a9c273 thanks :) setting the flag