1175562 - Persist last update time for updates/gethash completion

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Currently, you can make Firefox update much faster than the protocol recommends, for example by shutting down and restarting Firefox immediately. This will also invalidate cached entries that were supposed to be valid for 45 minutes.

This is particularly on issue on Android where we are killed and restarted all the time.

We should persist the last update time past restarts.

Comment 1

[François Marier [:francois]]

BTW, we should also have a way to force updates. Currently we relying on this bug for our end-to-end tests. If we fix it without adding a way around it, it will turn our tests from something we can do in 10 minutes to something that takes half a day.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

We can just set the lastupdatetime pref to 0 for the test harnesses.

Comment 3

[François Marier [:francois]]

If all we need to do is set a pref to 0 and then restart Firefox, that sounds like a good work-around.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Persist last update time for SafeBrowsing

WIP

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Persist last update time for SafeBrowsing

Comment 6

[François Marier [:francois]]

Comment on attachment 8667853 [details] [diff] [review]
Persist last update time for SafeBrowsing

Review of attachment 8667853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Reading through this, it seems like we'll be able to set nextupdatetime to "1" if we want to force a list update on startup, right?

::: toolkit/components/url-classifier/content/listmanager.js
@@ +236,5 @@
> +      if (freshness) {
> +        Object.keys(this.tablesData).forEach(function(table) {
> +        if (this.tablesData[table].provider === provider) {
> +          this.dbService_.setLastUpdateTime(table, freshness);
> +        }}, this);

nit: there could also be an "else" clause here with a similar comment to what you've got below ("different providers for same update URL, wtf?!")

@@ +458,5 @@
> +
> +  // Store the last update time (needed to know if the table is "fresh")
> +  // and the next update time (to know when to update next).
> +  let lastUpdatePref = "browser.safebrowsing.provider." + provider + ".lastupdatetime";
> +  let targetTime = Date.now();

nit: I find the name confusing here. I would suggest using two variables to make the code easier to understand: "let now = Date.now();" here and then "let targetTime = now + delay;" later on.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #6)

> Looks good to me.
> 
> Reading through this, it seems like we'll be able to set nextupdatetime to
> "1" if we want to force a list update on startup, right?

Exactly.

I'll address the other comments.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6aea8b57e55

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Henrik, mozmill will have to take note of these new prefs. If you want an immediate update after startup, you need to set these prefs in the test harness - after this bug lands. (If not, there will be a delay of 0-5 minutes, similar to what we had when we first starting doing these)

browser.safebrowsing.provider.google.lastupdatetime=1
browser.safebrowsing.provider.google.nextupdatetime=1
browser.safebrowsing.provider.mozilla.lastupdatetime=1
browser.safebrowsing.provider.mozilla.nextupdatetime=1

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Gian-Carlo, so Mozmill is plainly dead. We are about to convert the tests over to Marionette. They will be part of our Firefox UI Tests (https://github.com/mozilla/firefox-ui-tests). Which tests would be affected by this? I assume all the safebrowsing tests we have?

Comment 11

[François Marier [:francois]]

I noticed that the SBv4 docs take the initial update delay from 0-5 min to 0-1 min. Maybe it's worth asking Google whether we can use 0-1 min in our v2.2 code too?

Waiting a full 5 minutes before being protected is not that great...

Comment 12

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #11)
> Waiting a full 5 minutes before being protected is not that great...

Note that this is only at the very first start on a new profile. Otherwise it can be as short as 3 seconds.

The spec wording isn't all that great in this area:

"
    The first request for data MUST happen at a random interval between 0 and 5 minutes after the browser starts.
    The second update request MUST happen at the update interval last specified by the server. If this value is unknown, the request MUST happen between 15 and 45 minutes later.
    After that, each update MUST happen at the update interval last specified by the server.
"

I've read this rather strictly in the new implementation in this bug, in the sense that "first, second, ..." refer to updates over the lifetime of the profile, not the browser. Considering them over the lifetime of the browser is not sensible on for example Android.

A fresh profile will have much large updates (fetching a large part of the DB) than an existing one (possibly tiny update), so this makes some sense.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Which tests would be affected by this? I assume all the safebrowsing tests we have?

All SafeBrowsing tests that check for updates against the "real" servers. Tests against www.itisatrap.org won't be affected.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> All SafeBrowsing tests that check for updates against the "real" servers.
> Tests against www.itisatrap.org won't be affected.

Given that we only use itisatrap.org for testing the safebrowsing stuff, we should be fine for now. But thank for letting me know.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Gian-Carlo, so Mozmill is plainly dead. 
...
> Given that we only use itisatrap.org for testing the safebrowsing stuff, we should be fine for now. 

So we no longer have any "real" tests for this feature? Because IIRC that was the point of Mozmill.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/988fdc8043e03c28301a45910c2c4076f58fd6f6
Bug 1175562 - Persist last update time for SafeBrowsing. r=francois

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The current tests for SafeBrowsing you can find under the following location which are for the notification and warning pages:

https://github.com/mozilla/firefox-ui-tests/tree/mozilla-central/firefox_ui_tests/functional/security

I can only remember that one other test which has been implemented in bug 1018161 and which we haven't ported over yet. How important would you rate that work? Also can you remember another test we have created based on your request? I don't find anything else.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

No, I indeed meant that test - which checks if the right databases are appearing once we're connected to the "real" Internet. (And which this bug would cause to take longer unless the above-mentioned prefs are set).

I wouldn't rate that more or less important than anything else, it just caught me completely unaware we no longer have this test coverage for SafeBrowsing. The Mozmill test was the only one that verified whether updates with the real servers actually work.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/988fdc8043e0

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sorry for the delay. I finally filed bug 1237396 to get the mozmill test converted.