Closed Bug 1045163 Opened 6 years ago Closed 6 years ago

remove delay before updating existing tables on disk, only initialize updateCheckers if updates are needed

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently the listmanager waits 0-5 minutes before updating existing tables on disk. Why?

 for (var updateUrl in this.needsUpdate_) {
    // If the user has tables, add a fuzz of a few minutes.
    if (updatingExisting) {
      // Add a fuzz of 0-5 minutes.
      initialUpdateDelay += Math.floor(Math.random() * (5 * 60 * 1000));
      log("Waiting " + initialUpdateDelay / 1000 +
          " for updating existing table from " + updateUrl);
    }
Hey gcp, do you know the history of this check?
Flags: needinfo?(gpascutto)
No, I don't. I've always assumed it's historical to stop the servers from being swamped on new Firefox releases. Probably not much of an issue these days.

(It's also bad for mobile because the browser might not even have a lifetime that long.)
Flags: needinfo?(gpascutto)
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8468909 - Flags: review?(gpascutto)
Attachment #8468909 - Flags: review?(gpascutto) → review+
Attachment #8468909 - Attachment is obsolete: true
Summary: remove delay before updating existing tables on disk → remove delay before updating existing tables on disk, only initialize updateCheckers if updates are needed
Comment on attachment 8469400 [details] [diff] [review]
Remove delay before updating existing tables on disk (

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

Please re-review. I found a corner case which was hidden by the previous behavior (delay after flipping pref allowed enough time to enable/disable before the first update was kicked off).
Attachment #8469400 - Flags: review?(gpascutto)
Attachment #8469400 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/f5e5b0d07766
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment on attachment 8469400 [details] [diff] [review]
Remove delay before updating existing tables on disk (

Approval Request Comment
[Feature/regressing bug #]: bug 1021419
[User impact if declined]: In rare circumstances, enabling safebrowsing after being in a disabled state will fail to work until the next browser restart.
[Describe test coverage new/current, TBPL]: Manual
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8469400 - Flags: approval-mozilla-aurora?
Attachment #8469400 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Somehow we read over this part of the documentation:

https://developers.google.com/safe-browsing/developers_guide_v2?hl=en

Client Behavior:

    The first request for data MUST happen at a random interval between 0 and 5 minutes after the browser starts.

I will restore the behavior in Bug 1175562.
That's going to make our tests more annoying. I'll make a note of it.
https://bugzilla.mozilla.org/show_bug.cgi?id=1175562#c2

as you pointed out in the review comments we can force an immediate update by setting the nextupdate in the past.
You need to log in before you can comment on or make changes to this bug.