Closed Bug 432492 Opened 16 years ago Closed 16 years ago

rate limit long-running safebrowsing updates

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: dcamp, Assigned: dcamp)

Details

Attachments

(1 file, 2 obsolete files)

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).
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → dcamp
Status: NEW → ASSIGNED
Attachment #319702 - Flags: review?(tony)
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.
Whiteboard: [has ptach][needs review tony]
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.
Attachment #319702 - Flags: review?(tony) → review+
Whiteboard: [has ptach][needs review tony] → [has patch][needs approval]
Keywords: checkin-needed
Whiteboard: [has patch][needs approval] → [has patch][has review][has approval]
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Target Milestone: --- → Firefox 3
Version: unspecified → Trunk
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has patch][has review][has approval]
Yeah, it looks like this definitely caused the failures...
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.
Attached patch v2 (obsolete) — Splinter Review
New patch fixes the CancelUpdate() problem and bumps the uuid.
Attachment #319702 - Attachment is obsolete: true
Attachment #319851 - Flags: review?(tony)
Attached patch with nit fixedSplinter Review
Attachment #319851 - Attachment is obsolete: true
Attachment #319854 - Flags: review?(tony)
Attachment #319851 - Flags: review?(tony)
Attachment #319854 - Flags: review?(tony) → review+
Keywords: checkin-needed
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 
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Comment on attachment 319854 [details] [diff] [review]
with nit fixed

a1.9=beltzner
Attachment #319854 - Flags: approval1.9+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: