1365877 - resetDatabase in SafeBrowsing test case should also reset cache

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Dimi Lee [:dimi]]

The purpose of classifierHelper.resetDB is to reset both database and cache.

It accomplishes this by applying an update to expire existing chunks. And since
applying an update will also clear all the caches(old design), both database and cache will be cleared.

But in Bug 1333328, we sync the caching mechanism for V2 and V4, now apply an update will not clear cache, it will only remove expired ones[1]

We should make sure resetDB will still clear the cache.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/tests/mochitest/classifierHelper.js#110

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug 1365877 - resetDatabase in SafeBrowsing test case should also reset cache.

There are two solutions to address this issue:
1. Update database and then reload database in classifierHelper.js
   Update database can reset database data and reload database ensures
   cache is empty.
2. During an update, remove cache entries which are not in the database.

I prefer taking the first one because if we implement the second
approach, an update will take longer since we need to check if each prefix
in the cache can also be found in the database. I think this is not necessary
because prefixes not in the database will eventually be removed when they
are expired.

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

Comment 2

[François Marier [:francois]]

Comment on attachment 8869294 [details]
Bug 1365877 - resetDatabase in SafeBrowsing test case should also reset cache.

https://reviewboard.mozilla.org/r/140852/#review145012

Patch looks great. I would suggest changing the commit message to capture the details you included in the bug description, as well as the extra information you told me in person. Maybe something along these lines:

"resetDatabase() is used to clear out the Safe Browsing database and cache in tests. Since bug 1333328 however we can no longer rely on updates clearing the cache.

In order to clear the cache after updating the database, resetDatabase() now calls another test-only function: reloadDatabase(). Since the cache is in memory, reloading the URL classifier will automatically clear the cache."

::: commit-message-baf05:1
(Diff revision 1)
> +Bug 1365877 - resetDB in SafeBrowsing test case should also reset cache. r?francois

It's now called `resetDatabase()` :)

Comment 3

[Dimi Lee [:dimi]]

Comment on attachment 8869294 [details]
Bug 1365877 - resetDatabase in SafeBrowsing test case should also reset cache.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8b6ab63c4c3
resetDatabase in SafeBrowsing test case should also reset cache. r=francois

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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