Closed Bug 1395411 Opened 2 years ago Closed 2 years ago

Removing lists from urlclassifier.*Table doesn't remove them from the update checker

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: francois, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m9)

Attachments

(2 files, 2 obsolete files)

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.
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
(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
(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
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 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+
Oh, it seems be broken in asan or debug builds because missing googleAPI key
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.
Keywords: checkin-needed
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
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)
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
Attachment #8904142 - Attachment is obsolete: true
Flags: needinfo?(tnguyen)
Attachment #8904142 - Flags: approval-mozilla-beta?
That's interesting that I have to add a comment "/* import-globals-from classifierHelper.js */" above of the test to pass lint check
Keywords: checkin-needed
Attachment #8904401 - Attachment description: Unregister tables when they're removed from urlclassifier.*Table → Beta - Unregister tables when they're removed from urlclassifier.*Table
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
https://hg.mozilla.org/mozilla-central/rev/1f0f12bf3a3a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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-
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?
Oh, password protection list is not in beta, thanks for finding this.
Flags: needinfo?(tnguyen)
Attachment #8904401 - Attachment is obsolete: true
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?
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+
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)
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)
thanks Timea!

Gerry, would you kindly approve the uplift? Thanks!
Flags: needinfo?(gchang)
(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 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+
(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!
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".
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.