Closed
Bug 1312339
Opened 8 years ago
Closed 8 years ago
Return length in LookupCache.Has and support VariableLengthPrefix in LookupResultArray
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: tnguyen, Assigned: hchang)
References
Details
(Whiteboard: #sbv4-m5)
Attachments
(1 file)
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•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: #sbv4-m4
Updated•8 years ago
|
Whiteboard: #sbv4-m4 → #sbv4-m5
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hchang
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8818495 -
Flags: review?(francois)
Comment 4•8 years ago
|
||
mozreview-review |
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.
Attachment #8818495 -
Flags: review?(francois) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8818495 [details]
Bug 1312339 - LookupResult to support variable length partial hash.
https://reviewboard.mozilla.org/r/98570/#review99554
Looks great, thanks!
Attachment #8818495 -
Flags: review?(francois) → review+
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa12e9471978
LookupResult to support variable length partial hash. r=francois
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•