432492 - rate limit long-running safebrowsing updates

Status: RESOLVED FIXED

Description

[Dave Camp (:dcamp)]

We need to prevent long-running safebrowsing updates from chewing up CPU for long amounts of time.  At the safebrowsing meeting today we discussed rate-limiting this to deal with potential worst-cases (server bugs like 431563, and very slow machines).

Comment 1

[Dave Camp (:dcamp)]

Created attachment 319702 [details] [diff] [review]
v1

This is a hasty fix, but it should be better than what's there.

Basically, after each redirect we check to see if we've gone over our time limit (currently 5 seconds), and if so we commit the current transaction and wait 60 seconds before restarting.

Comment 2

[Dave Camp (:dcamp)]

FWIW, the reason we do this per-redirect is that we can't commit the transaction mid-stream.  We can't commit mid-stream because we haven't yet confirmed that the MAC is correct. 

Long-term we can work around this by separating downloading of the chunks from adding them to the db (allowing us to check the MAC before beginning the db-updating process), but this is a bigger change than we should take right now.

Comment 3

[Tony Chang (Google)]

Comment on attachment 319702 [details] [diff] [review]
v1

>+  // Start time of the current update interval.  This will be reset
>+  // every time weapply the update.
>+  PRIntervalTime mUpdateStartTime;

Nit: weapply -> we apply

This may also result in some weird behavior if the user puts their computer to sleep and resumes, but that's probably not a big deal.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319702 [details] [diff] [review]
v1

a1.9=beltzner

Comment 5

[Reed Loden [:reed] (use needinfo?)]

Checking in toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl,v  <--  nsIUrlClassifierDBService.idl
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.76; previous revision: 1.75
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp,v  <--  nsUrlClassifierStreamUpdater.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h,v  <--  nsUrlClassifierStreamUpdater.h
new revision: 1.16; previous revision: 1.15
done

Comment 6

[Dave Townsend [:mossop]]

I've backed this out as reed thought it might be causing the tinderbox failures. When it is landed again make sure to rev the iid on the idl.

Comment 7

[Mike Connor [:mconnor]]

Yeah, it looks like this definitely caused the failures...

Comment 8

[Dave Camp (:dcamp)]

Doh.  The patch moved the call to CloseDb() to clear the update cache up before the update is applied.  CloseDb() called CancelUpdate(), causing confusion.

Testing a new patch now that takes the CancelUpdate() out of CloseDb(), and calls it manually at shutdown.

Comment 9

[Dave Camp (:dcamp)]

Created attachment 319851 [details] [diff] [review]
v2

New patch fixes the CancelUpdate() problem and bumps the uuid.

Comment 10

[Dave Camp (:dcamp)]

Created attachment 319854 [details] [diff] [review]
with nit fixed

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl 	1.25
mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp 	1.79
mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp 	1.26
mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h 	1.18 

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319854 [details] [diff] [review]
with nit fixed

a1.9=beltzner