1421500 - Disable back-off for SafeBrowsing testcases not testing back-off

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Dimi Lee [ooo 16.04 - 21.04］]

For testcases are not testing back-off, we should provide a way to disable it.
Otherwise, it could cause an error when running testcases multiple times.

Some of discussion around this bug is in Bug 1416987 Comment 19 - Comment 23.

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

After this patch landed, we will have 3 cases:
1. For providers are not "test", for example, google, google4, mozilla ... etc
   backoff CANNOT be disabled.
2. For "test" provider, if preference "browser.safebrowsing.provider.test.disableBackoff" is ON
   backoff is disabled.
3. For "test" provider, if preference "browser.safebrowsing.provider.test.disableBackoff" is Off
   backoff is NOT disabled.

So if your testcase will use listmanager or hashcompleter, you should try to use "test" provider
if possible, otherwise testcase may encounter intermittent failure due to backoff.

Review commit: https://reviewboard.mozilla.org/r/206522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206522/

Comment 2

[François Marier [:francois]]

Comment on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

https://reviewboard.mozilla.org/r/206522/#review212632

I agree with the approach. Just a few questions/suggestions inline.

::: toolkit/components/url-classifier/nsUrlClassifierLib.js:194
(Diff revision 1)
> -                requestPeriod /* request time, 60 min */,
> +                   requestPeriod /* request time, 60 min */,
> -              backoffInterval /* backoff interval, 60 min */,
> +                 backoffInterval /* backoff interval, 60 min */,
> -          24 * 60 * 60 * 1000 /* max backoff, 24hr */,
> +             24 * 60 * 60 * 1000 /* max backoff, 24hr */,
> -                         1000 /* tolerance of 1 sec */);
> +                            1000 /* tolerance of 1 sec */);
> +
> +  // For test provider, we will disable backoff if preference is set to false.

I don't understand why this block is in `RequestBackoffV4()` instead of `RequestBackoff()`.

It seems like we may want to disable backoff in V2 test cases too.

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter.js:243
(Diff revision 1)
>  // Expected highest completion set for which the server sends a response.
>  var expectedMaxServerCompletionSet = 0;
>  var maxServerCompletionSet = 0;
>  
>  function run_test() {
> +  // This testcase will test backoff

Suggestion to be more explicit:

    // This test case exercises the backoff functionality so we can't leave it disabled.

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter.js:398
(Diff revision 1)
>      }
>    },
>  };
>  
>  function finish() {
> +  Services.prefs.setBoolPref("browser.safebrowsing.provider.test.disableBackoff", true);

Is this required? Will if affect the next tests if we simply clear the user pref here instead?

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

https://reviewboard.mozilla.org/r/206522/#review212750

::: toolkit/components/url-classifier/nsUrlClassifierLib.js:194
(Diff revision 1)
> -                requestPeriod /* request time, 60 min */,
> +                   requestPeriod /* request time, 60 min */,
> -              backoffInterval /* backoff interval, 60 min */,
> +                 backoffInterval /* backoff interval, 60 min */,
> -          24 * 60 * 60 * 1000 /* max backoff, 24hr */,
> +             24 * 60 * 60 * 1000 /* max backoff, 24hr */,
> -                         1000 /* tolerance of 1 sec */);
> +                            1000 /* tolerance of 1 sec */);
> +
> +  // For test provider, we will disable backoff if preference is set to false.

We can put it in RequestBackoff.

The reason that I put this in RequestBackoffV4 is because both listmanager & hashcopmpler use RequestBackoffV4[1].
For me, RequestBackoff is more like base class right now, so I put the tricky thing in ReuqestBackoffV4 to make ReuqestBackoff more clean (we don't have to pass 'provider' to RequetsBackoff).

For unit test test "RequestBackoff" shouldn't have problem because this preference only affects tests that use listmanger to update or hashcompleter to trigger get hash.

I also agree with you that this should be common for v2 and v4, so we can put it in RequestBackoff.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#229

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter.js:398
(Diff revision 1)
>      }
>    },
>  };
>  
>  function finish() {
> +  Services.prefs.setBoolPref("browser.safebrowsing.provider.test.disableBackoff", true);

Yes, we can use Services.prefs.clearUserPref here.
I just forget we have this option, thanks for reminding :)

Comment 4

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206522/diff/1-2/

Comment 5

[François Marier [:francois]]

Comment on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

https://reviewboard.mozilla.org/r/206522/#review213342

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3608d94b8b7e142373ffb4b776f9af39814f47a5

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2467ce87767e
Disable back-off for SafeBrowsing testcases not testing back-off. r=francois

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out for ESLint failure  /toolkit/components/url-classifier/nsUrlClassifierLib.js:108

backout: https://hg.mozilla.org/integration/autoland/rev/8591409a1c109a9500614743bb336085d7becd8d

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2467ce87767ee4f34b97d7497bc5bf9d1d7e5e9b

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=151583549&repo=autoland&lineNumber=230

[task 2017-12-14T01:29:51.115Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/url-classifier/nsUrlClassifierLib.js:108:35 | Unexpected space before function parentheses. (space-before-function-paren)
231
[task 2017-12-14T01:29:51.115Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/url-classifier/nsUrlClassifierLib.js:113:6 | Missing semicolon. (semi)
232
[taskcluster 2017-12-14 01:29:51.313Z] === Task Finished ===
233
[taskcluster 2017-12-14 01:29:51.313Z] Unsuccessful task run with exit code: 1 completed in 707.02 seconds

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206522/diff/2-3/

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

error fixed

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e96a38ecdd86
Disable back-off for SafeBrowsing testcases not testing back-off. r=francois

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e96a38ecdd86