1296751 - Updating browser.safebrowsing.provider.*.(nextupdatetime|lastupdatetime) causes all Safe Browsing to be re-initialized

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

The prefs don't need to be re-read when we update the nextupdatetime or lastupdatetime just after a successful update.

This is probably causing unnecessary blocking disk I/O.

Comment 1

[François Marier [:francois]]

listmanager: 14:07:08 GMT-0700 (PDT): update success for goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar,goog-badbinurl-shavar from https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=51.0a&pver=2.2&key=[trimmed-google-api-key]: 1713
listmanager: 14:07:08 GMT-0700 (PDT): Waiting 1713000ms
listmanager: 14:07:08 GMT-0700 (PDT): Setting last update of google to 1471640828888
SafeBrowsing: 14:07:08 GMT-0700 (PDT): reading prefs
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.phishTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.malwareTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.downloadBlockTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.downloadAllowTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.trackingTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.trackingWhitelistTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): getLists: urlclassifier.blockedTable
SafeBrowsing: 14:07:08 GMT-0700 (PDT): initializing safe browsing URLs, client id navclient-auto-ffox

Comment 2

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1296751 - Skip observe browser.safebrowsing.provider.*.(nextupdatetime|lastupdatetime).

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8784276 [details]
Bug 1296751 - Skip observe browser.safebrowsing.provider.*.(nextupdatetime|lastupdatetime).

https://reviewboard.mozilla.org/r/73798/#review71856

That looks good to me.

Before landing, if you haven't done so already, please do a manual test.

Something like:

1. enable the debug output to the console
2. reset the `browser.safebrowsing.provider.mozilla.nextupdatetime` pref back to `1`
3. confirm this doesn't trigger reading the prefs again
4. update the tracking protection pref `urlclassifier.trackingTable` to add `content-track-digest256` to the list
5. confirm that the prefs are re-read as soon as you've modified that pref and that the `content-track-digest256` is downloaded/updated
6. after the list is updated, the `nextupdatetime` should be updated without triggering a re-reading of the prefs

Comment 4

[Thomas Nguyen (:tnguyen)]

Just run the test test_classifer_changetablepref.html and observed the result.
Thanks you for your review

Comment 5

[Thomas Nguyen (:tnguyen)]

> 4. update the tracking protection pref `urlclassifier.trackingTable` to add
> `content-track-digest256` to the list
> 5. confirm that the prefs are re-read as soon as you've modified that pref
> and that the `content-track-digest256` is downloaded/updated

The update is not kicked off because updateCheckers_ is not cleared. This ends with "No updates needed or already initialized ..."
Anyway, enable debug log in test_classifer_changetablepref.html and change the UPDATE_URL = "http://mochi.test:8888/chrome/toolkit/components/url-classifier/tests/mochitest/update.sjs";
I can observe it works
Thanks you

Comment 6

[Thomas Nguyen (:tnguyen)]

Waiting for try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6af492bb2ff

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa10bd34b8f7
Skip observe browser.safebrowsing.provider.*.(nextupdatetime|lastupdatetime). r=francois

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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