Closed Bug 1463162 Opened 3 years ago Closed 3 years ago

Cache distributionID and move browser.search.order checks to distribution only

Categories

(Firefox :: Search, enhancement, P1)

Product:
Firefox
Component:
Search
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(2 files)

First pass
21.82 KB, patch
Details | Diff | Splinter Review
Bug 1463162 - Cache distributionID and move browser.search.order checks to distribution only.
59 bytes, text/x-review-board-request
florian
: review+
Details 
In bug 1461345, I moved search order to be in list.json.

I didn't make the browser.search.order prefs distribution only in that patch because I wanted to make some other changes as well.

This bug is for those changes.

1. distribution.id should use a lazy preference getter.
2. browser.search.order.* should only be allowed for distribution builds.
Attached patch First pass — — Splinter Review 
First pass.

It's failing on the tests that depend on isUS:

test_location_malformed_json
test_location_sync

We just need to kill these tests.
So I moved getIsUS to a new place in list.json parsing (which is probably where it belongs), so we can keep these tests a while longer...

This will also allow me to remove the geoSpecificPref code after this patch is in.
Comment on attachment 8986268 [details]
Bug 1463162 - Cache distributionID and move browser.search.order checks to distribution only.

https://reviewboard.mozilla.org/r/251642/#review258220

I like the direction this is going :-).

::: toolkit/components/search/nsSearchService.js:373
(Diff revision 1)
>  function isPartnerBuild() {
>    try {
> -    let distroID = Services.prefs.getCharPref("distribution.id");
> -
>      // Mozilla-provided builds (i.e. funnelcake) are not partner builds
>      if (distroID && !distroID.startsWith("mozilla")) {

I think you can now simplify this whole function to a oneliner:
  return distroID && !distroID.startsWith("mozilla");

::: toolkit/components/search/nsSearchService.js:3610
(Diff revision 1)
>        if (this.originalDefaultEngine) {
>          this.__sortedEngines.push(this.originalDefaultEngine);
>          addedEngines[this.originalDefaultEngine.name] = this.originalDefaultEngine;
>        }
>  
> +      if (distroID) {

Do we have funnelcakes that modify the order?

::: toolkit/components/search/nsSearchService.js:4169
(Diff revision 1)
>        // For privacy, we only collect the submission URL for default engines...
>        let sendSubmissionURL = engine._isDefault;
>  
>        // ... or engines sorted by default near the top of the list.
>        if (!sendSubmissionURL) {
> +        if (distroID) {

I'm not sure about this change. We were using the presence of the order prefs as smoke indicating the very high likelyhood of a hijacking attempt. I'm not sure the fact that we no longer process these prefs to change the order means we should stop collecting the submission url for these engines. Is this something you thought about, or just an added if block around every piece of code touching the order pref?

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js
(Diff revision 1)
>                 getDefaultEngineName(true), "expected US default search engine");
>  
>    Services.prefs.clearUserPref("browser.search.region");
>  });
> -
> -// Giving defaultenginename prefs a user value for partner build

Are these 3 tests removed because you are moving them to their own file?

::: toolkit/components/search/tests/xpcshell/xpcshell.ini:45
(Diff revision 1)
>  [test_init_async_multiple.js]
>  [test_init_async_multiple_then_sync.js]
>  [test_json_cache.js]
>  [test_list_json_locale.js]
>  [test_list_json_searchdefault.js]
> +[test_list_json_searchdefault_distro.js]

Forgot an hg add?
Attachment #8986268 - Flags: review?(florian) → review-
Comment on attachment 8986268 [details]
Bug 1463162 - Cache distributionID and move browser.search.order checks to distribution only.

https://reviewboard.mozilla.org/r/251642/#review258220

> Do we have funnelcakes that modify the order?

Possibly, but they would go down this case (because they have a distroID).

Are you saying we should be using is partner build?

> I'm not sure about this change. We were using the presence of the order prefs as smoke indicating the very high likelyhood of a hijacking attempt. I'm not sure the fact that we no longer process these prefs to change the order means we should stop collecting the submission url for these engines. Is this something you thought about, or just an added if block around every piece of code touching the order pref?

I mainly just put the distro ID stuff around, but I see where you are going. I'll change.

> Are these 3 tests removed because you are moving them to their own file?

Yes. Hence the missing file later.
(In reply to Mike Kaply [:mkaply] from comment #5)
> Comment on attachment 8986268 [details]
> Bug 1463162 - Cache distributionID and move browser.search.order checks to
> distribution only.
> 
> https://reviewboard.mozilla.org/r/251642/#review258220
> 
> > Do we have funnelcakes that modify the order?
> 
> Possibly, but they would go down this case (because they have a distroID).
> 
> Are you saying we should be using is partner build?

I just wanted to ensure choosing to test for distroID vs partner build was a conscious decision here.
> I just wanted to ensure choosing to test for distroID vs partner build was a conscious decision here.

Yes, conscious decision.

Sidenote.

I added this code when parsing List.json

    } else if (getIsUS()) {
      searchRegion = "US";
    }

because the tests assume that it is going to happen.

This will cause us to use the US search region in cases where we didn't before (although we don't store it in this new case, so it will always be requeried).

Do you think this is the right thing to do?
The more I think about it, the getISUS setting US is a bad idea. I'm going to have to figure out another way to make those tests work.
(In reply to Mike Kaply [:mkaply] from comment #7)

> I added this code when parsing List.json
> 
>     } else if (getIsUS()) {
>       searchRegion = "US";
>     }
> 
> because the tests assume that it is going to happen.
> 
> This will cause us to use the US search region in cases where we didn't
> before (although we don't store it in this new case, so it will always be
> requeried).
> 
> Do you think this is the right thing to do?

I can't point to something this would break, but I don't understand clearly which tests this is fixing. Should I expect a change of behavior for some tests when running on a machine that isn't in the US timezone?
Comment on attachment 8986268 [details]
Bug 1463162 - Cache distributionID and move browser.search.order checks to distribution only.

https://reviewboard.mozilla.org/r/251642/#review258548

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault_distro.js:1
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.

We would have a nicer hg history if this file was recorded as a copy rather than as a new file.
Attachment #8986268 - Flags: review?(florian) → review+
Blocks: 1460419
> I can't point to something this would break, but I don't understand clearly which tests this is fixing. Should I expect a change of behavior for some tests when running on a machine that isn't in the US timezone?

No. I'm going to be explit in the test that it should not check the timezone at all.

None of our code should be timezone dependent anymore.

I'll have someone double check though.
Comment on attachment 8986268 [details]
Bug 1463162 - Cache distributionID and move browser.search.order checks to distribution only.

https://reviewboard.mozilla.org/r/251642/#review258558

::: toolkit/components/search/tests/xpcshell/test_location_malformed_json.js:26
(Diff revision 3)
>    // Here we have malformed JSON
>    Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code"');
>    Services.search.init(() => {
>      ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
>      ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
> -    // fetch the engines - this should force the timezone check, but still
> +    // fetch the engines - this should not doesn't persist any prefs.

typo: "not doesn't"
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2ccfbc9fe616
Cache distributionID and move browser.search.order checks to distribution only. r=florian
https://hg.mozilla.org/mozilla-central/rev/2ccfbc9fe616
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.