Closed Bug 1421500 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
Blocks: 1417762
Attachment #8935637 - Flags: review?(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?
Attachment #8935637 - Flags: review?(francois) → review-
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 on attachment 8935637 [details]
Bug 1421500 - Disable back-off for SafeBrowsing testcases not testing back-off.

https://reviewboard.mozilla.org/r/206522/#review213342
Attachment #8935637 - Flags: review?(francois) → review+
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2467ce87767e
Disable back-off for SafeBrowsing testcases not testing back-off. r=francois
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
Flags: needinfo?(dlee)
error fixed
Flags: needinfo?(dlee)
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e96a38ecdd86
Disable back-off for SafeBrowsing testcases not testing back-off. r=francois
https://hg.mozilla.org/mozilla-central/rev/e96a38ecdd86
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.