1830041 - Nimbus integration with Search Service should have protection for invalid values

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Mark Banner (:standard8)]

I set up a test Nimbus experiment with incorrect extra parameter values. These caused the code to throw and stop initialisation of the search service.

Due to the level of QA that we have I think it would be unlikely to happen in reality. However, I would still be concerned that something could get changed in newer versions of Firefox, and the impacts not realised for older versions.

I think we should have protections in place so that a broken Nimbus experiment cannot stop initialisation/use of the search service.

We should review all Nimbus points, to make sure that we handle exceptions appropriately, and maybe see if there's a way to report back via Nimbus about the experiment not being applied (maybe unenroll with a flag?)

The incorrect branch value was:

{
  "extraParams": {
      "google_channel_row" : "bar"
    }
  ]
}

This should have been:

{
  "extraParams": [
    {
      "key": "google_channel_row",
      "value": "bar"
    }
  ]
}

This caused this function to throw, which then stopped the search service initialising.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1830041 - Remove unnecessary setting of search url params in search tests. r?#search-reviewers!

These are mainly previous copy/paste of items that are not needed for the specific tests. Params tests are covered in other tests.

Depends on D180965

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1830041 - Rename test_pref.js and test_purpose.js to provide a better indication of which parts of search they are testing. r?#search-reviewers!

Depends on D181067

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1830041 - Change skipping of test_getSubmission_params_pref.js for nightly builds. r?#search-reviewers!

This avoids skipping the whole test and provides a better indication of why it is skipped.

Depends on D181068

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1830041 - Clean up the test_getSubmission_* tests. r?#search-reviewers!

Depends on D181069

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1830041 - Ensure that an incorrect structure received from Nimbus would not stop the search service starting up. r?#search-reviewers!

Depends on D181070

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09f429a2a3d9
Remove unnecessary setting of search url params in search tests. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/e96fa65a3ba3
Rename test_pref.js and test_purpose.js to provide a better indication of which parts of search they are testing. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/1e710f8aee6d
Change skipping of test_getSubmission_params_pref.js for nightly builds. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/f5d8c994b0ce
Clean up the test_getSubmission_* tests. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/163eab9d0e76
Ensure that an incorrect structure received from Nimbus would not stop the search service starting up. r=search-reviewers,jteow

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/09f429a2a3d9
https://hg.mozilla.org/mozilla-central/rev/e96fa65a3ba3
https://hg.mozilla.org/mozilla-central/rev/1e710f8aee6d
https://hg.mozilla.org/mozilla-central/rev/f5d8c994b0ce
https://hg.mozilla.org/mozilla-central/rev/163eab9d0e76