1374851 - about:url-classifier doesn't show the back-off time

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

Attachment: backoff-time-na.png

The back-off time column of about:url-classifier doesn't seem to be working correctly.

It always shows "N/A" even though the status of a provider is "cannot update" (see attached screenshot).

The code suggests that "cannot update" is mostly returned when the back-off is active:

https://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/toolkit/content/aboutUrlClassifier.js#181

but maybe there's another reason why my updates are failing?

listmanager: 23:28:33 GMT+0000 (UTC): checkForUpdates with

Comment 1

[Dimi Lee [:dimi]]

Hi francois,
From the debug message in your description it seems it fails in the following line:
https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#318

For some reason the updateUrl is empty, from my experience it is mostly because preference or google API key.
Could you help check it ? Thanks!

Comment 2

[François Marier [:francois]]

The pref is set to the default value:

browser.safebrowsing.provider.google.updateURL;https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%MAJOR_VERSION%&pver=2.2&key=%GOOGLE_API_KEY%

and the API key is fine because the google4 updates work fine.

I took today's Nightly and added these:

urlclassifier.malwareTable += "goog-malware-shavar"
urlclassifier.phishTable += "goog-phish-shavar"

Comment 3

[François Marier [:francois]]

For some reason, the scheduled updates seem to work but not the manually triggered ones:

listmanager: 00:10:05 GMT+0000 (UTC): update success for goog-phish-shavar,goog-malware-shavar from https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=56.0a&pver=2.2&key=[trimmed-google-api-key]: 1793
listmanager: 00:10:05 GMT+0000 (UTC): Waiting 1793000ms
listmanager: 00:10:05 GMT+0000 (UTC): Setting last update of google to 1498003805800
listmanager: 00:10:05 GMT+0000 (UTC): Setting next update of google to 1498005598800 (1793000ms from now)
listmanager: 00:11:46 GMT+0000 (UTC): checkForUpdates with

Comment 4

[Dimi Lee [:dimi]]

Ah...I found the root cause.
It is because in aboutUrlClassifier.js I use the first table in "provider.lists" and get the updateUrl of the table by using listmanager API. Since we removed the V2 tables in bug 1366920, listmanager could not find the updateUrl of "the first table" (goog-badbinurl-shavar[1]), updateUrl is empty and trigger update manually will fail.

A faster fix for this will be iterate through all the tables in "provider.lists" until we find an updateUrl in aboutUrlClassifier.js.
Otherwise we will have to add a "provider" to "updateUrl" API.

francois, thanks for finding this problem!

[1] https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#5260

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1374851 - about:url-classifier should not use hard-coded table to get update URL.

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

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8879768 [details]
Bug 1374851 - about:url-classifier should not use hard-coded table to get update URL.

https://reviewboard.mozilla.org/r/151132/#review155980

::: toolkit/content/aboutUrlClassifier.js:178
(Diff revision 1)
>      let listmanager = Cc["@mozilla.org/url-classifier/listmanager;1"]
>                        .getService(Ci.nsIUrlListManager);
>  
>      let pref = "browser.safebrowsing.provider." + provider + ".lists";
> -    let table = Services.prefs.getCharPref(pref, "").split(",")[0];
> +    let tables = Services.prefs.getCharPref(pref, "").split(",");
> +    let table = tables.find(t => listmanager.getUpdateUrl(t) != "");

I didn't check |table| and reduce additional |getUpdateUrl| here because I want to make the code more clear.

getUpdateUrl API will return empty updateUrl when table is empty.

Comment 7

[François Marier [:francois]]

Comment on attachment 8879768 [details]
Bug 1374851 - about:url-classifier should not use hard-coded table to get update URL.

https://reviewboard.mozilla.org/r/151132/#review156006

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5259dd001b15
about:url-classifier should not use hard-coded table to get update URL. r=francois

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5259dd001b15