1698568 - Investigate why browser_urlbar_telemetry.js fails if the search engine template is changed

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P3)

Description

[Mark Banner (:standard8)]

In the patches on bug 1649186, I am changing how the search engine is installed.

If the search engine is installed with:

  await SearchTestUtils.installSearchExtension({
    name: "MozSearch",
    keyword: "mozalias",
    search_url: "https://example.com/"
  });

then the test fails (note the default for search_url_get_params is q={searchTerms}), but if it is installed with:

  await SearchTestUtils.installSearchExtension({
    name: "MozSearch",
    keyword: "mozalias",
    search_url: "https://example.com/?q={searchTerms}",
    search_url_get_params: "",
  });

Then the test passes. SearchEngine does treat these slightly differently, but I thought that the different was hidden away from other components.

We should investigate to understand why the test is failing, and if any code needs changing.

Comment 1

[James Teow [:jteow]]

I spent some time looking into this bug. I ran the tests with both the correct format and the current format that works but should be re-factored in order to know how the tests should behave.

It seems like in createEngineManifest, the data is encoded differently:

// When search_url is https://www.example.com and search_url_get_params is not provided:
manifest.chrome_settings_overrides.search_provider.search_url = "https://example.com/";
manifest.chrome_settings_overrides.search_provider.search_url_get_params = "?q={searchTerms}";

// When search_url is https://example.com/?q={searchTerms} and search_url_get_params is ""
manifest.chrome_settings_overrides.search_provider.search_url = "https://example.com/?q={searchTerms}";
manifest.chrome_settings_overrides.search_provider.search_url_get_params = "";

Likewise in _setUrls:

// this._urls with the failing url
this._urls =  [{params:[{name:"q", _value:"{searchTerms}", purpose:(void 0)}], rels:[], type:"text/html", method:"GET", _queryCharset:"UTF-8", template:"https://example.com/", templateHost:"example.com"}];

// this._urls with the test passing url
this._urls = [{params:[], rels:[], type:"text/html", method:"GET", _queryCharset:"UTF-8", template:"https://example.com/?q={searchTerms}", templateHost:"example.com"}];

I'm noticing when some of the tests break, some of the auto suggest entries are missing. I added a delay on one of the failing tests, test_oneOff_enterSelection()

On the working version, you should be able to write "query" in the URL bar and see two suggested urls. And then when using one-off search, you should also see two suggested urls.

However, in the version that's failing, when I type "query", it briefly shows both results up until I write the full word, at which point only one of the past search results. And in the one-off search, neither result are shown.

Also, the failing test is generating results with dupe params. getSubmission() can cause this:

var url = ParamSubstitution(this.template, searchTerms, engine); // returns https://example.com/?q=query
...
let dataString = dataArray.join("&"); // q=query
... 
if (dataString) {
  if (url.includes("?")) {
    url = `${url}&${dataString}`; // "https://example.com/?q=query&q=query"
  } else {
    url = `${url}?${dataString}`;
  }
}

Edit: The dupe "error" was just a mistake on my part! Sorry about that.

Comment 2

[Mark Banner (:standard8)]

Thank you for the analysis. One thing is confusing me though:

// this._urls with the failing url
this._urls =  [{params:[{name:"q", _value:"{searchTerms}", purpose:(void 0)}], rels:[], type:"text/html", method:"GET", _queryCharset:"UTF-8", template:"https://example.com/", templateHost:"example.com"}];

In this case, we have params but the template does not include the query...

var url = ParamSubstitution(this.template, searchTerms, engine); // returns https://example.com/?q=query
...
let dataString = dataArray.join("&"); // q=query
... 
if (dataString) {
  if (url.includes("?")) {
    url = `${url}&${dataString}`; // "https://example.com/?q=query&q=query"
  } else {
    url = `${url}?${dataString}`;
  }
}

... how is the template getting the query url?

I tried reproducing this with the Wolne Lektury engine which uses a similar format, and couldn't reproduce the duplicated params.

I'm therefore wondering if this is something to do with the test set-up, maybe how that engine is being installed?

If you want to try Wolne Lektury, the easiest way is to change this line to set locale to pl and then load your dev profile.

