378132 - table update requests once per minute on http 4xx response

Status: RESOLVED DUPLICATE of bug 423790

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

Murray has reported that if the table update request sees a http 404 response, it keeps hammering away trying to update once a minute.  This is a bug in the response handling.  If there's an update error but it's not one of the backoff responses (5xx), it falls into this unhandled area and we try to update again after a minute.

The code in question is here:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/components/url-classifier/content/listmanager.js#543

I think the right thing to do is just ignore the response and let the regular 30min check kick in.

Comment 1

[Tony Chang (Google)]

Attachment: ignore server responses that don't trigger backoff

If the download fails and the server returns something that isn't a backoff status, just ignore it.

Comment 2

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

Comment on attachment 263038 [details] [diff] [review]
ignore server responses that don't trigger backoff

Is this patch obsolete given the changes in bug 387196/bug 388652 etc?

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 263038 [details] [diff] [review]
ignore server responses that don't trigger backoff

please re-request approvals when gavin's question is answered and dcamp has reviewed this

Comment 4

[Tony Chang (Google)]

(In reply to comment #2)
> (From update of attachment 263038 [details] [diff] [review])
> Is this patch obsolete given the changes in bug 387196/bug 388652 etc?

No, we still need this patch, although the line numbers have changed due to bug 387196.

Even with the removal of enhanced mode, we'd still have this problem (this is for updates to the local phishing/malware database).

Comment 6

[Samuel Sidler (old account; do not CC)]

Comment on attachment 263038 [details] [diff] [review]
ignore server responses that don't trigger backoff

Requesting approval on this branch patch to get this on the radar.

Dave says: "very little [risk]"

"the only thing it touches is error cases anyway; so even totally worst case it would break anti-phishing backoff. and it's a simple patch that won't break :)"

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 263038 [details] [diff] [review]
ignore server responses that don't trigger backoff

approved for 1.8.1.14 and 1.8.1.15, a=dveditz for release-drivers.

This means it needs to land on two branches, the regular MOZILLA_1_8_BRANCH for future 2.0.0.x releases, and GECKO181_20080311_RELBRANCH for the 1.8.1.14 crash-fix release.

Comment 8

[Brian Ryner (not reading)]

Checked in on MOZILLA_1_8_BRANCH:
/cvsroot/mozilla/toolkit/components/url-classifier/content/listmanager.js,v  <--  listmanager.js
new revision: 1.1.2.15; previous revision: 1.1.2.14
done
Checking in request-backoff.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/request-backoff.js,v  <--  request-backoff.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done

and on GECKO181_20080311_RELBRANCH:
Checking in listmanager.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/listmanager.js,v  <--  listmanager.js
new revision: 1.1.2.14.24.1; previous revision: 1.1.2.14
done
Checking in request-backoff.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/request-backoff.js,v  <--  request-backoff.js
new revision: 1.1.2.1.24.1; previous revision: 1.1.2.1
done

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Do we have any sort of test server to verify this fix against?