bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Implement the update fail scheme for v4

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dimi, Assigned: dimi)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: #sbv4-m2)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
Probably duplicated of bug 1297330
See Also: → bug 1297330
(Assignee)

Comment 2

2 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

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

2 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.
Blocks: 1167038
No longer blocks: 1305486
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

2 years ago
Assignee: dlee → nobody
Status: ASSIGNED → NEW
Depends on: 1305581
No longer depends on: 1305484
(Assignee)

Updated

2 years ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Blocks: 1305486
No longer blocks: 1167038
Priority: P5 → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 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

2 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

2 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

2 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

2 years ago
Whiteboard: #sbv4-m2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 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

2 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)
Created attachment 8803738 [details]
safebrowsing-updatewreck.zip

This is an instance that would cause update failure.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e05100c3ca4c
https://hg.mozilla.org/mozilla-central/rev/178ed8fe0e87
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1297330
You need to log in before you can comment on or make changes to this bug.