1360480 - about:url-classifier: Cache information

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P3)

Description

[Dimi Lee [:dimi]]

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

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP patch

Comment 2

[Dimi Lee [:dimi]]

Attachment: Screenshot - without showing cache entries panel

Comment 3

[Dimi Lee [:dimi]]

Attachment: Screenshot - show cache entries panel

Comment 4

[Dimi Lee [:dimi]]

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.

Comment 5

[François Marier [: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.

Comment 6

[Dimi Lee [:dimi]]

(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 :)

Comment 7

[Dimi Lee [:dimi]]

Attachment: Bug 1360480 - about:url-classifier: Cache information.

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

Comment 8

[François Marier [:francois]]

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"

Comment 9

[Dimi Lee [:dimi]]

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 10

[Dimi Lee [:dimi]]

Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

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

Comment 11

[François Marier [:francois]]

Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

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

Comment 12

[François Marier [:francois]]

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.

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8868400 [details]
Bug 1360480 - about:url-classifier: Cache information.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/261fe706dd83
about:url-classifier: Cache information. r=francois

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/261fe706dd83

Comment 16

[Ton [:Tonnes]]

(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.

Comment 17

[Francesco Lodolo [:flod]]

(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.