841554 - Ensure FHR search engine works in non en-US locales

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P2)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #828540 +++

In the comments in bug 828540, Gavin mentioned that the search engine name (which we take the lower case of to normalize to the FHR field name) is localized. If non en-US locales use a different search engine name (e.g. "Amazon" instead of the en-US of "Amazon.com"), these searches would inadvertently fall into the "other" bucket.

We should at least audit the search XML files in other locales and ensure the search engine name matches the en-US definition. We should further considering adding a constant-across-locales engine identifier and use that instead of name for bucketing into FHR.

Comment 1

[Gregory Szorc [:gps]]

Axel: I'm embarrassed to ask this, but I'm not sure how I can go about finding a build with localized search engine names. Could you please teach me how to fish? If you know of such a build offhand, please let me know!

Comment 2

[Axel Hecht [:Pike]]

So the quick answer is, no, that won't work as is.

Amazon uses different domains in different regions.

I'm not sure why ebay isn't on the list? We also have partnerships with yandex, and a bunch of the plugins in Chinese and Japanese, IIRC.

Builds are http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/, and you can mxr across locales on http://mxr.mozilla.org/l10n-mozilla-aurora/. If FHR is on aurora, http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora-l10n/ are the aurora builds, we have better l10n coverage on those, some of the l10n-central ones are not maintained.

Comment 3

[Richard Newman [:rnewman]]

I took a look. As an example:

     <SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">
     <ShortName>Amazon.fr</ShortName>
     <Description>Recherche Amazon.fr</Description>
     <InputEncoding>ISO-8859-1</InputEncoding>
     <Image width="16" height="16">…
     <Url type="text/html" method="GET" template="http://www.amazon.fr/exec/obidos/external-search/">
       <Param name="field-keywords" value="{searchTerms}"/>
       <Param name="mode" value="blended"/>
       <Param name="tag" value="firefox-fr-21"/>
       <Param name="sourceid" value="Mozilla-search"/>
     </Url>
     <SearchForm>http://www.amazon.fr/</SearchForm>
     </SearchPlugin>

with the list being:

  google
  amazon-france
  bing
  wikipedia-fr
  eBay-france
  yahoo-france
  cnrtl-tlfi-fr


So no, a French user searching Amazon.fr will have their search reported in the "other" bucket.

There are a few possible courses of action here.

We could dump all of our partner search engines into the list (basically `grep ShortName */browser/searchplugins/*.xml` and munge), and treat them all separately: we'd have Amazon.co.uk, Amazon.fr, Amazon.com as partners, and rely on Metrics to combine them somehow. Brittle.

We could change our partner recognition logic to process searchplugins/list.txt and parse the XML files in that directory, rather than a hard-coded list of partners -- but again, we'd get a very scattered set of responses. Complex.

We could attempt to produce a set of sets, picking some partner identifier for each search provider. This identifier would have to be maintained somewhere, either in FHR as part of a mapping, or bottom-up in each locale's searchplugins files (e.g., extending the SearchPlugin XML format to include a partner identifier). Also crummy.

Either the second or third solution seems best to me, but it depends whether we want to get "Amazon" in our analytics, regardless of country, or whether we want the granularity.

Do we have a document/summary somewhere with all of our search partners, or are each of these partners added ad hoc to the l10n tree?

Anyone know who would know?

Comment 4

[Gregory Szorc [:gps]]

Relying on the output of localization is extremely brittle.

I think we should add an optional, constant, non-localized search engine ID to the XML files and use that as the FHR identifier. We still need to manage a "whitelist" of search engines in FHR because we a) only care about search engines Mozilla has an official relationship with b) we don't want to leak potentially privacy sensitive or user identifying search engine names in the payload.

This will probably involve some kind of test added to localization output to ensure all the relevant partners' XML files have the IDs defined.

This is going to be an ugly bug and I don't know where to start. I kind of doubt we'll get this in 21.

Comment 5

[Gregory Szorc [:gps]]

Dolske: I'm curious what your thoughts are here.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

