Closed
Bug 1305780
Opened 8 years ago
Closed 8 years ago
Implement the update fail scheme for v4
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: dimi, Assigned: dimi)
References
Details
(Whiteboard: #sbv4-m2)
Attachments
(3 files)
For now, when an update error occurs(except for OOM, crash during update), we will call Classifier::Reset which will reset all the tables.
In V4, we might be able to just skip this update and keep previous state, then the next time we don't have to do an update from scrath.
Assignee | ||
Comment 2•8 years ago
|
||
Hi gcp,
Would like to know your opinion about not resetting database while there is an update error in V4[1].
Since in v4 we will have "state" for our current database, so if an error occurs while applying update, we can just skip the update this time and ask for an update later(by using the state we have in db).
This might not be related to this bug but another question is why do we reset entire database when there is an update error? why not just clear the table cause the update failure ?
Thanks!
[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/Classifier.cpp?q=UpdateHashStore&redirect_type=direct#546
Flags: needinfo?(gpascutto)
Assignee | ||
Updated•8 years ago
|
Summary: Do not reset entire database when there is an update error for V4 → Do not reset database when there is an update error for V4
Comment 3•8 years ago
|
||
(In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> Hi gcp,
> Would like to know your opinion about not resetting database while there is
> an update error in V4[1].
> Since in v4 we will have "state" for our current database, so if an error
> occurs while applying update, we can just skip the update this time and ask
> for an update later(by using the state we have in db).
>
> This might not be related to this bug but another question is why do we
> reset entire database when there is an update error?
The reason to reset the table on an update error, rather than just trying again later, is that the update may fail because our data is incorrect or corrupted. So the next update might fail again and again, and the user might be stuck with an old database that is no longer secure.
Conversely, if our data is correct, there should be no reason for the update to ever fail in the first place.
So the reset is there because it will recover the users' situation if their data is bad.
> why not just clear the table cause the update failure ?
I think when this was implemented, there were only 2 tables and 1 provider, and the transactional stuff always backed up/restored on all of them at once. So clearing everything was simpler.
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
Thanks for explanation.
Per discussion with francois, what we can do for v4 is using the checksum in update data to ensure the integrity of data on disk.
Following is the initial idea about how we gonna do it:
1. Whenever we apply an update, store the checksum to disk(the checksum is send along with update data).
2. If an update happens and there is an error, then we should compute the checksum of origin database and check if it matches the checksum we stored in disk,
3. If it doesn't match , reset the database
4. If it matches, we will just skip the update this time and keep the original database and state.
(We may still need to consider what if the partial update keeps fail)
But for now, we will keep the old way(reset database whenever an update error occurs).We will use the approach above until the telemetry shows us partial update errors happens frequently.
So set this bug as P5 for now.
Priority: P2 → P5
Summary: Do not reset database when there is an update error for V4 → Implement the update fail scheme for v4
Whiteboard: #sbv4-m1
Assignee | ||
Updated•8 years ago
|
Assignee: dlee → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Updated•8 years ago
|
Priority: P5 → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
Hi gcp,
I am still working on testcase but would like to know your opinion first so flag r?
Could you help check this ? thanks!
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.
https://reviewboard.mozilla.org/r/85836/#review84450
::: toolkit/components/url-classifier/Classifier.cpp:351
(Diff revision 1)
> }
>
> void
> Classifier::ResetTables(const nsTArray<nsCString>& aTables)
> {
> // Clear lookup cache
I made change here because i think we shouldn't only clear completion cache of the lookup cache when calling ResetTables(when receive pleasereset, and now when update fail).
This should be my mistake when writing pleasereset patch.
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.
https://reviewboard.mozilla.org/r/85836/#review85160
::: toolkit/components/url-classifier/Classifier.cpp:358
(Diff revision 1)
> + LOG(("Reset table: %s", aTables[i].get()));
> +
> + mTableFreshness.Remove(aTables[i]);
> + LookupCache *cache = GetLookupCache(aTables[i]);
> + if (cache) {
> + cache->ClearAll();
This is a copypaste of MarkSpoiled with a tiny difference.
How about reworking MarkSpoiled into something like ResetTable(Classifier::Clear_Cache) or ResetTable(Classifier::Clear_All).
::: toolkit/components/url-classifier/Classifier.cpp:400
(Diff revision 1)
> }
> NS_ENSURE_SUCCESS_VOID(rv);
> }
>
> void
> +Classifier::AbortUpdate(const nsCString& aTable)
This does a bit more than just Abort. It's a full reset. So maybe make the name reflect that. AbortUpdateAndReset?
::: toolkit/components/url-classifier/Classifier.cpp:409
(Diff revision 1)
> + // ResetTables will clear both in-memory & on-disk data.
> + ResetTables(nsTArray<nsCString> { aTable });
> +
> + // Remove the backup and delete directory since we are aborting
> + // from an update.
> + RemoveBackupTables();
Might need an Unused << annotation to avoid compiler warnings.
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.
https://reviewboard.mozilla.org/r/85836/#review85168
Attachment #8801025 -
Flags: review?(gpascutto)
Updated•8 years ago
|
Whiteboard: #sbv4-m2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.
https://reviewboard.mozilla.org/r/85836/#review86602
Attachment #8801025 -
Flags: review?(gpascutto) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8802396 [details]
Bug 1305780 - P2. Testcase for update fail.
https://reviewboard.mozilla.org/r/86790/#review86606
Attachment #8802396 -
Flags: review?(gpascutto) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
This is an instance that would cause update failure.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 17•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e05100c3ca4c
P1. Implement the update fail scheme for v4. r=gcp
https://hg.mozilla.org/integration/autoland/rev/178ed8fe0e87
P2. Testcase for update fail. r=gcp
Keywords: checkin-needed
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e05100c3ca4c
https://hg.mozilla.org/mozilla-central/rev/178ed8fe0e87
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•