Comment 3

[James Teow [:jteow]]

Hi Mark,

Sorry about that, the so-called "dupe" error I saw was due to the way I had briefly modified the file. I'll be sure to be more diligent next time about recording errors with a clean build.

I updated the video showing test_oneOff_enterSelection() failing without the so-called "dupe error", as it's still breaking when the search_url is set to example.com.

As mentioned, it's a bit strange that typing quer will bring up both results, but query will result in only simple_query being suggested.

Comment 4

[Marco Bonardo [:mak]]

Hm so until you have typed "quer", the first generated result (heuristic) is a search for "quer" while the next history result is a search for "query".
Once you finish typing "query", the heuristic result and the first history result are pointing to the same search, thus UrlbarMuxerUnifiedComplete.jsm is deduping that result, by removing it.
Why is that not happening in the first case? I'm not sure, but maybe there's effectively an encoding difference for which the code in UrlbarMuxerUnifiedComplete.jsm doesn't think those urls are the same.
I'd suggest printing out the results at index 0 and 1 and compare them. Also debugging the dedupe part of UrlbarMuxerUnifiedComplete.jsm may be interesting.

I didn't remember this was assigned to me, I'm not actively working on it so feel free to take it.

Comment 5

[James Teow [:jteow]]

Thanks for the advice Marco, I think I've figured out why it's not happening in the first case and failing in the second case.

In SearchService.jsm, _buildParseSubmissionMap iterates through an array of engines, but in the case where search_url is https://example.com/?q={searchTerms} and search_url_get_params is "", attempting to add the search engine example.com to the _parseSubmissionMap fails because engine.getURLParsingInfo() returns null , skipping the loop.

_parseSubmissionMap is used by Services.search.parseSubmissionURL, which is used by one of the de-dupe checks in UrlbarMuxerUnifiedComplete.jsm:

let submission = Services.search.parseSubmissionURL(result.payload.url); // returns { ..., terms: "", engine: null } 
if (submission) {
  let resultQuery = submission.terms.trim().toLocaleLowerCase(); // ""
    if (state.suggestions.has(resultQuery)) { // evaluates to false
      let [newSerpURL] = UrlbarUtils.getSearchQueryUrl(submission.engine, submission.terms);
      if (UrlbarSearchUtils.serpsAreEquivalent(result.payload.url, newSerpURL)) {
        return false;
      }
    }
  }
}

Excerpt of parseSubmissionURL

Services.search.parseSubmissionURL(url) {
  ...
  // soughtKey = "example.com/"
  let mapEntry = this._parseSubmissionMap.get(soughtKey); // null
  if (!mapEntry) {
    return gEmptyParseSubmissionResult;
  }
}

As a sanity check, if I setup the tests with just https://example.com/, but prevent the engine from being added to this._parseSubmissionMap, the tests pass.

Comment 6

[Marco Bonardo [:mak]]

that seems unfortunate, parseSubmissionURL should not return a null engine I think?

Comment 7

[James Teow [:jteow]]

Just to add more info, one way the two installations differ is in the params property of EngineURL. The installation that adds its engine to the _parseSubmissionMap but fails due to the de-dupe code in the muxer, has EngineURL.params that look like this:

params: [{
  value: "{searchTerms}",
  name: "q",
}]

but if you provide a blank string in search_url_get_params, params is a blank array.

In the SearchService code, getURLParsingInfo ends up null, because url._getTermsParameterName() only checks this.params and doesn't look for params in this.template.

Comment 8

[Mark Banner (:standard8)]

I think parseSubmissionURL should be returning the same in both instances, and null is obviously the wrong thing here.

We should probably make sure that the SearchEngine/EngineURL objects all end up the same with both configurations.

Comment 9

[James Teow [:jteow]]