The "engines we have a relationship with" requirement seems kind of fuzzy. Does this data collection really need to be that comprehensive? I agree there's no easy solution here.

Comment 7

[Richard Newman [:rnewman]]

Daniel, can you clarify what problem this reported data is trying to solve, and what kind of granularity we need to return?

The original FHR spreadsheet mentions "engines with a search partner agreement with Mozilla", but then provides a non-exhaustive list, and doesn't describe how those partners are to be identified.

Comment 8

[Gregory Szorc [:gps]]

The original set of requirements defined {Google, Yahoo, Bing, Amazon.com} as the search engines we care about. While some have theorized that this list may be expanded to include search engines like Yandex, no such requirements have been communicated to me. Let's assume the generic requirement is "some subset of the default search engines must be reliably reported on in FHR."

Comment 9

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #8)
> The original set of requirements defined {Google, Yahoo, Bing, Amazon.com}
> as the search engines we care about.

See preceding comment!

> Let's assume the generic
> requirement is "some subset of the default search engines must be reliably
> reported on in FHR."

I elected to ask, rather than assume :D

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Gregory Szorc [:gps] from comment #4)

> I think we should add an optional, constant, non-localized search engine ID
> to the XML files and use that as the FHR identifier. 

I don't have a strong opinion here, but that sounds reasonable. But I do think the first step is understanding exactly what the need is here (comment 7) so you can know if the solution is adequate or not.

> We still need to manage
> a "whitelist" of search engines in FHR because we a) only care about search
> engines Mozilla has an official relationship with b) we don't want to leak
> potentially privacy sensitive or user identifying search engine names in the
> payload.

I assume that's only relevant for non-default search plugins? I wonder if it would be useful to detect the plugin's origin, and only report info if it's a thing that's built-in.

Comment 11

[Daniel Einspanjer [:dre] [:deinspanjer]]

Thanks for asking.  gps is right that we provided a known subset that we had previously encountered questions about.

The purpose of this data is to ensure the correctness of the data that comes from our partners about the volume and type of traffic being directed to the partner.

Because the client only communicates directly with the partner, we have no ability to verify how many searches are being made through the various mechanisms.

In a couple of cases, we have wanted to verify that the numbers coming from the partners were accurate.  In one case, we were told by a partner that they found long after the fact they had been giving us incorrect numbers.

In another case, the partner asked us if we could supply the information to them because they were not set up to be able to capture and report on it themselves.

I couldn't say for sure what other engines should or shouldn't be added, nor at what stage/version it would be right to add them.  The only thing I can say with certainty is that we know we needed the data for the subset that the original spec called for.

Comment 12

[Richard Newman [:rnewman]]

So let me ask two concrete questions:

