Closed Bug 1281083 Opened 8 years ago Closed 8 years ago

Changing the urlclassifier.*Table prefs doesn't take effect before the next browser restart

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dlee, Assigned: dlee)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(2 files, 7 obsolete files)

12.72 KB, patch
dlee
: review+
Details | Diff | Splinter Review
9.45 KB, patch
dlee
: review+
Details | Diff | Splinter Review
Tables are defined in different urlclassifier preferences, for example, urlclassifier.phishTable, urlclassifier.malwareTable ... etc
But updating those preferences will not re-register update url and gethash url for corresponding table in listmanager.
No longer blocks: 1272239
Blocks: 1272239
Summary: urlclassifier tables do no reset after updating preference → change urlclassifier tables in preference doesn't refresh corresponding parameters
Hi Francois,
I didn't really test if the full page should work after updating preference as discussed in work week. Because mainly the patch just make sure gethash/update url[1] and enableUpdate[2] will update after changing preference. I want to make testcase simpler, please let me know if you have any concern, thx!

[1]https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#78
[2]https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#248
Attachment #8763820 - Flags: review?(francois)
> I didn't really test if the full page should work after updating preference
> as discussed in work week. Because mainly the patch just make sure
> gethash/update url[1] and enableUpdate[2] will update after changing
> preference. I want to make testcase simpler, please let me know if you have
> any concern, thx!
> 
I think i will still add the testcase we discussed in workweek because we are going to resolve bug 1272528.
I will upload the testcase tomorrow.
Comment on attachment 8763820 [details] [diff] [review]
P2. testcase for classifier table preferences change.

Sorry forget obsolete this because i am still writing testcase
Attachment #8763820 - Attachment is obsolete: true
Attachment #8763820 - Flags: review?(francois)
The testcase is not as easy as i expected...

The problem is that in testcase even i use the trick(set nextupdatetime to 1) to trigger update, we still cannot know when the "update" is complete(written to database). So after loading a page with tracker url, if the url is blocked or not may depend on when the update is complete.
Also i don't think use timer between update and loading page is a good idea...
Noting that once this is fixed, we'll be able to remove the forced restart when switching between the standard and strict tracking protection lists.
Summary: change urlclassifier tables in preference doesn't refresh corresponding parameters → Changing the urlclassifier.*Table prefs doesn't take effect before the next browser restart
Comment on attachment 8763818 [details] [diff] [review]
P1. change urlclassifier tables in preference doesn't refresh corresponding parameters

Review of attachment 8763818 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +66,5 @@
>      }
>  
>      Services.prefs.addObserver("browser.safebrowsing", this.readPrefs.bind(this), false);
>      Services.prefs.addObserver("privacy.trackingprotection", this.readPrefs.bind(this), false);
> +    for (tablePref of tablePreferences) {

How about simply adding an observer on "urlclassifier"?

Wouldn't that watch all of the urlclassifier.* prefs automatically?

@@ -286,3 @@
>        if (this.trackingEnabled) {
> -        listManager.enableUpdate(trackingProtectionLists[i]);
> -        listManager.enableUpdate(trackingProtectionWhitelists[i]);

Oh, there is a bug in here!

We're going through all of the elements in the trackingProtectionLists array and then also using the same "i" to index into a different array (trackingProtectionWhiltelists). This will be a problem when the two arrays have a different length (e.g. when bug 1258041 lands).

I will file a new P1 bug for this and fix it separately so that we can uplift. Of course that means you'll need to rebase your patch once mine has landed.
Attachment #8763818 - Flags: review?(francois)
Depends on: 1285428
Comment on attachment 8768272 [details] [diff] [review]
P2. Testcase for changing the urlclassifier.*Table prefs

Review of attachment 8768272 [details] [diff] [review]:
-----------------------------------------------------------------

I very much like the approach in this test.

I do however see a problem: if the pref change and table update fails to happen, tracking.example.com will still get blocked because it's set in https://dxr.mozilla.org/mozilla-central/rev/94cce4e79310565ff5b169f890842499ea713f8a/toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm#18.

I would suggest using a different url (e.g. bug1281083.tracking.com) and then having a simpler version of classifiedAnnotatedFrame.html that only checks that one of the resource types (e.g. script) is blocked. That way, the check should not pass unless the update has happened successfully.

::: toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref.html
@@ +29,5 @@
> +
> +// If default preference contain the table we want to test,
> +// We should change test table to a different one.
> +var trackingTables = SpecialPowers.getCharPref("urlclassifier.trackingTable").split(",");
> +ok(!trackingTables.includes(testTable), "test table should not be in the preference");

It's a really good idea to enforce this!

@@ +82,5 @@
> +             fullhashParam, true);
> +    xhr.setRequestHeader("Content-Type", "text/plain");
> +    xhr.onreadystatechange = function() {
> +      if (this.readyState == this.DONE) {
> +        dump("[Dmi]XHR ok...\n");

dump("[Dimi]...")

@@ +115,5 @@
> +   * In this test we try to modify only urlclassifier.*Table preferenceto see if
> +   * url specified in the table will be blocked after update.
> +   */
> +  SpecialPowers.pushPrefEnv(
> +    {"set" : [["urlclassifier.trackingTable", testTable]]});

Are you sure this works? It doesn't look like you're actually waiting for this pushPrefEnv() to finish.

I think you probably want to use a callback here like you did on line 138.

@@ +123,5 @@
> +      loadTestFrame();
> +    });
> +}
> +
> +// Call by allowlistedAnnotatedFrame.html.