(In reply to Mark Banner (:standard8) from comment #8)

I think parseSubmissionURL should be returning the same in both instances, and null is obviously the wrong thing here.

We should probably make sure that the SearchEngine/EngineURL objects all end up the same with both configurations.

SearchTestUtils.createEngineManifest is respecting the blank search_url_get_params that's entered by the test:

    if (options.search_url_post_params) {
      manifest.chrome_settings_overrides.search_provider.search_url_post_params =
        options.search_url_post_params;
    } else {
      manifest.chrome_settings_overrides.search_provider.search_url_get_params =
        options.search_url_get_params ?? "?q={searchTerms}";
    }

Does it make more sense to modify the test utility such that it generates these manifests in a method that's more consistent with how they exist in the library? Because I'm assuming the library is very consistent in having manifests put params in search_url_get_params instead of combined with search_url. So in this case, a fix could be if search_url_get_params is blank but params are available in the search_url, remove the params from the search_url and place them into manifest.chrome_settings_overrides.search_provider.search_url_get_params.

Or is it better to modify the library code with added logic to deal with these situations?

Comment 10

[Mark Banner (:standard8)]

(In reply to James Teow [:jteow] from comment #9)

Does it make more sense to modify the test utility such that it generates these manifests in a method that's more consistent with how they exist in the library? Because I'm assuming the library is very consistent in having manifests put params in search_url_get_params instead of combined with search_url. So in this case, a fix could be if search_url_get_params is blank but params are available in the search_url, remove the params from the search_url and place them into manifest.chrome_settings_overrides.search_provider.search_url_get_params.

Or is it better to modify the library code with added logic to deal with these situations?

We need to change SearchService/SearchEngine. Although almost all of our application provided search engines use search_url_get_params, however WebExtensions don't really know about that - the API only specifies search_url. We use both because of historical reasons and because it was easier to work with for the configuration.

Comment 11

[James Teow [:jteow]]

So it seems there's two problems here:

1. The way the manifest is structured can cause installations of SearchEngine object to be slightly different, even if the intentions are the same. Specifically, if you have query params in the search_url vs putting them into a search_url_get_params property.

2. Once the WebExtension installation is fixed, the tests will break because the Muxer is de-duping the results.

For problem 1, I can think of couple solutions for this:

1. If search_url_get_params is not provided and we are installing a non-app provided extension, we check search_url for the presence of query params. If they exist, either copy or move them into search_url_get_params. I suppose copy might be safer in case WebExtensions need the query params to remain in the search_url , but moving would prevent the possibilities of params being added to the URL again that I'm assuming could happen when query params are merged back into the url template to construct the search url. Regardless, modifications to search_url and search_url_get_params could be done in this.chrome_settings_overrides.processSearchProviderManifestEntry() closer to when the WebExtension manifest is processed, or in SearchService._createAndAddEngine when we're about to add an engine.

2. Alternatively, in EngineURL._getTermsParameterName(), if the search for searchTerms in the this.params array fails, extract params that could be in this.template and look for searchTerms there.

Comment 12

[Mark Banner (:standard8)]

I think we should end up with the same structure of data in SearchEngine regardless of if the query params are in search_url_get_params or search_url, i.e. the properties of SearchEngine should have the same values in both cases.

I think we should probably be handling it in SearchEngine._setUrls or maybe SearchEngine._getEngineURLFromMetaData. In these function we're handling the urls and params anyway, so I think it makes most sense to handle it there.

Comment 13

[Drew Willcoxon :adw]

James and I debugged this today. There are a few orthogonal issues here:

1. In addition to fixing the TODO, the proper fix for the test is to make its tasks independent of each other. The failing task (test_oneOff_enterSelection) depends on SERPs in history that were added by previous tasks. When those SERPs are present, the task passes. When they're not, it fails. The task only needs two results to be present so that it can select the second one and it doesn't seem to matter what kind of result it is. The task can accomplish that by adding a visit to an example.com URL, for instance, as part of its setup.
2. Services.search.parseSubmissionURL(historyUrl) returns a parse result without an engine when SearchTestUtils.installSearchExtension() is called with { search_url: "https://example.com/?q={searchTerms}", search_url_get_params: "" }, but it returns a result with an engine when SearchTestUtils.installSearchExtension() is called with { search_url: "https://example.com/" }. That seems like a bug to me, but what do you think Mark?
3. Currently, when UrlbarProviderPlaces restyles a SERP so that it becomes a search history result (a.k.a. a historical search suggestion or form history result), it does not add the restyled result if the hidden pref maxHistoricalSearchSuggestions is zero (source here). IMO the SERP should still be added as a regular history result. But not a big deal since it's a hidden pref that the vast majority of users don't modify.

I think we should use this bug to fix the test (point 1 above) and file separate bugs for the other two points (or only point 3, if point 2 isn't a bug).

More details I wrote before typing up the summary above:

A larger point is that this test is messy because tasks depend on state created by previous tasks. The failing task here depends on SERPs in history that were added by previous tasks. (When those SERPs are present, the task passes. When they're not, it fails.) That can be OK sometimes if the test is really complex, but this test isn't, and in general tasks should be independent of each other in part so that failures like this can be isolated during debugging. I'd like for us to clean this test up either as part of this bug or as a follow up.

The difference is that we return false on this line when the test installs the MozSearch engine in the following way:

  await SearchTestUtils.installSearchExtension({
    name: "MozSearch",
    keyword: "mozalias",
    search_url: "https://example.com/?q={searchTerms}",
    search_url_get_params: "",
  });

But, we do not return false at that point and instead we continue on in _maybeRestyleSearchMatch() and return a new match object when the engine is installed this way:

  await SearchTestUtils.installSearchExtension({
    name: "MozSearch",
    keyword: "mozalias",
    // `search_url` isn't actually necessary here since example.com is used as
    // the default
    search_url: "https://example.com/",
  });

So Services.search.parseSubmissionURL(historyUrl) returns a parse result without an engine using the first way but it returns a parse result with an engine using the second way. Seems like a bug in the search service to me.

_maybeRestyleSearchMatch() is called here in _addMatch(). When it returns false using the first install method, _addMatch() continues and adds the results. When it returns true using the second method, we hit that early return because the test also happens to set maxHistoricalSearchSuggestions to zero. So the results aren't added. The point of that early return is to prevent adding search history (a.k.a historical suggestions or form history), which restyled SERPs end up being, so it kind of makes sense, but IMO the SERPs should still be added as regular history URL results. That's a separate question though, and we should probably file another bug for it.

Comment 14

[Mark Banner (:standard8)]

(In reply to Drew Willcoxon :adw from comment #13)

2. Services.search.parseSubmissionURL(historyUrl) returns a parse result without an engine when SearchTestUtils.installSearchExtension() is called with { search_url: "https://example.com/?q={searchTerms}", search_url_get_params: "" }, but it returns a result with an engine when SearchTestUtils.installSearchExtension() is called with { search_url: "https://example.com/" }. That seems like a bug to me, but what do you think Mark?

Yes, this is a bug. As I mentioned in comment 12, the search engine object should end up with the same properties regardless of which one of the two versions is used - that would then result in Services.search.parseSubmissionURL(historyUrl) returning the same result.

Comment 15

[James Teow [:jteow]]

Attachment: WIP: - Bug 1698568 - Standardize query params handling in SearchEngine.jsm - r?standard8

The reason why browser_urlbar_telemetry.js was passing the tests was because the
search engine installation prevented it being added to the parseSubmissionMap in
SearchService.jsm. That prevented de-duping code in the Muxer to match the search term
with the constructed search url.

The tests I wrote in test_webextensions_install is checking that setting the get
parameters of a search extension will result in the same EngineURLs regardless
of whether the input came from within the search_url itself or it came from an explicit
property.

This will break the tests in browser_url_telemetry.js, so the adjustments I made:

• Allow a historical suggestion, or else no results show up in the one-off
  search
• Decrement the index for the expected autosuggest result because the Muxer will de-dupe
  one of the results in the list because "query" will match the url of the one the results.

Comment 18

[James Teow [:jteow]]

Just writing an additional note that this bug causes the showSearchTerms feature to not work for users who install and make default a search engine that doesn't provide its query params in search_url_get_params / searchGetUrlParams

Comment 19

[James Teow [:jteow]]

Attachment: Bug 1698568 - Allow engines to try retrieving terms param name from its template - r?standard8

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d260bcb443e
Allow engines to try retrieving terms param name from its template - r=Standard8

Comment 21

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/5d260bcb443e