If I search through "Google SSL" (which we don't ship, but is a partner), should it be reported as Google, as "Google SSL", or as "Other"?

If I search through "Amazon.co.uk" (which we ship with en-GB builds, and is a partner), should it be reported as "Amazon.com" (or "Amazon", or whatever central identifier), as "Amazon.co.uk", or as "Other"?

Feel free to redirect this question to someone else if you don't feel like it's your call.

Comment 13

[Daniel Einspanjer [:dre] [:deinspanjer]]

I can't say with any certainty.  We need to try to loop someone from bizdev in.

Comment 14

[Richard Newman [:rnewman]]

I have reached out to bizdev.

Comment 15

[Richard Newman [:rnewman]]

OK, here's the direction we plan to go: we'll report granular search providers individually, but only those with whom we have a partner agreement.

Joanne will be giving me that list, and I'll be figuring out how to integrate that filtering in a way that's least painful.

To ward off any privacy concerns…

The user value here is, as I see it, twofold:

* The community value accorded by improving our shipped set of partner providers, and tuning search UI. Same as for other Mozilla-centric value-adds in FHR.

* The ability to distinguish between different providers when making health recommendations (e.g., "maybe you should be using Google UK SSL", or "we stopped partnering with Joe's Evil Search Site in the UK, because they broke our privacy rules, but you're still using them; you should consider switching".

I also don't think there's a significant privacy cost to being granular here: there is likely already a strong correlation between the locale that we report (en_GB) and the search providers that a user uses (Google UK, Amazon UK). Coalescing providers would harm our ability to draw useful conclusions without significantly limiting our ability to make educated guesses about individual users, which is a poor tradeoff.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Part 1, WIP.

browser.js change: gives us access to the other search engine fields, not just the name.

Untested.

Comment 17

[Richard Newman [:rnewman]]

Attachment: Part 2: WIP.

Most of the way there, I think.

This allows us to dump whatever's in the DB, and so the search count measurement only records fields for the current default engines. It intersects those with our defined 61 partner engines; and any other search goes in the "other" bucket.

Untested; I have only verified that this builds, not even passes existing tests.

I haven't restored SearchCountsProvider1 yet. Just putting this here as a WIP in case I get hit by a bus tomorrow.

Note also that this includes synchronous access to the search service right there in FHR's provider initialization. This might be a problem, in which case we will have to preprocess this file by parsing the locale-appropriate searchplugins/list.txt, or somehow delay field creation to immediately prior to recording a search.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 727076 [details] [diff] [review]
Part 2: WIP.

>diff --git a/services/healthreport/providers.jsm b/services/healthreport/providers.jsm

>+  engineID: function (engine) {
>+    let file = engine.wrappedJSObject._file;
>+    let leaf = file.leafName;
>+
>+    return leaf.substring(0, leaf.lastIndexOf("."));

ew :(

Setting up some kind of ID outside of the search service (that relies on some of its implementation details!) doesn't seem like a great idea. If you need to expose a per-locale ID of some sort, let's do that more explicitly, in the search service itself.

Comment 19

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #18)

> ew :(
> 
> Setting up some kind of ID outside of the search service (that relies on
> some of its implementation details!) doesn't seem like a great idea. If you
> need to expose a per-locale ID of some sort, let's do that more explicitly,
> in the search service itself.

Ew indeed, but this is something we want to ship in 21.

As I understand it, doing this "right" -- wiring the list.txt entry (the filename sans extension, which is the unique identifier) into each engine entry in a stable and supported way -- would involve a moderate amount of hackery to the search service and nsISearchEngine, maybe the search engine cache JSON format, and perhaps includes IDL changes (to expose the new property in a way that doesn't require wrappedJSObject).

I'd be less confident about getting Aurora (or even Beta) approval for that change.

Could you outline which broad changes to the search system you'd r+, and which you think are important enough to argue for uplift?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Couldn't we instead just use the engine name+build locale(+default engine status) to identify engines, rather than basing it on the engine filename?

Comment 21

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #20)
> Couldn't we instead just use the engine name+build locale(+default engine
> status) to identify engines, rather than basing it on the engine filename?

That's closer than just using the name, so thanks for the excellent suggestion! Alas, it's imperfect for two reasons:

* When the same engine appears in multiple locales, we'd like it to be the same identifier. That is, we don't want French users of Amazon.co.uk to be counted separately from UK users, unless we explicitly query for that in FHR queries (which we can do, because FHR reports locale).

* These values will be used in queries and appear in reports. The shorter and more stable the identifier, the better.

We might have another way out; stay tuned.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

As long as we can identify the specific cases we care about on the client, we can have it report them however we'd like (e.g. by mapping all Amazon locale variants to report as just "Amazon").

Comment 23

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #22)
> As long as we can identify the specific cases we care about on the client,
> we can have it report them however we'd like (e.g. by mapping all Amazon
> locale variants to report as just "Amazon").

Figuring that out is why I roped in Joanne. We'd like to report each unique search provider separately, rather than rolling them up. (And rolling them up *well* would be a pain regardless.)

The constraints are:

* Different search providers have the same name, but should be reported as different. (E.g., eBay-de and eBay (US), both of which are named "eBay").

* The same search provider can exist unchanged in multiple locales, and should be reported as the same.

* The same search provider (by our internal conception) might have a different name in different locales, but again should be reported as the same. (No example to hand, but they could.)


That's why I converged on using the list.txt entry: they're fastidiously done, seem to be globally scoped, and I can get to them.


It now doesn't look like this work will be aiming for uplift. Given that change in scope, do you object to me wending the on-disk provider identifier through the search service?

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #23)
> * Different search providers have the same name, but should be reported as
> different. (E.g., eBay-de and eBay (US), both of which are named "eBay").
> 
> * The same search provider can exist unchanged in multiple locales, and
> should be reported as the same.

Reporting name+engine URL should satisfy these constraints.

> * The same search provider (by our internal conception) might have a
> different name in different locales, but again should be reported as the
> same. (No example to hand, but they could.)

If there are no examples at hand, I claim this isn't a v1 constraint.

Comment 25

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)

> Reporting name+engine URL should satisfy these constraints.

Unless the URL is subtly different between different locales (due to outdated searchplugin XML files, for example) in a way that we don't care about from an analytics standpoint.

Also, I don't think it's realistic to report engine URLs. It breaks longitudinal correctness (every time we tweak a search param, it becomes a new engine), and it will inflate payloads -- each engine name is reported up to four unique times for 180 days. Compression helps, but that doesn't help prior to the wire.


All of this comes down to "there is a conception of 'same' -- both between locales and across time -- that is not present in the code".


Perhaps let's come at this from a different direction. For bizint reasons, I would like to attach an arbitrary partner identifier to 61 of the searchplugin files in the l10n repos, and report that partner identifier in FHR search counts. That partner identifier isn't the ShortName, and it isn't the URL. It can be the filename, but doesn't have to be.

How can I make that happen?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #25)
> Perhaps let's come at this from a different direction. For bizint reasons, I
> would like to attach an arbitrary partner identifier to 61 of the
> searchplugin files in the l10n repos, and report that partner identifier in
> FHR search counts. That partner identifier isn't the ShortName, and it isn't
> the URL. It can be the filename, but doesn't have to be.
> 
> How can I make that happen?

Fair enough. Filename doesn't work very well (it's not clear that it has semantic value since it hasn't in the past; localizers can mis-manage it). Adding an attribute to our "MozSearch" format and exposing it via nsISearchEngine should be relatively straightforward, let me know if you want to discuss further (or if you discover otherwise).

Comment 27

[Axel Hecht [:Pike]]

fwiw, on the l10n drivers side, we're badly flooded. Managing updating several dozen searchplugins will take time, and isn't going to be high on our list of things to do.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

We can commit the changes ourselves, right? But I understand that's still a pain. Given the pain involved, it still feels to me like we should push back on the requirements.

Comment 29

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)
> We can commit the changes ourselves, right? But I understand that's still a
> pain. Given the pain involved, it still feels to me like we should push back
> on the requirements.

That's actually why I experimented to see if I could get the filename, because these look close to perfect to me:

http://rnewman.pastebin.mozilla.org/2234160

These are actually the values I would add to the XML files, because they're remarkably clean.

I'm happy to go either way, because I don't consider either of these painful: either exposing the filename through the API, or adding a partner identifier to the file contents for this set of providers and committing it myself. Both seem straightforward if you don't mind me extending the interfaces, and the latter is trivially scriptable.

Comment 30

[Richard Newman [:rnewman]]

It turns out that the search engine code *already* uses the filename as an internal unique identifier.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2314

I'm working on a patch now to simply factor out that identifier code into something a little more reusable, and expose it as an attribute on nsISearchEngine.

Comment 31

[Richard Newman [:rnewman]]

Attachment: Part 1: extend nsISearchEngine to include identifier. v1

This has a simple test, but not total coverage.

I believe I preserved the existing behavior of _id.

Comment 32

[Richard Newman [:rnewman]]

Attachment: Part 2: alter browser.js to pass the whole search engine to FHR, not just the name.

Not much to it. Half the lines are cleanup.

Comment 33

[Richard Newman [:rnewman]]

Attachment: Part 3: alter search counts provider to record all of Mozilla's partner engines in each locale.

This needs a little more fleshing out, so just feedback for now.

Comment 34

[Richard Newman [:rnewman]]

Those three patches seem happy on the small try push I submitted:

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

Comment 35

[Richard Newman [:rnewman]]

Actually, scratch part 2. It's not necessary. Updated patches this weekend.

Comment 36

[Richard Newman [:rnewman]]

Attachment: Part 1: extend nsISearchEngine to include identifier. v2

Not sure if this changed, but uploading the current version nonetheless.

Comment 37

[Richard Newman [:rnewman]]

Attachment: Part 2: alter search counts provider to record all of Mozilla's partner engines in each locale. v2

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Attachment: tweaked part 1

Rather than submitting review comments, I just applied the patch and made some adjustments:
- make the "identifier" getter assume "app-provided" engines have leafNames (AFAICT this is always true - your patch handled getLeafName returning null when it never should)
- getLeafName -> _getLeafName (not a "public" API)
- refactored the _id getter to be a bit easier to read. Given my understanding of the invariants it should still be correct, but would appreciate a double-check.
- adjusted the test to avoid the "let's pretend" hack, and actually load an engine from a JAR in addition to from the profile.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 729094 [details] [diff] [review]
tweaked part 1

>diff --git a/toolkit/components/search/tests/xpcshell/test_identifiers.js b/toolkit/components/search/tests/xpcshell/test_identifiers.js

>+    // Let's pretend that we shipped it. Set the install location and
>+    // blow away the cached identifier.
>+    do_check_eq(jarEngine.identifier, "bug645970");

This comment needs updating!

Comment 40

[Richard Newman [:rnewman]]

Comment on attachment 729094 [details] [diff] [review]
tweaked part 1

LGTM, and tests pass. Thanks!

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 729087 [details] [diff] [review]
Part 2: alter search counts provider to record all of Mozilla's partner engines in each locale. v2

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

Looks good. Cancelling review until we have test coverage of shouldIncludeField for unknown search engines.

::: services/healthreport/providers.jsm
@@ +1139,5 @@
> +  },
> +
> +  _initialize: function () {
> +    // Don't create all of these for every profile.
> +    // There are 61 partner engines, translating to 244 fields. 

Nit: TWS

@@ +1173,5 @@
> +    if (!this._fieldSpecs) {
> +      this._initialize();
> +    }
> +    return this._fieldSpecs;
> +  },

And we're back to non-static field declarations. Boo.

@@ +1309,5 @@
> +   * data.
> +   */
> +  shouldIncludeField: function (field) {
> +    return true;
> +  },

Shouldn't this be on the measurement? This highlights the lack of test coverage!

::: services/metrics/dataprovider.jsm
@@ +336,5 @@
> +   * Allow subclasses to change this behavior, e.g., to implement a dynamic
> +   * fieldset.
> +   *
> +   * @return (boolean) true if the specified field should be included in
> +   *                   payload output.

Please document that this only applies to the default serializers.

Comment 42

[Richard Newman [:rnewman]]

Attachment: Part 2: alter search counts provider to record all of Mozilla's partner engines in each locale. v3

Nits addressed, and we now are able to serialize types we don't know about (by assuming that they're counters).

