Closed Bug 1360480 Opened 4 years ago Closed 4 years ago

about:url-classifier: Cache information

Categories

(Toolkit :: Safe Browsing, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Blocks 1 open bug)

Details

(Whiteboard: #sbv4-m7 )

Attachments

(4 files)

https://bugzilla.mozilla.org/show_bug.cgi?id=1338079#c0

A panel in about:url-classifier to show cache information and have a button to clear cache
Whiteboard: #sbv4-m7
Priority: -- → P3
Attached patch WIP patchSplinter Review
Hi francois,
Attach the screenshots for cache panel I am working now, would like to know if you have any suggestion, thx!

The data of secondscreen shot is fake(All the fullhashes are the same), I just want to show that what it will look likes if there are multiple completions for a prefix.
Flags: needinfo?(francois)
That looks good to me.

The only thing I'd change is how we display the hashes. Instead of using base64 of the hash, I would use the same hexadecimal codes we use in the MOZ_LOG() messages.
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #5)
> That looks good to me.
> 
> The only thing I'd change is how we display the hashes. Instead of using
> base64 of the hash, I would use the same hexadecimal codes we use in the
> MOZ_LOG() messages.

sure! thanks for suggestion :)
Depends on: 1333328
Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

https://reviewboard.mozilla.org/r/139992/#review143692

