461891 - switch to using v2.2 safebrowsing servers

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

Attachment: switch prefs

Simple fix to switch to reporting a pver of 2.2 when requesting from the safebrowsing server.  The fixes necessary to support 2.2 are already on trunk.

Comment 1

[Tony Chang (Google)]

Comment on attachment 345025 [details] [diff] [review]
switch prefs

+        // XXX: temp
+        result->mConfirmed = PR_TRUE;

Can you explain this to me?  Otherwise, looks good.

Comment 2

[Dave Camp (:dcamp)]

err yeah, that snuck in for a completely unrelated test, I'll take that out before landing.

Comment 3

[Bartłomiej Brzozowiec (BartZilla)]

Comment on attachment 345025 [details] [diff] [review]
switch prefs

>--- a/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp       Mon Oct 27 10:55:51 2008 -0700
>+++ b/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp       Mon Oct 27 17:02:38 2008 -0700
>-#define MAX_PATH_COMPONENTS 4
>+#define MAX_PATH_COMPONENTS 5

Could you comment on this change somehow?

BTW: Where is the specification of the new "safebrowsing" protocol (v2.2)? http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec says at the moment: "Client specification for the Google Safe Browsing v2.1 protocol".

If it is not publicly available, could you enumerate all differences between version 2.1 and 2.2?

Comment 4

[Dave Camp (:dcamp)]

(In reply to comment #3)
> (From update of attachment 345025 [details] [diff] [review])
> >--- a/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp       Mon Oct 27 10:55:51 2008 -0700
> >+++ b/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp       Mon Oct 27 17:02:38 2008 -0700
> >-#define MAX_PATH_COMPONENTS 4
> >+#define MAX_PATH_COMPONENTS 5
> 
> Could you comment on this change somehow?

Err, yes:  that was part of the unrelated test from comment #2, I'll post a new patch with just the pref changes.

> BTW: Where is the specification of the new "safebrowsing" protocol (v2.2)?
> http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec says at the
> moment: "Client specification for the Google Safe Browsing v2.1 protocol".

I'll bug google to update it.

There are actually no significant protocol changes in 2.2.  There has been one behavior change on the server - the server can send an empty add chunk as a way to fill in gaps in the chunk list.  Previously after some expiration, the add chunk list would say something like "1-2,4-10,13-50".  With server version 2.2, they might send an empty chunk for 3, 11, and 12, so that the client will say it has 1-50.  This is just to keep the request size smaller.

There's nothing in the spec that actually needs to change (empty add chunks are perfectly legal in 2.1).  All that they might consider adding is a recommendation that an empty add chunk should be allowed to expire subs that were supposed to apply to that add chunk.  It's this change that I refer to as "the fixes needed to support 2.2 on trunk".

Comment 5

[Dave Camp (:dcamp)]

Attachment: the right patch

just the prefs, none of the crap.

Comment 6

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #4)
> (...)
Yeah, some time ago I noticed similar changes (regarding zero size chunks) in Google Chrome too (http://src.chromium.org/viewvc/chrome?view=rev&sortby=date&revision=3131)...
Thanks for your explanation.

Comment 7

[Dave Camp (:dcamp)]

Landed as http://hg.mozilla.org/mozilla-central/rev/ee04803822c9

Comment 8

[Dave Camp (:dcamp)]

Attachment: branch patch

Branch patch includes a low-risk subset of the patch from bug 459281, which has been on trunk for a few weeks now.

Comment 9

[Dave Camp (:dcamp)]

Asking for this to block 3.0.5 at google's request.  Switching to the 2.2 servers will significantly reduce bandwidth usage at google's end (and for individual users), due to less fragmented chunk requests.

Comment 10

[Tony Chang (Google)]

Comment on attachment 347892 [details] [diff] [review]
branch patch

LG

Are the tests not portable?

Comment 11

[Daniel Veditz [:dveditz]]

Yes, please, we want some tests for the 1.9.0 branch
Code freeze is on Monday, and although we've marked it blocking we probably wouldn't really hold the release for this so please make sure you don't miss.

Comment 12

[Dave Camp (:dcamp)]

Attachment: branch patch, now with tests

New branch patch backports the relevant test too.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 348093 [details] [diff] [review]
branch patch, now with tests

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 14

[Dave Camp (:dcamp)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.339; previous revision: 1.338
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.82; previous revision: 1.81
done
Checking in toolkit/components/url-classifier/tests/unit/test_addsub.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v  <--  test_addsub.js
new revision: 1.8; previous revision: 1.7
done

Comment 15

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

Verified changes with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre.