Closed
Bug 1395411
Opened 7 years ago
Closed 7 years ago
Removing lists from urlclassifier.*Table doesn't remove them from the update checker
Categories
(Toolkit :: Safe Browsing, defect, P1)
Toolkit
Safe Browsing
Tracking
()
VERIFIED
FIXED
mozilla57
People
(Reporter: francois, Assigned: tnguyen)
References
Details
(Whiteboard: #sbv4-m9)
Attachments
(2 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
10.32 KB,
patch
|
francois
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Steps: 1. Create a new profile 2. Go into about:config and set the following: pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple"); pref("urlclassifier.phishTable", "goog-phish-shavar,test-phish-simple"); pref("urlclassifier.downloadAllowTable", "goog-downloadwhite-digest256"); pref("urlclassifier.downloadBlockTable", "goog-badbinurl-shavar"); 3. Go into about:url-classifier and trigger an update of google4 Expected: The google4 cannot update because it doesn't have any lists associated with it. Actual: Both the google and google4 providers can happily requests updates from the server. (Note that if you reboot between steps 2 and 3, everything is as expected.) What this means is that when we re-register tables, we correctly register new tables, but we don't clear the removed ones.
Reporter | ||
Comment 1•7 years ago
|
||
Thomas, can you take a look at this to see if it can easily be fixed? If so, we'll uplift it because it's blocking the crash rate study on beta (bug 1377267).
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Whiteboard: #sbv4-m9
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to François Marier [:francois] from comment #1) > Thomas, can you take a look at this to see if it can easily be fixed? > > If so, we'll uplift it because it's blocking the crash rate study on beta > (bug 1377267). We have to find a way to create share tables between listmanager and SafeBrowsing.jsm (or compare tables of them and filter out obsoleted tables) For the long term, I would suggest moving all prefs reading and table lists to listmanager because the tables in SafeBrowsing.jsm are only used for updating SB list
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3) > Created attachment 8903117 [details] > Bug 1395411 - remove table from listmanager if the table is removed from > urlclassifier.*Table pref > > Review commit: https://reviewboard.mozilla.org/r/174914/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/174914/ Basic idea, I will verify and revise it tomorrow
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8903117 [details] Bug 1395411 - Unregister tables when they're removed from urlclassifier.*Table. https://reviewboard.mozilla.org/r/174914/#review180202 I like the approach. Just have a few minor comments. ::: commit-message-d10c9:1 (Diff revision 1) > +Bug 1395411 - remove table from listmanager if the table is removed from urlclassifier.*Table pref Suggestion: "Unregister tables when they're removed from urlclassifier.*Table." ::: toolkit/components/url-classifier/SafeBrowsing.jsm:239 (Diff revision 1) > > let flashAllowTable, flashAllowExceptTable, flashTable, > flashExceptTable, flashSubDocTable, > flashSubDocExceptTable; > > + let obsoletedLists; nit: "obsolete" ::: toolkit/components/url-classifier/SafeBrowsing.jsm:239 (Diff revision 1) > > let flashAllowTable, flashAllowExceptTable, flashTable, > flashExceptTable, flashSubDocTable, > flashSubDocExceptTable; > > + let obsoletedLists; I would add a comment here: // Make a copy of the original lists before we re-read the prefs. ::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:113 (Diff revision 1) > return true; > } > > /** > + * Unregister a table table from list > + * @param tableName - the name of the table nit: you don't need to describe the parameter in a comment if it has a good explicit name :) ::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:119 (Diff revision 1) > + */ > +PROT_ListManager.prototype.unregisterTable = function(tableName) { > + log("unregistering " + tableName); > + var table = this.tablesData[tableName]; > + if (table) { > + this.needsUpdate_[table.updateUrl][tableName] = false; Do we need to keep the table in this array or could we remove `tablename` from `this.needsUpdate_[table.updateUrl]`? ::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:121 (Diff revision 1) > + log("unregistering " + tableName); > + var table = this.tablesData[tableName]; > + if (table) { > + this.needsUpdate_[table.updateUrl][tableName] = false; > + } > + this.tablesData[tableName] = {}; Why not this? delete this.tablesData[tableName];
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8903117 [details] Bug 1395411 - Unregister tables when they're removed from urlclassifier.*Table. https://reviewboard.mozilla.org/r/174914/#review180780 ::: toolkit/components/url-classifier/tests/mochitest/chrome.ini:30 (Diff revision 2) > [test_trackingprotection_whitelist.html] > tags = trackingprotection > [test_safebrowsing_bug1272239.html] > [test_donottrack.html] > [test_classifier_changetablepref.html] > +[test_classifier_changetablepref-1.html] `-1` is not a very descriptive name. You could say `test_classifier_changetablepref_bug1395411`. ::: toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref-1.html:1 (Diff revision 2) > +<!DOCTYPE HTML> Missing public domain header: https://www.mozilla.org/en-US/MPL/headers/
Attachment #8903117 -
Flags: review?(francois) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Oh, it seems be broken in asan or debug builds because missing googleAPI key
Assignee | ||
Comment 10•7 years ago
|
||
Seems it is not a good idea to use "google", "google4" provider in the test because we have some missing GAPI key issues on various test machines. Changing to use mozilla, mozilla4 is good enough to cover the test.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15200fa7bf64a9fcc6b9bbeeb89e2ad6e3ca74f
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•7 years ago
|
||
Comment hidden (obsolete) |
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7bb00f5e0120 Unregister tables when they're removed from urlclassifier.*Table. r=francois
Keywords: checkin-needed
Comment 16•7 years ago
|
||
Backed out for eslint failures in test_classifier_changetablepref_bug1395411.html: 'classifierHelper' is not defined: https://hg.mozilla.org/integration/autoland/rev/7bb00f5e01201380ca9d9afd68db1738f39c9380 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128290836&repo=autoland TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref_bug1395411.html:43:11 | 'classifierHelper' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref_bug1395411.html:61:11 | 'classifierHelper' is not defined. (no-undef)
Flags: needinfo?(tnguyen)
Comment 17•7 years ago
|
||
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/bfb3d1d3cc85 Backed out changeset 7bb00f5e0120 for eslint failures in test_classifier_changetablepref_bug1395411.html: 'classifierHelper' is not defined. r=backout
Assignee | ||
Updated•7 years ago
|
Attachment #8904142 -
Attachment is obsolete: true
Flags: needinfo?(tnguyen)
Attachment #8904142 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 18•7 years ago
|
||
That's interesting that I have to add a comment "/* import-globals-from classifierHelper.js */" above of the test to pass lint check
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8904401 -
Attachment description: Unregister tables when they're removed from urlclassifier.*Table → Beta - Unregister tables when they're removed from urlclassifier.*Table
Comment hidden (obsolete) |
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1f0f12bf3a3a Unregister tables when they're removed from urlclassifier.*Table. r=francois
Keywords: checkin-needed
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f0f12bf3a3a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter | ||
Comment 25•7 years ago
|
||
Comment on attachment 8904401 [details] [diff] [review] Beta - Unregister tables when they're removed from urlclassifier.*Table Review of attachment 8904401 [details] [diff] [review]: ----------------------------------------------------------------- This patch won't work on Beta. ::: toolkit/components/url-classifier/SafeBrowsing.jsm @@ +234,5 @@ > + obsoleteLists = [this.phishingLists, > + this.malwareLists, > + this.downloadBlockLists, > + this.downloadAllowLists, > + this.passwordAllowLists, Needs to be removed, it doesn't exist on Beta. @@ +268,5 @@ > + let newLists = [this.phishingLists, > + this.malwareLists, > + this.downloadBlockLists, > + this.downloadAllowLists, > + this.passwordAllowLists, ditto
Attachment #8904401 -
Flags: review-
Reporter | ||
Comment 26•7 years ago
|
||
Comment on attachment 8904401 [details] [diff] [review] Beta - Unregister tables when they're removed from urlclassifier.*Table Thomas, can you update the patch, test it on Beta and then re-request the uplift please?
Flags: needinfo?(tnguyen)
Attachment #8904401 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 27•7 years ago
|
||
Oh, password protection list is not in beta, thanks for finding this.
Flags: needinfo?(tnguyen)
Assignee | ||
Comment 28•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8904401 -
Attachment is obsolete: true
Assignee | ||
Comment 29•7 years ago
|
||
Comment on attachment 8904815 [details] [diff] [review] Unregister tables when they're removed from urlclassifier.*Table Approval Request Comment [Feature/Bug causing the regression]: No [User impact if declined]: it's blocking the crash rate study on beta. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: Comment 0 [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: no [Why is the change risky/not risky?]: The logic is fairly simple. We compare the old and new preferences, collect obsolete lists and unregister them [String changes made/needed]: No
Attachment #8904815 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Reporter | ||
Comment 30•7 years ago
|
||
Comment on attachment 8904815 [details] [diff] [review] Unregister tables when they're removed from urlclassifier.*Table Review of attachment 8904815 [details] [diff] [review]: ----------------------------------------------------------------- It applies cleanly on beta, it works fine, and it passes all of the tests in toolkit/components/url-classifier/tests/mochitest.
Attachment #8904815 -
Flags: review+
Comment 31•7 years ago
|
||
Hi Timea: Would you also kindly help verify this issue in Nightly 57 with the steps in comment 0, to facilitate the beta uplift? Thanks! (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #29) > Approval Request Comment > [Has the fix been verified in Nightly?]: No > [Needs manual test from QE? If yes, steps to reproduce]: Comment 0
Flags: needinfo?(timea.zsoldos)
Comment 32•7 years ago
|
||
I have reproduced this issue using Firefox 56.0b9 (2017.09.02.03, buildID: 20170903140023) because the fix doesn't there yet, on Win 8.1 x64. I can confirm this issue is fixed, I verified using Firefox 57.0a1 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12, the Last update status for google4 is: download error (400).
Flags: needinfo?(timea.zsoldos)
Comment 33•7 years ago
|
||
thanks Timea! Gerry, would you kindly approve the uplift? Thanks!
Flags: needinfo?(gchang)
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Timea Zsoldos from comment #32) > Ubuntu 14.04 x64 and Mac OS X 10.12, the Last update status for google4 is: > download error (400). That is basically we are missing google api key.
Comment 35•7 years ago
|
||
Comment on attachment 8904815 [details] [diff] [review] Unregister tables when they're removed from urlclassifier.*Table Fix the update list issue for safe browsing and was verified. Beta56+.
Flags: needinfo?(gchang)
Attachment #8904815 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 36•7 years ago
|
||
(In reply to Timea Zsoldos from comment #32) > I can confirm this issue is fixed, I verified using Firefox 57.0a1 8.1 x64, Thanks for the verification effort, Timea!
Comment 37•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/79c631797f0d
Flags: in-testsuite+
Comment 38•7 years ago
|
||
I can confirm this issue is fixed on 56.0b10, I verified using Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12, the Last update status for google4 is: "cannot update" and for google is: "success".
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•