::: toolkit/components/url-classifier/Entries.h:327
(Diff revision 1)
>  typedef nsCStringHashKey FullHashString;
>  
>  typedef nsDataHashtable<FullHashString, int64_t> FullHashExpiryCache;
>  
>  struct CachedFullHashResponse {
> -  int64_t negativeCacheExpirySec;
> +  int64_t negativeCacheExpirySec = 0;

Does this make sense as a default value?

It means that the prefix is already expired as soon as we receive it. A value like 60 seconds seems more reasonable if we don't get a cacheExpiry from the server.

I know we put an upper bound on expiries, do we also have a lower bound or should we file a follow up bug for this?

::: toolkit/components/url-classifier/LookupCache.cpp:55
(Diff revision 1)
>  const int LookupCacheV2::VER = 2;
>  
> +static
> +void CStringToHexString(const nsACString& aIn, nsACString& aOut)
> +{
> +  static const char* const lut = "0123456789ABCDEF";

What does `lut` stand for?

::: toolkit/components/url-classifier/LookupCache.cpp:56
(Diff revision 1)
>  
> +static
> +void CStringToHexString(const nsACString& aIn, nsACString& aOut)
> +{
> +  static const char* const lut = "0123456789ABCDEF";
> +  // 32 bytes is the longest hash

Maybe that should be a `MOZ_ASSERT(len <= COMPLETE_SIZE)` instead of a comment?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:964
(Diff revision 1)
> +nsresult
> +nsUrlClassifierDBServiceWorker::GetCacheInfo(const nsACString& aTable,
> +                                             nsIUrlClassifierCacheInfo** aCache)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread(), "Must be on the background thread");
> +  MOZ_ASSERT(mClassifier, "Classifier connection must be opened");

"open"

::: toolkit/components/url-classifier/nsUrlClassifierInfo.cpp:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla 

nit: trailing whitespace

::: toolkit/components/url-classifier/nsUrlClassifierInfo.cpp:17
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +nsUrlClassifierPositiveCacheEntry::GetExpiry(int64_t* aExpiry)
> +{
> +  MOZ_ASSERT(aExpiry);

That should probably be a check, not just an assert.

::: toolkit/components/url-classifier/nsUrlClassifierInfo.cpp:47
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +nsUrlClassifierCacheEntry::GetExpiry(int64_t* aExpiry)
> +{
> +  MOZ_ASSERT(aExpiry);

ditto

::: toolkit/components/url-classifier/nsUrlClassifierInfo.cpp:56
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +nsUrlClassifierCacheEntry::GetMatches(nsIArray** aMatches)
> +{
> +  MOZ_ASSERT(aMatches);

ditto

::: toolkit/components/url-classifier/nsUrlClassifierInfo.cpp:86
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +nsUrlClassifierCacheInfo::GetEntries(nsIArray** aEntries)
> +{
> +  MOZ_ASSERT(aEntries);

ditto

::: toolkit/content/aboutUrlClassifier.js:220
(Diff revision 1)
> +      let dbservice = Cc["@mozilla.org/url-classifier/dbservice;1"]
> +                      .getService(Ci.nsIUrlClassifierDBService);
> +      dbservice.clearCache();
> +      // Since clearCache is async call, we just simply assume it will be
> +      // updated in 100 milli-seconds.
> +      setTimeout(() => { this.refresh(); }, 100);

Ideally this should be based on a callback instead of a timer.

But if it's too hard, then it's not the end of the world. This is just a debug page after all.

::: toolkit/locales/en-US/chrome/global/aboutUrlClassifier.dtd:23
(Diff revision 1)
> +<!ENTITY aboutUrlClassifier.cachePCacheEntries          "Number of positive cache entries">
> +<!ENTITY aboutUrlClassifier.cacheShowEntries            "Show entries">
> +<!ENTITY aboutUrlClassifier.cacheEntries                "Cache Entries">
> +<!ENTITY aboutUrlClassifier.cachePrefix                 "Prefix">
> +<!ENTITY aboutUrlClassifier.cacheNCacheExpiry           "Negative cache expiry">
> +<!ENTITY aboutUrlClassifier.cacheFullhash               "Fullhash">

"Full hash"
Attachment #8868400 - Flags: review?(francois) → review-
Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

https://reviewboard.mozilla.org/r/139992/#review143692

Thanks for review!

> Does this make sense as a default value?
> 
> It means that the prefix is already expired as soon as we receive it. A value like 60 seconds seems more reasonable if we don't get a cacheExpiry from the server.
> 
> I know we put an upper bound on expiries, do we also have a lower bound or should we file a follow up bug for this?

I think we don't need to set the default value here.
I use this default value in LookupCacheV2::AddGethashResultToCache to know if a "CachedFullHashResponse" object is newly created or it already exists(when return from mCache.LookupOrAdd). If it already exists, I will not update negative cache expiry, only update positive cache expiry in this case.

There are two mistakes here:
First, there is other API to get this done instead of using a default value.
Second, I think we should still update negative cache expiry when finding a completion, otherwise, an old negative cache expiry might cause a new positive cache entry being removed when update.

I'll update this in next patch.

> What does `lut` stand for?

I think it means Lookup table.
I copy this function from https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/Entries.h#96

> Ideally this should be based on a callback instead of a timer.
> 
> But if it's too hard, then it's not the end of the world. This is just a debug page after all.

It isn't hard, but it will make the code more complex.
Since this function is currently only used by a debug page, I would prefer keeping it as simple as possible.
How do you think?
Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

https://reviewboard.mozilla.org/r/139992/#review144650
Attachment #8868400 - Flags: review?(francois) → review+
Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

https://reviewboard.mozilla.org/r/139992/#review143692

> It isn't hard, but it will make the code more complex.
> Since this function is currently only used by a debug page, I would prefer keeping it as simple as possible.
> How do you think?

Fair enough.
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/261fe706dd83
about:url-classifier: Cache information. r=francois
https://hg.mozilla.org/mozilla-central/rev/261fe706dd83
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Carsten Book [:Tomcat] from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/261fe706dd83
> +<!ENTITY aboutUrlClassifier.cacheTitle                  "Cache">

Note that aboutUrlClassifier.cacheTitle was already present in the file so now appears twice.
(In reply to Ton from comment #16)
> (In reply to Carsten Book [:Tomcat] from comment #15)
> > https://hg.mozilla.org/mozilla-central/rev/261fe706dd83
> > +<!ENTITY aboutUrlClassifier.cacheTitle                  "Cache">
> 
> Note that aboutUrlClassifier.cacheTitle was already present in the file so
> now appears twice.

We need to get rid quickly of this string, i.e I can't expose strings safely to localization tools with a duplicate like this.

On a side note, can you start looping me in in bugs that land strings for this feature? Some of the strings you're landing could benefit from localization tools.
Depends on: 1367000
You need to log in before you can comment on or make changes to this bug.