432332 - duplicate chunks in client request

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Marria Nazif]

The client request appears to repeat the same chunk over and over again in many cases. For example, here is a snippet from the middle of one client request:
1003-1004,1004,1004-1005,1005-1006,1006-1009,1009-1010,1010-1011,1011-1012,1012-1013,10
13-1014,1014-1015,1015,1015-1016,

The appropriate behavior should have been to write this as "1003-1016".

Comment 1

[Marria Nazif]

To clarify, I'm seeing this mostly with subs, but I can't verify that it isn't happening for adds.

Comment 2

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

Hmm. Related to bug 431563 or bug 402469?

Comment 3

[Marria Nazif]

From my end, this appears to be a different bug.  It probably just has to do with how the client keeps track of which chunks it has.  Perhaps the mechanism for collapsing chunk ranges is not working as expected.

Comment 4

[Dave Camp (:dcamp)]

So, it turns out I saw something like this while I was testing the fix for 430530.

I saw it because my testing framework was accidentally sending a chunk multiple times.  Is there any chance the server is sending the same chunk multiple times?

(The patch in 430530 fixed up the chunk range collapsing to not choke in this case)

Comment 5

[Dave Camp (:dcamp)]

Err, didn't mean to mark this fixed - it sounds like the server is possibly sending us duplicate subs, which is bad news.  We should figure that out.

Comment 6

[Marria Nazif]

It is indeed possible that the server sends the same chunk down more than once (has to do with how the data is cached) but there should be no where near this amount of duplication, if the client is in fact sending exactly the chunk numbers that it got from the server.

Can you tell me what the client was doing to maintain chunk numbers before your fix, so I can get an idea for how much duplication there actually is?

Comment 7

[Marria Nazif]

Also, note that we are seeing this for beta 5, and we have totally different server behavior for the nightlies.  It would help to know if this is still a problem on nightlies.

Comment 8

[Dave Camp (:dcamp)]

Yeah, basically it would append the chunk number to the list of active chunk numbers.  At the end of an update it would sort and stringify.  Each duplicated chunk number led to an extra comma, as you saw above.  So yeah, it looks like you ARE sending that amount of duplication :(.

So, I didn't realize that duplicates like that were a common occurrence.  Checking for duplicates is an easy, though somewhat expensive, operation.  I chose not to actually do this duplicate checking, because (aside from the bad stringification), applying the same chunk twice isn't a significant problem - duplicate entries will be cleaned up by their subs/expirations appropriately, so it's just a matter of some extra db space.

But if this is a common, expected occurrence, we should suck it up and spend the CPU to check for duplicates before adding :/.  I'm in the process of rebuilding my laptop, but once my build environment is set back up I'll put together a patch.

Comment 9

[Dave Camp (:dcamp)]

I think we should block.  And I'm exaggerating the problem, it'll be a simple, low-risk patch.

Comment 10

[Dave Camp (:dcamp)]

Attachment: v1

Comment 11

[Tony Chang (Google)]

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

I'm a bit worried that InsertElementAt can become quite expensive for larger arrays, but maybe in practice it's pretty fast because it's only ints.

Comment 12

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

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

a1.9=beltzner

Comment 13

[Dave Camp (:dcamp)]

I don't have my ssh key readily available, reed can you land this for me please?

Comment 14

[Dave Camp (:dcamp)]

Attachment: v2

New version is the same as the last but bumps the IMPLEMENTATION_VERSION too - people with the old code could very well have a whole lot of dups in their dbs causing problems.

Comment 15

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

Comment on attachment 319701 [details] [diff] [review]
v2

Doesn't need approval again.

Comment 16

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

mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp 	1.75
mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js 	1.14
mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js 	1.7
mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js 	1.8