1261930 - Force a list update when the Safe Browsing *.enabled prefs are toggled

Status: NEW

(Toolkit :: Safe Browsing, defect, P5)

Description

[François Marier [:francois]]

Right now, if we enable a pref like `browser.safebrowsing.blockedURIs.enabled`, we have to work until the next list update before the list is downloaded locally and ready to be used.

We already have watches on these *.enabled prefs (i.e. browser.safebrowsing.enabled, browser.safebrowsing.malware.enabled, privacy.trackingprotection.enabled, privacy.trackingprotection.pbmode.enabled, browser.safebrowsing.forbiddenURIs.enabled, browser.safebrowsing.blockedURIs.enabled) and so if we reset the "nextupdatetime" for the relevant providers to 1 and trigger an update, the relevant features will be enabled sooner.

Also related to bug 1181617.

Comment 1

[Henry Chang [:hchang]]

Steal from dimi.

Comment 2

[Henry Chang [:hchang]]

Attachment: Patch: Reset nextupdatetime if any of the safebrowsing features is enabled.

Comment 3

[Henry Chang [:hchang]]

Attachment: Patch: Reset nextupdatetime if any of the safebrowsing features is enabled.

Comment 4

[Henry Chang [:hchang]]

Comment on attachment 8745936 [details] [diff] [review]
Patch: Reset nextupdatetime if any of the safebrowsing features is enabled.

Hi Francois,

Could you help review this patch, which checks if any of the safebrowsing features is just enabled and then reset "nextupdatetime" to trigger a list download immediately?

Thanks!

Comment 5

[François Marier [:francois]]

Comment on attachment 8745936 [details] [diff] [review]
Patch: Reset nextupdatetime if any of the safebrowsing features is enabled.

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

(In reply to Henry Chang [:henry] from comment #4)
> Could you help review this patch, which checks if any of the safebrowsing
> features is just enabled and then reset "nextupdatetime" to trigger a list
> download immediately?

Thanks for that patch Henry, that's pretty much exactly what I suggested in the bug description. Re-reading what I proposed though, one thing that comes to mind is that this would violate the spec and potentially send too much traffic to the Google servers. So we'd probably need to restrict this to the "mozilla" provider.

Thinking a bit more about it though, this would give users to ability to generate a lot of traffic to our shavar server simply by checking and un-checking a UI setting repeatedly. It would be nice to have the features work immediately after they are enabled, but it is ignoring what the server asked us to do.

Comment 6

[François Marier [:francois]]

gcp, can you think of a way to do this without being too mean to the server?

I would assume that whatever we do will need to be restricted to the "mozilla" provider.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

So the underlying problem is that we want to fetch some new lists that we're now updating from a certain provider, but our structure makes it so updates will request updates for all the lists (including those previously enabled). This means there's a nextupdatetime already in play and we have data, but not the complete data we need.

I would propose to safeguard the list providers with a sanity check on lastupdatetime vs nextupdatetime. If there's no nextupdatetime and we want to update, but we do have a lastupdatetime, check whether we're too close to lastupdatetime, and if so clamp the next update to some minimum value and reschedule.

IIRC Google's server typically schedules 20 minute minimal delays. So let's arbitrarily make the minimum distance 10 minutes.

Comment 8

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> I would propose to safeguard the list providers with a sanity check on
> lastupdatetime vs nextupdatetime. If there's no nextupdatetime and we want
> to update, but we do have a lastupdatetime, check whether we're too close to
> lastupdatetime, and if so clamp the next update to some minimum value and
> reschedule.

Doing that will break the hack (nextupdatetime=1) we are currently using to force list updates in a few places, for example the end-to-end tests.

> IIRC Google's server typically schedules 20 minute minimal delays. So let's
> arbitrarily make the minimum distance 10 minutes.

I'm not sure about this solution. It would violate the wishes of the server and it wouldn't get us immediate updates (the original motivation for opening this bug).

Having to wait 10 min v. 20 min for the feature to work doesn't feel like a meaningful improvement (compared to waiting for 2 sec v. 20 min).

Comment 9

[Henry Chang [:hchang]]

Introducing a new flag to keep track of the last update time triggered by toggling the features might work, but it seems a little over-engineering and bug-prone :(

if (oldFeatureSet !== newFeatureSet &&
    some features are enabled /* I forgot to do this check int the patch */ &&
    lastupdatetime_trigger_by_toggle is not too close) {
  nextupdatetime = 1;
  lastupdatetime_trigger_by_toggle = now;
}

Comment 10

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #8)
> Doing that will break the hack (nextupdatetime=1) we are currently using to
> force list updates in a few places, for example the end-to-end tests.

Those could clear lastupdatetime?
 
> > IIRC Google's server typically schedules 20 minute minimal delays. So let's
> > arbitrarily make the minimum distance 10 minutes.
> 
> I'm not sure about this solution. It would violate the wishes of the server

We can't avoid that unless we split lastupdate/nextupdate per list, and that conflicts with the protocol/requires an overhaul of the entirety of listmanager.

> and it wouldn't get us immediate updates (the original motivation for
> opening this bug).
> 
> Having to wait 10 min v. 20 min for the feature to work doesn't feel like a
> meaningful improvement (compared to waiting for 2 sec v. 20 min).

Ugh right, if you had an automatic update 5 minutes before then you'd still see nonfunctional features for the next 5 minutes.

I think we'll have to:
a) Limit it to our own lists.
b) Maybe just make sure the dialog doesn't "commit" the feature changes until the user clicks OK/closes it, so playing around with the settings doesn't hammer the server. If we don't want to rely/fiddle with the UX code for this then:

>Introducing a new flag to keep track of the last update time triggered by toggling the features might work, but it seems a little over-engineering and bug-prone

You can write a pref with the last forced update time and use that to throttle to once every 15 seconds or so. This would stop "accidental abuse" and still respond relatively quickly (instant for the first settings change, 15 seconds minus time spent clicking things, for subsequent ones).

If the pref you write out is a factual statement of what you did and when, I think it's going to be OK in terms of over-engineering and bug-prone-ness :-)

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Just to be clear, I don't think we should throttle on lastupdatetime<=>nextupdatetime distance as I proposed in comment 7, for the reasons Francois pointed out. I'm proposing to write a out forcedupdatetime pref, and if another forced update arrives before 15 seconds, to put the "clear nextupdatetime" action on a cancellable timer 15-x seconds in the future (after potentially canceling an existing outstanding timer).

Comment 12

[François Marier [:francois]]

This has turned into a much more complicated fix than I thought and is likely to make our update code even more complicated/brittle.

Given that we've been able to live with the current behaviour so far, I'm starting to think we should punt on this until we have an actual request from product.