1522265 - Moving malware, phishing and blocked URIs to features

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

Malware, phishing and blockedURIs are 3 classifiers which have not been ported to features yet. I have 3 patches to do it. This is not clean as much as I would like to, but it's a good starting point.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - features

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - factory

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - DBService

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - features

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - features

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 9039517 [details] [diff] [review]
part 1 - features

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

Take a quick look and I guess another part1 feature is duplicate?

Comment 7

[Dimi Lee [:dimi]]

Comment on attachment 9038653 [details] [diff] [review]
part 2 - factory

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

I think you want to update this patch because it still uses the old for loop

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - factory

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 9038654 [details] [diff] [review]
part 3 - DBService

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

LGTM! baku, thank you for working on this!

::: toolkit/components/url-classifier/Classifier.cpp
@@ -406,5 @@
>  
> -nsresult Classifier::CheckURI(const nsACString& aSpec,
> -                              const nsTArray<nsCString>& aTables,
> -                              LookupResultArray& aResults) {
> -  Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_CL_CHECK_TIME> timer;

move this telemetry to FeatureHolder::DoLocalLookup to keep monitoring the lookup time.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1700,5 @@
>  
>    // XXX: Do we *really* need to be able to change all of these at runtime?
>    // Note: These observers should only be added when everything else above has
>    //       succeeded. Failing to do so can cause long shutdown times in certain
>    //       situations. See Bug 1247798 and Bug 1244803.

The comment can be removed.

@@ +1770,5 @@
>  
>    NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
>  
> +  nsTArray<RefPtr<nsIUrlClassifierFeature>> features;
> +  mozilla::net::UrlClassifierFeatureFactory::GetFeaturesNoChannel(features);

This looks strange to me, I think it is more clear that we use
UrlClassifierFeaturePhishingProtection and add a comment about Phishing Protection feature doesn't require a channel.

The reason I suggest use PhisingProtection instead of SafeBrowsing is because : "The Safe Browsing feature in Firefox has been renamed to Phishing Protection, but it's still known as Safe Browsing internally[1]"

[1] https://wiki.mozilla.org/Security/Safe_Browsing

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h
@@ +134,5 @@
>    // processed.
>    bool mInUpdate;
>  
>    // The list of tables that can use the default hash completer object.
>    nsTArray<nsCString> mGethashTables;

We can also remove this now

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 4 - Phishing protection

Just renamed NoChannel to PhishingProtection.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a0832f3b11
Moving malware, phishing and blocked URIs to features - part 1 - feature no channel, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/6085d51681f8
Moving malware, phishing and blocked URIs to features - part 2 - factory updated, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b824df9d02
Moving malware, phishing and blocked URIs to features - part 3 - DBService updated, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b4179568c7
Moving malware, phishing and blocked URIs to features - part 4 - Phishing Protection, r=dimi

Comment 12

[Arthur Iakab [arthur_iakab]]

Backed out 4 changesets (bug 1522265) for mochitest assertion failures on extensions/cookie/nsPermissionManager.cpp CLOSED TREE

Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/98040e23391e4cf7fe75d7da71f53ec2b2a2f28a

Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=224634754&revision=00a1fd64f704a8e73502f5857b1c47b1ef0363ef

Log failures https://treeherder.mozilla.org/logviewer.html#?job_id=224673394&repo=mozilla-inbound

:baku could you please take a look?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4973824e42
Moving malware, phishing and blocked URIs to features - part 1 - feature no channel, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/32eedda05a80
Moving malware, phishing and blocked URIs to features - part 2 - factory updated, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7f6aeaa6de
Moving malware, phishing and blocked URIs to features - part 3 - DBService updated, r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d216ba3e15
Moving malware, phishing and blocked URIs to features - part 4 - Phishing Protection, r=dimi

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0986d3e0a4
Moving malware, phishing and blocked URIs to features - part 5 - Fix a method name call, r=me CLOSED TREE

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ce4973824e42
https://hg.mozilla.org/mozilla-central/rev/32eedda05a80
https://hg.mozilla.org/mozilla-central/rev/ac7f6aeaa6de
https://hg.mozilla.org/mozilla-central/rev/c3d216ba3e15
https://hg.mozilla.org/mozilla-central/rev/7b0986d3e0a4