Closed
Bug 1359299
Opened 8 years ago
Closed 8 years ago
v4 caches in LookupCache need to be copied around in copy constructor
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
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.
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
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 hidden (mozreview-request) |
Reporter | ||
Comment 5•8 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-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.
Assignee | ||
Comment 8•8 years ago
|
||
Offline discussed with henry, he is ok with the naming changes in the latest patch.
Assignee | ||
Updated•8 years ago
|
Attachment #8867610 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0b940487708
Copy fullhash cache when update. r=hchang
![]() |
||
Comment 12•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 13•8 years ago
|
||
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 → ---
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(dlee)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
Hi Henry,
I just update another patch according to our discussion yesterday.
Could you help review again? Thanks!
Flags: needinfo?(hchang)
Reporter | ||
Comment 18•8 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years ago
|
||
(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)
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•