1312339 - Return length in LookupCache.Has and support VariableLengthPrefix in LookupResultArray

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Thomas Nguyen (:tnguyen)]

LookupCache.Has should provide length in return value, so that we could find the matched V4 prefix (variable length) to do fullHashes.find

Comment 1

[François Marier [:francois]]

This probably doesn't need to land separately and could land at the same time as the code which will use this new return value.

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug 1312339 - LookupResult to support variable length partial hash.

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

Comment 3

[Henry Chang [:hchang]]

About this patch:

1) Based on Bug 1276826 regarding the change of "nsIUrlClassifierHashCompleter.complete". 
   Other than that all the changes in this patch are independent.

2) Before this patch, the matched prefixes for v4 tables are dropped [1] to prevent 
   premature v4 hash completion. This patch will get rid of that guard so the 
   LookupResultArray would possibly contain v4 prefixes.

3) The primary goal of this patch is to extend |LookupCache::Has| to return 
   "how long (in bytes) a prefix is matched". We need the length to be propagated 
   all the way to LookupResult [2], which is the intermediary between "prefix match" 
   and "hash completion". LookupResult is currently a 32-byte storage where both a 4-byte
   fixed-length prefix or a 32-byte completion could be interpreted. We don't dramatically
   change the implementation but only

     a) Rename |prefix| (and anything related) to |fixedLengthPrefix|.
     b) Provide a new interface |PartialHash| to return a arbitrary-length prefix.
     c) Add a member variable |mPartialHashLength| in cooperation with (2)

   , which effectively turns LookupResult into a v4-compatible object with minimum
   changes.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/toolkit/components/url-classifier/Classifier.cpp#485
[2] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/toolkit/components/url-classifier/LookupCache.h#26

Comment 4

[François Marier [:francois]]

Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.

https://reviewboard.mozilla.org/r/98570/#review99168

I'm OK with landing this separately (unlike what I suggested in comment 1) as long as we address the following issues.

::: toolkit/components/url-classifier/Classifier.cpp:474
(Diff revision 1)
>      for (uint32_t i = 0; i < cacheArray.Length(); i++) {
>        LookupCache *cache = cacheArray[i];
>        bool has, complete;
> +      uint32_t matchLength;
>  
> +      /*

Why is this commented-out?

If it's no longer useful, we should remove it.

::: toolkit/components/url-classifier/LookupCache.h:44
(Diff revision 1)
>      return hash.complete;
>    }
>  
> +  nsCString PartialHash() {
> +    MOZ_ASSERT(mPartialHashLength <= COMPLETE_SIZE);
> +    return nsCString((char*)hash.complete.buf, mPartialHashLength);

We should use C++ casts whenever possible.

::: toolkit/components/url-classifier/LookupCache.cpp:427
(Diff revision 1)
>  nsresult
>  LookupCacheV2::Has(const Completion& aCompletion,
> -                   bool* aHas, bool* aComplete)
> +                   bool* aHas, bool* aComplete,
> +                   uint32_t* aMatchLength)
>  {
>    *aHas = *aComplete = false;

Should we also initialize `aMatchLength` to `0` here?

::: toolkit/components/url-classifier/LookupCacheV4.cpp:86
(Diff revision 1)
>  nsresult
>  LookupCacheV4::Has(const Completion& aCompletion,
> -                   bool* aHas, bool* aComplete)
> +                   bool* aHas, bool* aComplete,
> +                   uint32_t* aMatchLength)
>  {
>    *aHas = false;

This should probably set the same defaults as the V2 version (`*aHash = *aComplete = false` and `*aMatchLength = 0`).

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:967
(Diff revision 1)
>        // complete.
>        if ((!gethashUrl.IsEmpty() ||
>             StringBeginsWith(result.mTableName, NS_LITERAL_CSTRING("test-"))) &&
>            mDBService->GetCompleter(result.mTableName,
>                                     getter_AddRefs(completer))) {
>          nsAutoCString partialHash;

It looks like we don't need this variable anymore.

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.

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

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.

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

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98570/diff/3-4/

Comment 8

[François Marier [:francois]]

Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.

https://reviewboard.mozilla.org/r/98570/#review99554

Looks great, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa12e9471978
LookupResult to support variable length partial hash. r=francois

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa12e9471978