Persist last update time for updates/gethash completion

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Blocks: 1149867
Blocks: 1155646
Assignee: nobody → gpascutto
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.
We can just set the lastupdatetime pref to 0 for the test harnesses.
If all we need to do is set a pref to 0 and then restart Firefox, that sounds like a good work-around.
Attachment #8667320 - Attachment is obsolete: true
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.
Attachment #8667853 - Flags: review?(francois) → review+
(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.
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
Flags: needinfo?(hskupin)
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?
Flags: needinfo?(hskupin)
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...
Flags: needinfo?(gpascutto)
(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.
Flags: needinfo?(gpascutto)
(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.
(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.
(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.
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.
Flags: needinfo?(gpascutto)
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.
Flags: needinfo?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/988fdc8043e0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1214448
Depends on: 1216604
Sorry for the delay. I finally filed bug 1237396 to get the mozmill test converted.
Depends on: 1274131
You need to log in before you can comment on or make changes to this bug.