1305780 - Implement the update fail scheme for v4

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [:dimi]]

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.

Comment 1

[Thomas Nguyen (:tnguyen)]

Probably duplicated of bug 1297330

Comment 2

[Dimi Lee [:dimi]]

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

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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

Comment 4

[Dimi Lee [:dimi]]

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

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1305780 - P1. Implement the update fail scheme for v4.

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

Comment 6

[Dimi Lee [:dimi]]

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!

Comment 7

[Dimi Lee [:dimi]]

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

[Gian-Carlo Pascutto [:gcp]]

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

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.

https://reviewboard.mozilla.org/r/85836/#review85168

Comment 10

[Dimi Lee [:dimi]]

Attachment: Bug 1305780 - P2. Testcase for update fail.

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

Comment 11

[Dimi Lee [:dimi]]

Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.

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

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.

https://reviewboard.mozilla.org/r/85836/#review86602

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8802396 [details]
Bug 1305780 - P2. Testcase for update fail.

https://reviewboard.mozilla.org/r/86790/#review86606

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 8801025 [details]
Bug 1305780 - P1. Implement the update fail scheme for v4.

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

Comment 15

[Dimi Lee [:dimi]]

Comment on attachment 8802396 [details]
Bug 1305780 - P2. Testcase for update fail.

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

Comment 16

[Henry Chang [:hchang]]

Attachment: safebrowsing-updatewreck.zip

This is an instance that would cause update failure.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e05100c3ca4c
https://hg.mozilla.org/mozilla-central/rev/178ed8fe0e87