Closed
Bug 1434662
Opened 6 years ago
Closed 6 years ago
Ensure that partially failed updates are handled properly
Categories
(Toolkit :: Safe Browsing, enhancement, P1)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: francois, Assigned: francois)
References
Details
Attachments
(3 files)
GCP pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1431370#c2 that we don't return an error during a Safe Browsing update unless all of the responses failed. We should ensure that this doesn't lead to undesirable things like updates getting stuck or the previous (working) version of a list being blown away.
Assignee | ||
Comment 1•6 years ago
|
||
I ran through the following test: 1. rm -rf obj-x86_64-pc-linux-gnu/tmp/scratch_user/safebrowsing/google4/ 2. Start Firefox and open about:url-classifier 3. Trigger an update for google4 4. Shut down Firefox 5. Apply this patch: --- a/toolkit/components/url-classifier/ProtocolParser.cpp +++ b/toolkit/components/url-classifier/ProtocolParser.cpp @@ -826,16 +826,21 @@ ProtocolParserProtobuf::ProcessOneRespon } } if (listName.IsEmpty()) { PARSER_LOG(("We received an update for a list we didn't ask for. Ignoring it.")); return NS_ERROR_FAILURE; } + // TODO: remove this hack! + if (listName.EqualsLiteral("goog-badbinurl-proto")) { + return NS_ERROR_FAILURE; + } + // Test if this is a full update. bool isFullUpdate = false; if (aResponse.has_response_type()) { isFullUpdate = aResponse.response_type() == ListUpdateResponse::FULL_UPDATE; } else { NS_WARNING("Response type not initialized."); return NS_ERROR_UC_PARSER_MISSING_PARAM; 6. Start Firefox and open about:url-classifier 7. Trigger an update for google4 8. Open http://testsafebrowsing.appspot.com 9. Test the phishing and malware pages. The fact that the test pages in Step 9 worked fine confirms that a single failing table (goog-badbinurl-proto in this case) won't interfere with the other Safe Browsing tables that did update correctly. I also repeated the above test but skipping steps 2-4 in order to confirm that a failing table also won't prevent the other tables from being downloaded and used successfully even in the case of an empty profile (i.e. initial list download).
Assignee | ||
Comment 2•6 years ago
|
||
I tested whether or not disk corruption (i.e. pset doesn't match the checksum) will cause a table to be reset. Here's how I did it: 1. Clear Safe Browsing cache directory. 2. Start Firefox and force a successful update of the google4 provider using about:url-classifier. 3. Close Firefox. 4. cp goog-phish-proto.pset goog-malware-proto.pset 5. Start Firefox with MOZ_LOG=UrlClassifierPrefixSet:5,UrlClassifierDbService:5,UrlClassifierProtocolParser:5 I got the following log: [17159:URL Classifier]: D/UrlClassifierDbService Private store directory for goog-malware-proto is /home/francois/devel/mozilla-unified/obj-x86_64-pc-linux-gnu/tmp/scratch_user/safebrowsing/google4 [17159:URL Classifier]: D/UrlClassifierDbService Loading PrefixSet for goog-malware-proto [17159:URL Classifier]: D/UrlClassifierDbService stored PrefixSet exists, loading from disk [17159:URL Classifier]: D/UrlClassifierPrefixSet Loading PrefixSet successful ++DOCSHELL 0x7f6b8586b800 == 5 [pid = 17159] [id = {8fe46ebb-4393-4321-989e-29913e606ae9}] ++DOMWINDOW == 10 (0x7f6b8584ec00) [pid = 17159] [serial = 10] [outer = (nil)] ++DOMWINDOW == 11 (0x7f6b85850400) [pid = 17159] [serial = 11] [outer = 0x7f6b8584ec00] ++DOMWINDOW == 12 (0x7f6b85853800) [pid = 17159] [serial = 12] [outer = 0x7f6b8584ec00] ++DOMWINDOW == 13 (0x7f6b85855000) [pid = 17159] [serial = 13] [outer = 0x7f6b8584ec00] [17159:URL Classifier]: D/UrlClassifierDbService Checksum mismatch when loading prefixes from file. [Parent 17159, URL Classifier] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8052000B: file /home/francois/devel/mozilla-unified/toolkit/components/url-classifier/LookupCache.cpp, line 84 [17159:URL Classifier]: D/UrlClassifierDbService Reset() is called so we interrupt the update. The above Reset() was actually for the whole provider. If we detect disk corruption, we assume that other lists in the same directory might be bad too.
Assignee | ||
Comment 3•6 years ago
|
||
Looking into the results from comment 1 further, I noticed that failing tables don't get reset and so what happens is that the previous version of the database is used for any table that fails to update. I confirmed this by making goog-malware-proto fail and then testing the malware page on http://testsafebrowsing.appspot.com and making sure that it's still blocked even after a partially failed update. This mean that if a table gets itself into a bad state then it may never be able to apply a partial update and could just keep failing silently forever. So we should in fact reset any table (but just that table) that fails to update while continuing to report success for the update as whole as long as some of the tables got updated successfully.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
I tested my patch manually by repeating the steps in comment 3: 1. rm -rf obj-x86_64-pc-linux-gnu/tmp/scratch_user/safebrowsing/google4/ 2. Start Firefox and open about:url-classifier 3. Trigger an update for google4 4. Shut down Firefox 5. Apply this patch: --- a/toolkit/components/url-classifier/ProtocolParser.cpp +++ b/toolkit/components/url-classifier/ProtocolParser.cpp @@ -833,16 +833,21 @@ ProtocolParserProtobuf::ProcessOneRespon } } if (aListName.IsEmpty()) { PARSER_LOG(("We received an update for a list we didn't ask for. Ignoring it.")); return NS_ERROR_FAILURE; } + // TODO: remove this hack! + if (aListName.EqualsLiteral("goog-malware-proto")) { + return NS_ERROR_FAILURE; + } + // Test if this is a full update. bool isFullUpdate = false; if (aResponse.has_response_type()) { isFullUpdate = aResponse.response_type() == ListUpdateResponse::FULL_UPDATE; } else { NS_WARNING("Response type not initialized."); return NS_ERROR_UC_PARSER_MISSING_PARAM; 6. Open http://testsafebrowsing.appspot.com 7. Test the phishing and malware pages and confirm they get blocked. 8. Start Firefox and open about:url-classifier 9. Trigger an update for google4 10. Clear cache, history, etc. with Ctrl+Alt+Backspace. 10. Open http://testsafebrowsing.appspot.com 11. Test the phishing and malware pages and confirm that the phishing page is blocked but not the malware one.
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8969507 [details] Bug 1434662 - Reset Safe Browsing V4 tables that fail to update. https://reviewboard.mozilla.org/r/238270/#review244556 ::: toolkit/components/url-classifier/ProtocolParser.cpp:837 (Diff revision 1) > break; > } > } > > - if (listName.IsEmpty()) { > + if (aListName.IsEmpty()) { > PARSER_LOG(("We received an update for a list we didn't ask for. Ignoring it.")); Interesting question if you want to reset in this case (and whether the reset won't fail).
Attachment #8969507 -
Flags: review?(gpascutto) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8969508 [details] Bug 1434662 - Remove dead code. https://reviewboard.mozilla.org/r/238272/#review244558
Attachment #8969508 -
Flags: review?(gpascutto) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8969509 [details] Bug 1434662 - Move initialization code to ProtocolParser::Begin(). https://reviewboard.mozilla.org/r/238274/#review244562
Attachment #8969509 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969507 [details] Bug 1434662 - Reset Safe Browsing V4 tables that fail to update. https://reviewboard.mozilla.org/r/238270/#review244556 > Interesting question if you want to reset in this case (and whether the reset won't fail). Yeah, I'm not sure I remember the cases that we found where that was happening. I'm a little weary of doing a reset without digging into this one further.
Comment 13•6 years ago
|
||
Pushed by fmarier@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f652d798a0c Reset Safe Browsing V4 tables that fail to update. r=gcp https://hg.mozilla.org/integration/autoland/rev/39b9f46104ad Remove dead code. r=gcp https://hg.mozilla.org/integration/autoland/rev/983099e546d8 Move initialization code to ProtocolParser::Begin(). r=gcp
Comment 14•6 years ago
|
||
Backed out for android mass failres and OSX failures at /builds/worker/workspace/build/src/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:512 Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=983099e546d8ac5f677203e0ae39365164b1a131 Failure log (android): https://treeherder.mozilla.org/logviewer.html#?job_id=175150314&repo=autoland&lineNumber=2018 Failure log OSX: https://treeherder.mozilla.org/logviewer.html#?job_id=175149345&repo=autoland&lineNumber=38780 Backout: https://hg.mozilla.org/integration/autoland/rev/81983b3a34752f84638fb101e50b809340ddd78d
Flags: needinfo?(francois)
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
Pushed by fmarier@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7bbbf41e9210 Reset Safe Browsing V4 tables that fail to update. r=gcp https://hg.mozilla.org/integration/autoland/rev/e1f41b9f1d44 Remove dead code. r=gcp https://hg.mozilla.org/integration/autoland/rev/c7f6d061d06c Move initialization code to ProtocolParser::Begin(). r=gcp
Assignee | ||
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f3ba9284264b8e2c1d7f16dc9618ecb46ca321e
Flags: needinfo?(francois)
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7bbbf41e9210 https://hg.mozilla.org/mozilla-central/rev/e1f41b9f1d44 https://hg.mozilla.org/mozilla-central/rev/c7f6d061d06c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•