Closed Bug 1359299 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hchang, Assigned: dimi)

References

Details

(Whiteboard: #sbv4-m7)

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 1311935
Priority: -- → P2
Whiteboard: #sbv4-m7
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
Blocks: 1333328
No longer blocks: 1333328
No longer blocks: 1311935
Depends on: 1333328, 1311935
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 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.
Attachment #8867633 - Flags: review?(hchang) → review+
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.
Offline discussed with henry, he is ok with the naming changes in the latest patch.
Attachment #8867610 - Attachment is obsolete: true
Depends on: 1365877
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0b940487708
Copy fullhash cache when update. r=hchang
https://hg.mozilla.org/mozilla-central/rev/c0b940487708
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Status: RESOLVED → REOPENED
Flags: needinfo?(dlee)
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
Depends on: 1367394
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.
Flags: needinfo?(dlee)
Hi Henry,
I just update another patch according to our discussion yesterday.
Could you help review again? Thanks!
Flags: needinfo?(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.
(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!
Flags: needinfo?(hchang)
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1a9cda28b1b
V4 caches in LookupCache need to be copied around in copy constructor. r=hchang
https://hg.mozilla.org/mozilla-central/rev/f1a9cda28b1b
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1367394
You need to log in before you can comment on or make changes to this bug.