Do you mean "classifiedAnnotatedFrame.html" like on line 56?

nit: I would say "Called by" or "Required by".

::: toolkit/components/url-classifier/tests/mochitest/update.sjs
@@ +42,5 @@
> +  }
> +
> +  var responseBody = parseV2Request(bytes);
> +
> +  dump("[Dimi]responseBody = " + responseBody + "\n");

Did you mean to leave this in?

There are a couple more `dump("[Dimi]...")` in this file.

@@ +83,5 @@
> +    41,42,43,44, 45,46,47,48, 49,50,51,-1, -1,-1,-1,-1
> +];
> +const base64Pad = '=';
> +
> +function base64ToString(data) {

nit: it might be good to move this function into a test utils file. I'm sure I've either seen this before or that we'll use it in other tests.
Attachment #8768272 - Flags: review?(francois) → review-
Sorry about forgetting to remove my debug message...

(In reply to François Marier [:francois] from comment #9)
> @@ +115,5 @@
> > +   * In this test we try to modify only urlclassifier.*Table preferenceto see if
> > +   * url specified in the table will be blocked after update.
> > +   */
> > +  SpecialPowers.pushPrefEnv(
> > +    {"set" : [["urlclassifier.trackingTable", testTable]]});
> 
> Are you sure this works? It doesn't look like you're actually waiting for
> this pushPrefEnv() to finish.
> 
> I think you probably want to use a callback here like you did on line 138.

Thanks for finding this! I forget to add the callback.
I think the testcase passed in my local testing just because of timing...
- rebase latest code
- modify observer to listen domain "urlclassifier" instead of listen to each "urlclassifier.*table"
Attachment #8763818 - Attachment is obsolete: true
Attachment #8770063 - Flags: review?(francois)
Address Comment 9
Attachment #8768272 - Attachment is obsolete: true
Attachment #8770064 - Flags: review?(francois)
Hi Francois,
In Pathc P1.
The reason that I didn't listen to domain |urlclassifier| is to avoid listening preference changes are not actually handled in SafeBrowsing.jsm, for example, "urlclassifier.gethashnoise".
But the way you suggest make the code more clear, so i think either way it works for me.
Please let me know which way you prefer.

In Patch P2 about merging |base64ToString| function in sjs.
I can't find a good way to import utility module in sjs unless I added an JSM under "toolkit/module/".
But it doesn't look likes a good place for module just for testing. Also I try to import UrlClassifierTestUtils.jsm in sjs but it doesn't work.
I guess there will only be two sjs(gethash.sjs and update.sjs require this function) so maybe we can leave it there for now. If there are more files require this function in the future then we can figure out a way to do it.
How do you think ?
Flags: needinfo?(francois)
(In reply to Dimi Lee[:dimi][:dlee] from comment #13)
> The reason that I didn't listen to domain |urlclassifier| is to avoid
> listening preference changes are not actually handled in SafeBrowsing.jsm,
> for example, "urlclassifier.gethashnoise".
> But the way you suggest make the code more clear, so i think either way it
> works for me.
> Please let me know which way you prefer.

You're right that it may not always be unnecessary. On balance however, given that most of the urlclassifier.* prefs affect SafeBrowsing.jsm, I think it's OK to listen to all of these prefs regardless. So my preference would be for the clearer code.

> I guess there will only be two sjs(gethash.sjs and update.sjs require this
> function) so maybe we can leave it there for now. If there are more files
> require this function in the future then we can figure out a way to do it.
> How do you think ?

That sounds good to me. We can refactor this later.
Flags: needinfo?(francois)
Attachment #8770063 - Flags: review?(francois) → review+
Comment on attachment 8770064 [details] [diff] [review]
P2. Testcase for changing the urlclassifier.*Table prefs. v2

Review of attachment 8770064 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/tests/mochitest/bug_1281083.html
@@ +30,5 @@
> +
> +</head>
> +
> +<body onload="checkLoads()">
> +The following should not be hidden:

You can remove this line as well as the next one with the <div>. They are only relevant for the stylesheet blocking tests.
Attachment #8770064 - Flags: review?(francois) → review+
Address nits - remove unused div block in testcase
Attachment #8770064 - Attachment is obsolete: true
Attachment #8771812 - Flags: review+
rebase code
Attachment #8770063 - Attachment is obsolete: true
Attachment #8771812 - Attachment is obsolete: true
Attachment #8772221 - Flags: review+
wrong bug description for last patch, update it
Attachment #8772221 - Attachment is obsolete: true
Attachment #8772229 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/158b91a92d4c
P1. Changing the urlclassifier.*Table prefs doesn't take effect before the next browser restart. r=francois
https://hg.mozilla.org/integration/fx-team/rev/a0266bb6db01
P2. Testcase for changing the urlclassifier.*Table. r=francois
Keywords: checkin-needed
Whiteboard: #sbv4-m0
https://hg.mozilla.org/mozilla-central/rev/158b91a92d4c
https://hg.mozilla.org/mozilla-central/rev/a0266bb6db01
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: