Closed
Bug 432492
Opened 16 years ago
Closed 16 years ago
rate limit long-running safebrowsing updates
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
Firefox 3
People
(Reporter: dcamp, Assigned: dcamp)
Details
Attachments
(1 file, 2 obsolete files)
23.49 KB,
patch
|
tony
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
Updated•16 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 1•16 years ago
|
||
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 | ||
Comment 2•16 years ago
|
||
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.
Updated•16 years ago
|
Whiteboard: [has ptach][needs review tony]
Comment 3•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [has ptach][needs review tony] → [has patch][needs approval]
Comment 4•16 years ago
|
||
Comment on attachment 319702 [details] [diff] [review] v1 a1.9=beltzner
Attachment #319702 -
Flags: approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][needs approval] → [has patch][has review][has approval]
Comment 5•16 years ago
|
||
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
Comment 6•16 years ago
|
||
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 → ---
Updated•16 years ago
|
Whiteboard: [has patch][has review][has approval]
Comment 7•16 years ago
|
||
Yeah, it looks like this definitely caused the failures...
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
New patch fixes the CancelUpdate() problem and bumps the uuid.
Attachment #319702 -
Attachment is obsolete: true
Attachment #319851 -
Flags: review?(tony)
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #319851 -
Attachment is obsolete: true
Attachment #319854 -
Flags: review?(tony)
Attachment #319851 -
Flags: review?(tony)
Updated•16 years ago
|
Attachment #319854 -
Flags: review?(tony) → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 11•16 years ago
|
||
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 ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Comment 12•16 years ago
|
||
Comment on attachment 319854 [details] [diff] [review] with nit fixed a1.9=beltzner
Attachment #319854 -
Flags: approval1.9+
Updated•10 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•