Closed
Bug 1328821
Opened 7 years ago
Closed 7 years ago
hash completion request for v4 should not depend on table freshness
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: dlee, Assigned: dlee)
References
Details
(Whiteboard: #sbv4-m5)
Attachments
(1 file)
Right now when Classifier matches a completion, table freshness is used to decide if this completion is out of date or not, the reason we are doing that is because we don't have a TTL for caching. As for V4, positive caching and negative caching is introduced[1] so we should not use table freshness as TTL. [1] https://developers.google.com/safe-browsing/v4/caching
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8823980 -
Flags: review?(hchang)
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8823980 [details] Bug 1328821 - hash completion request for v4 should not depend on table freshness. https://reviewboard.mozilla.org/r/102464/#review103712 Looks pretty good to me! Ideally I'd like to combine "freshness" and "positive/negative" cache as one concept. However, per offline discussion with Dimi and Thomas, combining freshness and positive/negative cache is not a trivial work and I don't see any immediate benefit from it. So just leave freshness and positive/negative cache split apart. ::: toolkit/components/url-classifier/Classifier.cpp:486 (Diff revision 2) > - int64_t age; > - bool found = mTableFreshness.Get(cache->TableName(), &age); > - if (!found) { > - age = 24 * 60 * 60; // just a large number > - } else { > - int64_t now = (PR_Now() / PR_USEC_PER_SEC); > + // For V2, there is no TTL for caching, so we use table freshness to > + // decide if matching a completion should trigger a gethash request or not. > + // For V4, this is done by Positive Caching & Negative Caching mechanism. > + bool confirmed = false; > + if (fromCache) { > + cache->IsHashEntryConfirmed(lookupHash, mTableFreshness, Is there any reason or concern that we can't merge |IsHashEntryConfirmed| to |Has|? It seems to me |fromCache| is just an intermediary info that Classifier does not need to know. Why don't we have |Has| return |confirmed| directly? One of the reasons might be to reduce the number of arguments we have to pass to |Has|. (otherwise we have pass mTableFreshness and aFreshnessGuarantee to |Has|) Anyways both works for me just would like to know if I am understanding correctly :) ::: toolkit/components/url-classifier/LookupCache.cpp:463 (Diff revision 2) > + bool* aConfirmed) > +{ > + int64_t age; > + bool found = aTableFreshness.Get(mTableName, &age); > + if (!found) { > + age = 24 * 60 * 60; // just a large number It seems to be copied from the old code, but what do you think of using just UINT_MAX or return false here?
Attachment #8823980 -
Flags: review?(hchang) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Thanks for review! (In reply to Henry Chang [:henry][:hchang] from comment #3) > > ::: toolkit/components/url-classifier/Classifier.cpp:486 > (Diff revision 2) > > - int64_t age; > > - bool found = mTableFreshness.Get(cache->TableName(), &age); > > - if (!found) { > > - age = 24 * 60 * 60; // just a large number > > - } else { > > - int64_t now = (PR_Now() / PR_USEC_PER_SEC); > > + // For V2, there is no TTL for caching, so we use table freshness to > > + // decide if matching a completion should trigger a gethash request or not. > > + // For V4, this is done by Positive Caching & Negative Caching mechanism. > > + bool confirmed = false; > > + if (fromCache) { > > + cache->IsHashEntryConfirmed(lookupHash, mTableFreshness, > > Is there any reason or concern that we can't merge |IsHashEntryConfirmed| to > |Has|? It seems to me |fromCache| is just an intermediary info that > Classifier does not need to know. Why don't we have > |Has| return |confirmed| directly? > > One of the reasons might be to reduce the number of arguments we have to > pass to |Has|. (otherwise we have pass mTableFreshness and > aFreshnessGuarantee to |Has|) > > Anyways both works for me just would like to know if I am understanding > correctly :) > You are right, the reason I add "IsHashEntryConfirmed" is too reduce the number of arguments passed to |Has|. Another reason is that we have testcase to test |Has| and i don't want to make |Has| to have "expiration" logic in it. > ::: toolkit/components/url-classifier/LookupCache.cpp:463 > (Diff revision 2) > > + bool* aConfirmed) > > +{ > > + int64_t age; > > + bool found = aTableFreshness.Get(mTableName, &age); > > + if (!found) { > > + age = 24 * 60 * 60; // just a large number > > It seems to be copied from the old code, but what do you think of using just > UINT_MAX or return false here? I'll set UINT_MAX, thanks for suggestion.
Comment hidden (mozreview-request) |
Updated•7 years ago
|
No longer blocks: safebrowsingv4
Updated•7 years ago
|
Priority: -- → P2
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8823980 [details] Bug 1328821 - hash completion request for v4 should not depend on table freshness. https://reviewboard.mozilla.org/r/102464/#review105036 Looks good. Just a few small things before giving you an r+. Feel free to remove the "for reviewer only" bits too in your next revision. ::: toolkit/components/url-classifier/LookupCache.h:73 (Diff revision 3) > > nsCString mTableName; > > uint32_t mPartialHashLength; > + > + // True if the lookup result is confirmed as an unsafe url. I would suggest "True as long as this lookup hasn't expired" ::: toolkit/components/url-classifier/LookupCache.cpp:460 (Diff revision 3) > +LookupCacheV2::IsHashEntryConfirmed(const Completion& aEntry, > + const TableFreshnessMap& aTableFreshness, > + uint32_t aFreshnessGuarantee, > + bool* aConfirmed) > +{ > + int64_t age; Let's add a comment to be explicit: int64_t age; // in seconds ::: toolkit/components/url-classifier/LookupCache.cpp:468 (Diff revision 3) > + *aConfirmed = false; > + } else { > + int64_t now = (PR_Now() / PR_USEC_PER_SEC); > + > + // Considered completion as unsafe if its table is up-to-date. > + *aConfirmed = (now - age) < aFreshnessGuarantee; Let's put an assert here which ensures that `age <= now`. ::: toolkit/components/url-classifier/LookupCacheV4.cpp:105 (Diff revision 3) > uint32_t prefix = aCompletion.ToUint32(); > LOG(("Probe in V4 %s: %X, found %d, complete %d", mTableName.get(), > - prefix, *aHas, *aComplete)); > + prefix, *aHas, length == COMPLETE_SIZE)); > } > > + // Note. !!! We could add `// TODO : Bug 1311935 - Implement v4 caching` in here too. ::: toolkit/components/url-classifier/LookupCacheV4.cpp:155 (Diff revision 3) > + uint32_t aFreshnessGuarantee, > + bool* aConfirmed) > +{ > + // TODO : Bug 1311935 - Implement v4 caching > + > + *aConfirmed = false; Maybe we should return `true` until this is implemented? It would prevent us sending any extra gethash requests. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1007 (Diff revision 3) > if (NS_SUCCEEDED(rv)) { > mPendingCompletions++; > } > } else { > // For tables with no hash completer, a complete hash match is > // good enough, we'll consider it fresh, even if it hasn't been updated Let's use the word "valid" instead of "fresh" since we're no longer relying on table freshness. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1008 (Diff revision 3) > mPendingCompletions++; > } > } else { > // For tables with no hash completer, a complete hash match is > // good enough, we'll consider it fresh, even if it hasn't been updated > // in 45 minutes. We may as well remove the hard-coded "45 minutes" here and say "even if it hasn't been updated in a long time".
Attachment #8823980 -
Flags: review?(francois) → review-
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8823980 [details] Bug 1328821 - hash completion request for v4 should not depend on table freshness. https://reviewboard.mozilla.org/r/102464/#review105822
Attachment #8823980 -
Flags: review?(francois) → review+
Comment 9•7 years ago
|
||
Let's wait until bug 1331534 has landed before landing this completion fixed. That will ensure that regardless of caching, we won't be honouring V4 matches.
Depends on: 1331534
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5561d7805979 hash completion request for v4 should not depend on table freshness. r=francois,henry
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5561d7805979
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•