1359299 - v4 caches in LookupCache need to be copied around in copy constructor

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP patch

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

After adopting the new thread model for safebrowsing, we will create a new
lookup cache for update so we can still check lookup cache at the same time.

Prefix set, completions will be generated when we open the new lookup cache
but it won't include cache, so we will loss cache after that.

This patch will copy cache data from old lookup cache to new lookup
cache while update.

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

Comment 3

[Dimi Lee [:dimi]]

Hi Henry,
Could you help review this patch since you familiar the code here most, thanks!
This patch is based on patch in bug 1333328 but it is not yet landed at this moment.

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

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

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

https://reviewboard.mozilla.org/r/139210/#review143404

The approach looks pretty good to me except some cofusing naming. 
Sorry I can't come out better names but it's worth figuring out
more suitable names.

Thanks!

::: toolkit/components/url-classifier/Classifier.h:140
(Diff revision 2)
>    nsresult CopyInUseDirForUpdate();
>    nsresult RegenActiveTables();
>  
>    void MergeNewLookupCaches(); // Merge mNewLookupCaches into mLookupCaches.
>  
> +  void CopyOldLookupCacheToUpdate(LookupCache* aUpdateLookupCache);

Since we have two differnt level of "cache" in the "safebrowsing update" context, we might need a better naming for the LookupCache::mCache and rename this function to explicitly indicate we will only copy LookupCache::mCache but not the whole LookupCache object.

::: toolkit/components/url-classifier/Classifier.cpp:1472
(Diff revision 2)
>    if (NS_SUCCEEDED(rv)) {
> +    if (aForUpdate) {
> +      // We need to ensure the LookupCache for update should have the same
> +      // 'in-memory' data as the one in mLookupCaches.
> +      // Prefixes and completions will be generated in cache->Open so the
> +      // only data will be copied here is cache.

We may add comment to describe why we need to copy the internal cache before applying update.

::: toolkit/components/url-classifier/LookupCache.h:230
(Diff revision 2)
>    static T* Cast(LookupCache* aThat) {
>      return ((aThat && T::VER == aThat->Ver()) ? reinterpret_cast<T*>(aThat) : nullptr);
>    }
>  
> +  LookupCache& operator=(const LookupCache& aOther) {
> +    CopyClassHashTable<FullHashResponseMap>(aOther.mCache, mCache);

The assignment operator is supposed to do a deep copy so copying only partial of its members seems a little confusing. Maybe an explicit function name (e.g. CopyInternalCache() (sounds no good though)) is more suitable in your current design.

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

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

Comment 7

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

https://reviewboard.mozilla.org/r/139210/#review143716

::: toolkit/components/url-classifier/Classifier.h:140
(Diff revision 2)
>    nsresult CopyInUseDirForUpdate();
>    nsresult RegenActiveTables();
>  
>    void MergeNewLookupCaches(); // Merge mNewLookupCaches into mLookupCaches.
>  
> +  void CopyOldLookupCacheToUpdate(LookupCache* aUpdateLookupCache);

Thanks for the suggestion! Naming is really hard :(

I change mCache to mFullHashCache because it caches the response of fullhash request in v4 and gethash request in v2

Do you think it is good enough?

::: toolkit/components/url-classifier/LookupCache.h:230
(Diff revision 2)
>    static T* Cast(LookupCache* aThat) {
>      return ((aThat && T::VER == aThat->Ver()) ? reinterpret_cast<T*>(aThat) : nullptr);
>    }
>  
> +  LookupCache& operator=(const LookupCache& aOther) {
> +    CopyClassHashTable<FullHashResponseMap>(aOther.mCache, mCache);

Agree, I will use a explicit function.

Comment 8

[Dimi Lee [:dimi]]

Offline discussed with henry, he is ok with the naming changes in the latest patch.

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

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

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0b940487708
Copy fullhash cache when update. r=hchang

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing Windows safebrowsing crashes in automation.
https://hg.mozilla.org/mozilla-central/rev/d93182f36b3c134a3b1c6718f09eb87c2913e364

https://treeherder.mozilla.org/logviewer.html#?job_id=101461379&repo=autoland&lineNumber=4763

Comment 14

[Dimi Lee [:dimi]]

I think the problem is because mLookupCaches is accessed in both update thread and worker thread.
I'll discuss this with Henry after him back from PTO.

Comment 15

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139210/diff/5-6/

Comment 16

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139210/diff/6-7/

Comment 17

[Dimi Lee [:dimi]]

Hi Henry,
I just update another patch according to our discussion yesterday.
Could you help review again? Thanks!

Comment 18

[Henry Chang [:hchang]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

https://reviewboard.mozilla.org/r/139210/#review151170

The change looks good to me except for the function name. Probably your original name is already good enough. Thanks!

::: toolkit/components/url-classifier/Classifier.cpp:642
(Diff revision 7)
> +
> +  // New lookup caches are built from disk, data likes cache which is
> +  // generated online won't exist. We have to manually copy cache from
> +  // old LookupCache to new LookupCache.
> +  for (auto& newCache: mNewLookupCaches) {
> +    for (uint32_t i = 0; i < mLookupCaches.Length(); i++) {

nit: we can also use range-base for loop here.

::: toolkit/components/url-classifier/Classifier.cpp:893
(Diff revision 7)
>      return NS_OK;
>    }
>    if (NS_SUCCEEDED(aBackgroundRv)) {
> +    // Since mLookupCaches is only available on worker thread, we should
> +    // put operations that will manipulate mLookupCaches in PostUpdate().
> +    PostUpdate();

It's not so "post update" to me so we might just use a more specific function name here.

Comment 19

[Dimi Lee [:dimi]]

Comment on attachment 8867633 [details]
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139210/diff/7-8/

Comment 20

[Dimi Lee [:dimi]]

(In reply to Henry Chang [:hchang] from comment #18)
> The change looks good to me except for the function name. Probably your
> original name is already good enough. Thanks!
> 

Thanks for suggestion!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1a9cda28b1b
V4 caches in LookupCache need to be copied around in copy constructor. r=hchang

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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