Support `defaultSearch` parameter of core telemetry ping

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 4 bugs)

unspecified
Firefox 48
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
gfritzsche
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
Margaret
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
29.94 KB, patch
Details | Diff | Splinter Review
3.16 KB, patch
Details | Diff | Splinter Review
3.28 KB, patch
Details | Diff | Splinter Review
8.84 KB, patch
Details | Diff | Splinter Review
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
Flags: needinfo?(gfritzsche)
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. :)
Created attachment 8721558 [details]
MozReview Request: Bug 1249288 - Add default search engine to core ping. r=rnewman

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/
Attachment #8721558 - Flags: review?(rnewman)
Created attachment 8721559 [details]
MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman

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/
Attachment #8721559 - Flags: review?(gfritzsche)
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".
Flags: needinfo?(gfritzsche)

Comment 9

3 years ago
(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.
Attachment #8721559 - Flags: review?(gfritzsche)
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.
Attachment #8721558 - Flags: review?(rnewman)
Blocks: 1251636
No longer blocks: 1220177
Created attachment 8727641 [details]
MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander

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/
Attachment #8727641 - Flags: review?(nalexander)
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/
Attachment #8721558 - Flags: review?(rnewman)
Created attachment 8727642 [details]
MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret

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/
Attachment #8727642 - Flags: review?(margaret.leibovic)
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/
Attachment #8721559 - Flags: review?(gfritzsche)
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?"
Attachment #8727641 - Flags: review?(nalexander) → review+
Attachment #8721559 - Flags: review?(gfritzsche)
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

?
Status: NEW → ASSIGNED
Attachment #8721558 - Flags: review?(rnewman)
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 24

3 years ago
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
Attachment #8727642 - Flags: review?(margaret.leibovic) → review+
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?
Flags: needinfo?(gfritzsche)
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/
Attachment #8721558 - Flags: review?(rnewman)
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/
Attachment #8721559 - Attachment description: MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche → MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman
Attachment #8721559 - Flags: review?(rnewman)
Attachment #8721559 - Flags: review?(gfritzsche)
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/
Created attachment 8729290 [details]
MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39345/
Attachment #8729290 - Flags: review?(rnewman)
(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``").
Flags: needinfo?(gfritzsche)
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
```
Attachment #8721559 - Flags: review?(gfritzsche)
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/
Attachment #8721559 - Flags: review?(gfritzsche)
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!
Attachment #8721559 - Flags: review?(gfritzsche) → review+
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.
Attachment #8721559 - Flags: review?(rnewman) → review+
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!
Attachment #8729290 - Flags: review?(rnewman)
Attachment #8721558 - Flags: review?(rnewman)
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/
Attachment #8721558 - Flags: review?(rnewman)
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/
Attachment #8729290 - Flags: review?(rnewman)
Attachment #8729290 - Flags: review?(rnewman)
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.
Attachment #8729290 - Flags: review?(s.kaspari)
(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/
Attachment #8729290 - Attachment description: MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=rnewman → MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian
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
Attachment #8721558 - Flags: review?(rnewman) → review+
Attachment #8729290 - Flags: review?(s.kaspari) → review+
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.
Flags: needinfo?(michael.l.comella)
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
Alessio, can you verify the defaultSearch field is appearing on nightly builds?
Flags: needinfo?(alessio.placitelli)
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)]
Flags: needinfo?(alessio.placitelli)
(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
Flags: needinfo?(rnewman)
Seems reasonable that almost 1% of users will install a third-party search engine and set it as default.
Flags: needinfo?(rnewman)
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
Flags: needinfo?(gfritzsche)
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
Flags: needinfo?(gfritzsche)
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.
Flags: needinfo?(michael.l.comella)
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
Attachment #8721558 - Flags: approval-mozilla-aurora?
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!
Flags: needinfo?(michael.l.comella)

Updated

3 years ago
status-firefox47: --- → affected
(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.
Flags: needinfo?(michael.l.comella)
(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+
Attachment #8721558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8721559 [details]
MozReview Request: Bug 1249288 - Update telemetry docs to include defaultSearch. r=gfritzsche,rnewman

[Triage Comment]
Attachment #8721559 - Flags: approval-mozilla-aurora+
Comment on attachment 8727641 [details]
MozReview Request: Bug 1249288 - Move om.search.providers.SearchEngine\* to omg.search. r=nalexander

[Triage Comment]
Attachment #8727641 - Flags: approval-mozilla-aurora+
Comment on attachment 8727642 [details]
MozReview Request: Bug 1249288 - Don't call SearchEngineManager change callback if it's null. r=margaret

[Triage Comment]
Attachment #8727642 - Flags: approval-mozilla-aurora+
Comment on attachment 8729290 [details]
MozReview Request: Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian

[Triage Comment]
Attachment #8729290 - Flags: approval-mozilla-aurora+
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)
Flags: needinfo?(michael.l.comella)
Attachment #8742514 - Attachment is obsolete: true
Attachment #8742515 - Attachment is obsolete: true
Attachment #8742514 - Attachment is obsolete: false
Attachment #8742515 - Attachment is obsolete: false
Attachment #8742518 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella) → needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.