Two tests added.

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 729240 [details] [diff] [review]
Part 2: alter search counts provider to record all of Mozilla's partner engines in each locale. v3

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

Yay tests!

::: services/healthreport/tests/xpcshell/test_provider_searches.js
@@ +91,5 @@
>    yield storage.close();
>  });
>  
> +add_task(function test_includes_other_fields() {
> +  let storage = yield Metrics.Storage("record");

You should use a test-specific name here.

@@ +105,5 @@
> +  let testField = "test.searchbar";
> +  let now = new Date();
> +  yield provider.enqueueStorageOperation(function record() {
> +    return m.storage.incrementDailyCounterFromFieldID(id, now);
> +  });

Nit: The enqueue shouldn't be necessary in these controlled conditions.

@@ +112,5 @@
> +  do_check_false(testField in m.fields);
> +
> +  // But we want to include it in payloads.
> +  do_check_true(m.shouldIncludeField(testField));
> +  

Nit: TWS

Comment 44

[Richard Newman [:rnewman]]

Thanks for the speedy reviews, chaps!

https://hg.mozilla.org/services/services-central/rev/7e2f49fb5f4c
https://hg.mozilla.org/services/services-central/rev/73dd524468a1

Comment 45

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/7e2f49fb5f4c
https://hg.mozilla.org/mozilla-central/rev/73dd524468a1

Comment 46

[Gregory Szorc [:gps]]

Should we uplift this to 21?

Comment 47

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #46)
> Should we uplift this to 21?

I would *like* to, because it makes our search metrics infinitely more accurate, but it's a UUID change to nsISearchEngine. I don't have enough information on how we feel about uplifting that kind of change very late in the Aurora cycle, and I'm not inclined to push if it's unusual.

Gavin?

Comment 48

[:Gavin Sharp [email: [e-mail]]]

I don't know of any external native-code users of nsISearchEngine, and this is a purely additive change, so even if there were this shouldn't result in anything bad happening. And interface changes up to beta 1 should generally be tolerable. So no objection here.

Comment 49

[Richard Newman [:rnewman]]

Comment on attachment 729094 [details] [diff] [review]
tweaked part 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  828540

User impact if declined: 
  Not yet user facing; limitation on data collection in FHR. Non-US search providers will all be considered "other".

Testing completed (on m-c, etc.): 
  Has been on m-c for a little while; still need to do some analysis of submitted payloads to verify.

Risk to taking this patch (and alternatives if risky): 
  Slim.

String or UUID changes made by this patch: 
  One UUID change; see Gavin's last comment.

Comment 50

[Richard Newman [:rnewman]]

Comment on attachment 729240 [details] [diff] [review]
Part 2: alter search counts provider to record all of Mozilla's partner engines in each locale. v3

Second half of approval flag.

Comment 51

[bhavana bajaj [:bajaj]]

(In reply to Richard Newman [:rnewman] from comment #49)
> Comment on attachment 729094 [details] [diff] [review]
> tweaked part 1
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
>   828540
> 
> User impact if declined: 
>   Not yet user facing; limitation on data collection in FHR. Non-US search
> providers will all be considered "other".
> 
> Testing completed (on m-c, etc.): 
>   Has been on m-c for a little while; still need to do some analysis of
> submitted payloads to verify.
> 
> Risk to taking this patch (and alternatives if risky): 
>   Slim.
> 
> String or UUID changes made by this patch: 
>   One UUID change; see Gavin's last comment.

For any IDL changes as long as the IID is revved with the corresponding interface we should be fine until Aurora.

This looks good to land as long as the risk is contained to FHR and it will be helpful to report a brief metrics search analysis data you are working on here as a proof of concept . Thanks !

Comment 52

[:Gavin Sharp [email: [e-mail]]]

(In reply to bhavana bajaj [:bajaj] from comment #51)
> This looks good to land as long as the risk is contained to FHR

To be clear, the risk isn't contained to FHR - this could break the search service, too. That seems relatively unlikely, but some targeted QA testing around search settings being properly maintained across updates with/without the patch would be a good idea.

Comment 53

[bhavana bajaj [:bajaj]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #52)
> (In reply to bhavana bajaj [:bajaj] from comment #51)
> > This looks good to land as long as the risk is contained to FHR
> 
> To be clear, the risk isn't contained to FHR - this could break the search
> service, too. That seems relatively unlikely, but some targeted QA testing
> around search settings being properly maintained across updates with/without
> the patch would be a good idea.

err,sorry about misunderstanding the risk here & thanks for pointing that out Gavin :)

I also spoke offline to Gavin about this & he will be working with rnewman to get test coverage on this.CCing ashughes here as well as gavin may have some pointers around testing this for QA .

Comment 54

[Richard Newman [:rnewman]]

(In reply to bhavana bajaj [:bajaj] from comment #53)
 
> err,sorry about misunderstanding the risk here & thanks for pointing that
> out Gavin :)

No big deal -- I should have left a better comment than "Slim" :D

Will work with Gavin to make sure any risk is addressed.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc077ab0cebf
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6d8dfa7859d

Comment 56

[u279076]

I've done some testing with the Nightly builds from before and after this landed on mozilla-central with some different search settings, search engines, and locales and did not encounter any problems. Since this just landed today on Aurora I will wait a couple days to test this on Aurora (probably Monday).

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Backed out of Aurora for mochitest browser-chrome failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc9bc44e7f1c

https://tbpl.mozilla.org/php/getParsedLog.php?id=21210866&tree=Mozilla-Aurora

TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_aboutHome.js | Searches provider is available.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_aboutHome.js | Have data for today.
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_aboutHome.js :: onValues :: line 143
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/HealthReport.jsm :: onSuccess :: line 2991
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/HealthReport.jsm :: onSuccess :: line 2917
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 220
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 217
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/Sqlite.jsm :: onResult :: line 494
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: effort :: line 55
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolved :: line 117
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 37
    JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 143
    JS frame :: resource://gre/modules/Sqlite.jsm :: <TOP_LEVEL> :: line 772
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_aboutHome.js | Test timed out

Comment 58

[Richard Newman [:rnewman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #57)
> Backed out of Aurora for mochitest browser-chrome failures.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/dc9bc44e7f1c
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21210866&tree=Mozilla-Aurora

I think we're finally being bitten by this:

    // We rely on the listener in browser.js being installed and fired before
    // this one. If this ever changes, we should add an executeSoon() or similar.
    doc.addEventListener("AboutHomeSearchEvent", function onSearch(e) {

The search provider hasn't recorded the search by the time this test event listener fires.

Unfortunately, this test doesn't include any FHR logging, so I can't see what's actually happening.

Comment 59

[Gregory Szorc [:gps]]

Attachment: Log from failed test run

FHR logs on a hung test.

Comment 60

[Richard Newman [:rnewman]]

Attachment: Part 3: fix browser/base/content/test/browser_aboutHome.js. v1

I don't know why this test isn't failing on s-c, because it's obviously wrong. Here is the fix that makes things work on Aurora, and doesn't cause problems on s-c.

Comment 61

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/6d884c040870

I will land this flattened on Aurora.

Comment 62

[Richard Newman [:rnewman]]

Relanded:

   https://hg.mozilla.org/releases/mozilla-aurora/rev/d132b29c48cc
   https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca09c91ac7b

Comment 63

[Richard Newman [:rnewman]]

I'd like to thank Mozilla-Aurora for finding all of our broken mochitests, and RyanVM for his patience. I'd also like to thank the Academy…

https://hg.mozilla.org/releases/mozilla-aurora/rev/2174a373f5d6

I'd really like to know why these don't fail on s-c.

Comment 64

[Richard Newman [:rnewman]]

Keep whackin' dem moles.

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe2e51f95fdf

I think I've got them all now…

Comment 65

[u279076]

I just finished repeating the testing I did on Nightly in comment 56 on Aurora and everything seems to be working as expected here. Marking verified fixed.

Comment 66

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6d884c040870
https://hg.mozilla.org/mozilla-central/rev/663d6eb217ee
https://hg.mozilla.org/mozilla-central/rev/a51324f77edb

Comment 67

[Richard Newman [:rnewman]]

Relanded on Aurora, because m-c was merged before they landed there.

   https://hg.mozilla.org/releases/mozilla-aurora/rev/d153713f6cb9
   https://hg.mozilla.org/releases/mozilla-aurora/rev/5b8509e9a01f
   https://hg.mozilla.org/releases/mozilla-aurora/rev/6981e60ef18e

Comment 68

[Richard Newman [:rnewman]]

And somehow this test file ended up with duplicate lines after the merge.

https://hg.mozilla.org/releases/mozilla-aurora/rev/59a419eca635