1364607 - Add a test for empty Safe Browsing updates

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

Google suspects that the checksum mismatches we are seeing during updates (bug 1364606) may be due to empty updates. In that case, the server should return an empty list with the same hash as you already have.

They will add a unit test for this. We should do the same.

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

If I understand "empty update" correctly, it will be skipped by "CheckValidUpdate"[1] because it is empty[2].
So we won't treat this as checksum mismatch.

I'll see if I can reproduce this locally.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/Classifier.cpp#1326
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/Classifier.cpp#1184

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Bug 1364607 - Add a test for empty Safe Browsing updates.

Review commit: https://reviewboard.mozilla.org/r/140034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140034/

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Hi francois,
I think empty update is not related to checksum mismatch issue in bug 1364606, but add a test case for this is still useful.
This patch includes two test cases:
1. Apply an empty update through Classifier interface, which is the normal use case.
2. Apply an empty through LookupCacheV4::ApplyUpdate, this ensure update algorithm is correct when applying an empty update.
   This scenario actually shouldn't happen in normal use case because empty update will be skipped by Classifier::CheckValidUpdate

Comment 4

[François Marier [:francois]]

Comment on attachment 8868438 [details]
Bug 1364607 - Add a test for empty Safe Browsing updates.

https://reviewboard.mozilla.org/r/140034/#review143688

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:761
(Diff revision 1)
>    testOpenLookupCache();
>  
>    Clear();
>  }
>  
> +// This test ensure an empty update works correctly. Empty update

"This test ensures that an..."

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:795
(Diff revision 1)
> +  testFullUpdate(emptyAddition, &checksum);
> +
> +  Clear();
> +}
> +
> +// This test ensure apply an empty update directly through udpate algorithm

"This test ensures that applying an..."

also, small typo: "update"

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8868438 [details]
Bug 1364607 - Add a test for empty Safe Browsing updates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140034/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19e1fbbfc10f
Add a test for empty Safe Browsing updates. r=francois

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19e1fbbfc10f