1351147 - Support v4/ThreatHit request in Safe Browsing V4

Status: RESOLVED FIXED

Description

[Thomas Nguyen[:tnguyen] ni? plz]

http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/url-classifier/chromium/safebrowsing.proto#201-246
The protobuf ThreatHit is used to send a report of how client arrived at the unsafe url.

Comment 1

[Thomas Nguyen[:tnguyen] ni? plz]

Created attachment 8873767 [details] [diff] [review]
WIP - Support v4/ThreatHit request in SafeBrowsing V4

Comment 2

[Thomas Nguyen[:tnguyen] ni? plz]

Created attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

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

Comment 3

[Thomas Nguyen[:tnguyen] ni? plz]

Created attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

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

Comment 4

[Thomas Nguyen[:tnguyen] ni? plz]

I tested manually and get 200 OK when trying to send v4/ThreatHits. But I did not add ClientInfo and UserInfo to the request body.
The ClientInfo and UserInfo are included in https://intranet.mozilla.org/SafeBrowsing, but not included in the last Chromium code. Those fields have not added to Safe Browsing spec yet, not sure if google server has supported them yet

Comment 5

[Thomas Nguyen[:tnguyen] ni? plz]

After our discussion, Google server is supposed to support ClientInfo and UserInfo that are not included in Chrome
  // Client-reported identification.
  ClientInfo client_info = 5;

  // Details about the user that encountered the threat.
  message UserInfo {
    // The UN M.49 region code associated with the user's location.
    string region_code = 1;

    // Unique ID stable over a week or two
    bytes user_id = 2;
  }

  // Details about the user that encountered the threat.
  UserInfo user_info = 6;

I see ISO 3166 is what we are using in Firefox, we send a request to Mozilla Location Service server
http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/modules/libpref/init/all.js#5446
And get the response in ISO 3166 standard
https://mozilla.github.io/ichnaea/api/region.html#api-region-latest
Francois, could we change to use ISO 3166 instead of UN M.49?

Comment 6

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #5)
> I see ISO 3166 is what we are using in Firefox, we send a request to Mozilla
> Location Service server
> http://searchfox.org/mozilla-central/rev/
> 20963d7269b1b14d455f47bc0260d0653015bf84/modules/libpref/init/all.js#5446
> And get the response in ISO 3166 standard
> https://mozilla.github.io/ichnaea/api/region.html#api-region-latest
> Francois, could we change to use ISO 3166 instead of UN M.49?

The problem is that using a country code is too specific. Some countries are too small and so these users will stand out. M.49 allows us to send the region instead of the country.

So we would need a mapping from countries to regions.

Comment 7

[Thomas Nguyen[:tnguyen] ni? plz]

(In reply to François Marier [:francois] from comment #6)
> The problem is that using a country code is too specific. Some countries are
> too small and so these users will stand out. M.49 allows us to send the
> region instead of the country.
> 
> So we would need a mapping from countries to regions.

Ah, I see, thanks Francois

Comment 8

[François Marier [:francois]]

The updated ThreatSourceType enum (see https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a TAB_RESOURCE for any redirects encountered while fetching a bad subresource.

Comment 9

[Thomas Nguyen[:tnguyen] ni? plz]

Created attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

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

Comment 10

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146020/diff/1-2/

Comment 11

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146022/diff/1-2/

Comment 12

[Thomas Nguyen[:tnguyen] ni? plz]

(In reply to François Marier [:francois] from comment #8)
> The updated ThreatSourceType enum (see
> https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a
> TAB_RESOURCE for any redirects encountered while fetching a bad subresource.

...
      // A resource loaded within the final TAB_URL.
      TAB_RESOURCE = 4;
It looks like google wants us to send all sub-resource loadings of a warning page (eg all images, iframes, styles on the warning page). Seems quite a lot of things to do

Comment 13

[François Marier [:francois]]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

https://reviewboard.mozilla.org/r/146020/#review170430

Comment 14

[François Marier [:francois]]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

https://reviewboard.mozilla.org/r/146022/#review170434

Thank you for breaking this up into a separate commit!

I'm thinking that we should probably use something like `aFullHash` instead of `aCompletion`. Completions are V2 jargon and it makes the code harder to understand for anybody outside of our team.

::: commit-message-756c9:1
(Diff revision 2)
> +Bug 1351147 - Use completion instead of prefix in OnClassifyComplete

Can you describe in a few words why this change is needed?

Also, because a given prefix can map to multiple full hashes, I think it's worth pointing out that we can provide the completion here because the (list, provider, completion) tuple is only filled in when the URL matches. Is that right?

::: netwerk/base/nsIClassifiedChannel.idl:26
(Diff revision 2)
>     * @param aList
>     *        Name of the Safe Browsing list that matched (e.g. goog-phish-shavar).
>     * @param aProvider
>     *        Name of the Safe Browsing provider that matched (e.g. google)
> -   * @param aPrefix
> -   *        Hash prefix of URL that matched Safe Browsing list.
> +   * @param aCompletion
> +   *        Completion hash of URL that matched Safe Browsing list.

"Full hash of"

::: netwerk/base/nsIParentChannel.idl:44
(Diff revision 2)
>     * @param aList
>     *        Name of the list that matched
>     * @param aProvider
>     *        Name of provider that matched
> -   * @param aPrefix
> -   *        String represents hash prefix that matched
> +   * @param aCompletion
> +   *        String represents completion hash that matched

"Full hash that matched"

::: netwerk/base/nsIURIClassifier.idl:38
(Diff revision 2)
>     * @param aList
>     *        Name of the list that matched
>     * @param aProvider
>     *        Name of provider that matched
> -   * @param aPrefix
> -   *        Hash prefix of URL that matched
> +   * @param aCompletion
> +   *        Completion hash of URL that matched

"Full hash of URL that matched"

::: toolkit/components/url-classifier/tests/mochitest/test_classifier_match.html:165
(Diff revision 2)
>        let prin = ssm.createCodebasePrincipal(uri, {});
> -      SpecialPowers.doUrlClassify(prin, null, false, function(errorCode, table, provider, prefix) {
> +      SpecialPowers.doUrlClassify(prin, null, false, function(errorCode, table, provider, completion) {
>          is(errorCode, test.expect.error, `Test url ${test.url} correct error`);
>          is(table, test.expect.table, `Test url ${test.url} correct table`);
>          is(provider, test.expect.provider, `Test url ${test.url} correct provider`);
> -        is(prefix, btoa(test.expect.prefix), `Test url ${test.url} correct prefix`);
> +        is(completion, btoa(test.expect.completion), `Test url ${test.url} correct completion`);

"correct fullhash"

Comment 15

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review170436

That's a great start Thomas!

One thing that would be nice is to add a test for the `makeThreatHitReport()` function.

::: modules/libpref/init/all.js:5395
(Diff revision 1)
>  pref("browser.safebrowsing.provider.google4.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
>  pref("browser.safebrowsing.provider.google4.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
>  pref("browser.safebrowsing.provider.google4.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
>  pref("browser.safebrowsing.provider.google4.advisoryURL", "https://developers.google.com/safe-browsing/v4/advisory");
>  pref("browser.safebrowsing.provider.google4.advisoryName", "Google Safe Browsing.");
> +pref("browser.safebrowsing.provider.google4.threatHitReportURL", "https://safebrowsing.googleapis.com/v4/threatHits?$ct=application/x-protobuf&key=%GOOGLE_API_KEY%&$httpMethod=POST");

How about `google4.dataSharingURL`? It seems like we are overusing the word "report" already.

This feature really is all about sharing data with the Safe Browsing provider.

::: modules/libpref/init/all.js:5431
(Diff revision 1)
>  pref("plugins.flashBlock.enabled", false);
>  
>  // Allow users to ignore Safe Browsing warnings.
>  pref("browser.safebrowsing.allowOverride", true);
>  
> +pref("browser.safebrowsing.threatHitReporting.automatic", false);

I propose `browser.safebrowsing.provider.google4.dataSharing.enabled = false`.

It should be provider-specific since we're only reporting the hits from that provider's list.

::: netwerk/base/nsChannelClassifier.cpp:1200
(Diff revision 1)
>          // protection or Safe Browsing.
>          // Do update the security state of the document and fire a security
>          // change event.
>          SetBlockedContent(mChannel, aErrorCode, aList, aProvider, aCompletion);
>  
> +        if (aErrorCode != NS_ERROR_TRACKING_URI &&

Given that we will make this pref provider-specific, you can just check that the given provider has dataSharing enabled.

::: netwerk/base/nsIURIClassifier.idl:111
(Diff revision 1)
> +   * Create and send a threat hit report
> +   *
> +   * @param aChannel: channel which has encountered the threat hit.
> +   * @param aList: string which represents listname of the hit
> +   * @param aProvider: string which represents provider of the hit
> +   * @param aCompletion: string which represents the completion hash of the hit

"full hash"

::: netwerk/base/nsIURIClassifier.idl:115
(Diff revision 1)
> +   * @param aProvider: string which represents provider of the hit
> +   * @param aCompletion: string which represents the completion hash of the hit
> +   *
> +   */
> +
> +  void processThreatHitReport(in nsIChannel aChannel,

Maybe we should call this "sendThreatHitReport"?

::: toolkit/components/telemetry/Histograms.json:5065
(Diff revision 1)
>      "bug_numbers": [1338082],
>      "description": "Negative cache duration (ms) received in fullhash response from any v4 provider"
>    },
> +  "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],

Add your email address to the list too. All new telemetry probes needs the email of a real person.

::: toolkit/components/telemetry/Histograms.json:5066
(Diff revision 1)
>      "description": "Negative cache duration (ms) received in fullhash response from any v4 provider"
>    },
> +  "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "63",

You can make it expire "never" since it's [Category 1](https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories) data.

::: toolkit/components/telemetry/Histograms.json:5070
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "63",
> +    "kind": "enumerated",
> +    "n_values": 30,
> +    "bug_numbers": [1351147],
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host"

nit: space before 5, 11-15

Also the closing parenthesis is missing.

::: toolkit/components/telemetry/Histograms.json:5072
(Diff revision 1)
> +    "kind": "enumerated",
> +    "n_values": 30,
> +    "bug_numbers": [1351147],
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host"
> +  },
> +  "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": {

That should be "STATUS", not "STATUS2". We only add a number when we change the meaning of an existing probe.

::: toolkit/components/telemetry/Histograms.json:5074
(Diff revision 1)
> +    "bug_numbers": [1351147],
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host"
> +  },
> +  "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],

Add your email here too.

::: toolkit/components/telemetry/Histograms.json:5075
(Diff revision 1)
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host"
> +  },
> +  "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "63",

never

::: toolkit/components/url-classifier/SBTelemetryUtils.h:1
(Diff revision 1)
> +//* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Maybe we should call this just "TelemetryUtils"? Is it going to pollute the global namespace?

Otherwise, we should avoid using an acronym and just use "UrlClassifierTelemetryUtils".

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2113
(Diff revision 1)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  RefPtr<nsThreatHitReportListener> listener = new nsThreatHitReportListener();
> +  rv = reportChannel->AsyncOpen2(listener);
> +  if (NS_FAILED(rv)) {
> +    LOG(("Sent threat hit report failed"));

"Failure to send Safe Browsing threat hit report.

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
(Diff revision 1)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    return NS_OK;
>  }
>  
> -// We might need to expand the bucket here if telemetry shows lots of errors

Nice refactor!

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:606
(Diff revision 1)
> +  NS_ENSURE_TRUE(!aHashBase64.IsEmpty(), NS_ERROR_FAILURE);
> +
> +  ThreatHit hit;
> +  nsresult rv;
> +
> +  // 1) Set threat types.

I think the 1, 2, 3, 4, 5, 6 comments are unnecessary.

It's already quite clear since you broke it up in paragraphs.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:637
(Diff revision 1)
> +
> +  // XXX 6) Set UserInfo
> +  // Bug 1387364: we need to add unique ID stable over 1-2 week
> +  // Bug 1372456: Have to fetch region code before building the report message
> +
> +  //-------------------------------------------------------------------

I would also omit these comments.

Comment 16

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12)
> (In reply to François Marier [:francois] from comment #8)
> > The updated ThreatSourceType enum (see
> > https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a
> > TAB_RESOURCE for any redirects encountered while fetching a bad subresource.
> 
> ...
>       // A resource loaded within the final TAB_URL.
>       TAB_RESOURCE = 4;
> It looks like google wants us to send all sub-resource loadings of a warning
> page (eg all images, iframes, styles on the warning page). Seems quite a lot
> of things to do

No, that's only if the sub-resource was the one that got blocked.

Here are some examples:

     Case 1:
     1. page.com redirects to page.net
     2. page.net loads badimage.com
     3. badimage.com matches the malware list

     We would send the following:

     - TAB_URL = page.net
     - TAB_REDIRECT = page.com
     - MATCHING_URL = badimage.com

     Case 2:
     1. page.com redirects to page.net
     2. page.net loads image.net which redirects to badimage.com
     3. badimage.com matches the malware list

     We would send the following:

     - TAB_URL = page.net
     - TAB_REDIRECT = page.com
     - MATCHING_URL = badimage.com
     - TAB_RESOURCE = image.net

Comment 17

[Thomas Nguyen[:tnguyen] ni? plz]

Ah, I see. The description of the type is rather confusing. Thanks for clarifying that and taking a look.

Comment 18

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review170436

I have been trying to find a way to test that func, but still don't have good a way at the moment :(.
The reason is we have to test in gtest (protobuf things) but have to touch to many things in channel ( and it's member like loadinfo and redirect chains).
And server only returns OK, not OK for now, so not sure whether what we are writing to that is correct.

Comment 19

[Thomas Nguyen[:tnguyen] ni? plz]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #18)
> Comment on attachment 8893745 [details]
> Bug 1351147 - Support v4/ThreatHit request in SafeBrowsing V4
> 
> https://reviewboard.mozilla.org/r/164822/#review170436
> 
> I have been trying to find a way to test that func, but still don't have
> good a way at the moment :(.
> The reason is we have to test in gtest (protobuf things) but have to touch
> to many things in channel ( and it's member like loadinfo and redirect
> chains).
> And server only returns OK, not OK for now, so not sure whether what we are
> writing to that is correct.

I am writing a mochitest to see if we are going to send a threathit request from a correct channel, full hash, but we may not know whether the request is correct. Presumably, we may have to ask google to see from server side

Comment 20

[Thomas Nguyen[:tnguyen] ni? plz]

Created attachment 8899701 [details] [diff] [review]
Dump response

MozReview-Commit-ID: 8RNPt9ZK6Kr

Comment 21

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146020/diff/2-3/

Comment 22

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146022/diff/2-3/

Comment 23

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/1-2/

Comment 24

[Thomas Nguyen[:tnguyen] ni? plz]

Continue working on this bug and I fixed the comments of the last review. Francois, if you have time, could you please take a look at this?

Comment 25

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/2-3/

Comment 26

[François Marier [:francois]]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

https://reviewboard.mozilla.org/r/146022/#review187312

r+ with the comments below

::: commit-message-756c9:3
(Diff revisions 2 - 3)
> -Bug 1351147 - Use completion instead of prefix in OnClassifyComplete
> +Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete
> +
> +When an URL encourters a list, we have to collect the matching full hash to

I think this is what you mean:

"In order to optionally report the full hash back to Google, we need to keep it around in the callback."

::: netwerk/protocol/data/DataChannelParent.cpp:55
(Diff revision 3)
>  }
>  
>  NS_IMETHODIMP
>  DataChannelParent::SetClassifierMatchedInfo(const nsACString& aList,
>                                              const nsACString& aProvider,
> -                                            const nsACString& aPrefix)
> +                                            const nsACString& aCompletion)

Looks like you missed one: `aCompletion` should be `aFullHash` here too.

Comment 27

[François Marier [:francois]]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

https://reviewboard.mozilla.org/r/146022/#review187312

> I think this is what you mean:
> 
> "In order to optionally report the full hash back to Google, we need to keep it around in the callback."

Actually, looking at what I wrote last time I reviewed this patch, let's expand this:

"In order to optionally report the full hash back to Google, we need to keep it around in the callback. While a prefix is not the same as a full hash (multiple full hashes can map to the same prefix), in this case, the callback will only be called when the full hash matches."

Does that sound right to you Thomas?

Comment 28

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187324

::: commit-message-b7512:1
(Diff revision 3)
> +Bug 1351147 - Support v4/ThreatHit request in SafeBrowsing V4

Support ThreatHit requests in Safe Browsing V4

::: netwerk/base/nsChannelClassifier.cpp:1245
(Diff revision 3)
>          // protection or Safe Browsing.
>          // Do update the security state of the document and fire a security
>          // change event.
>          SetBlockedContent(mChannel, aErrorCode, aList, aProvider, aFullHash);
>  
> +        if (aErrorCode == NS_ERROR_MALWARE_URI ||

We also have `NS_ERROR_HARMFUL_URI` now.

::: netwerk/base/nsIURIClassifier.idl:106
(Diff revision 3)
>     * This is an internal interface used only for testing purposes.
>     */
>    ACString classifyLocal(in nsIURI aURI, in ACString aTables);
> +
> +  /**
> +   * Create and send a threat hit report

I would suggest "Report to the provider that a Safe Browsing warning was shown."

::: netwerk/base/nsIURIClassifier.idl:108
(Diff revision 3)
>    ACString classifyLocal(in nsIURI aURI, in ACString aTables);
> +
> +  /**
> +   * Create and send a threat hit report
> +   *
> +   * @param aChannel: channel which has encountered the threat hit.

"channel for which the URL matched something on the threat list"

::: toolkit/components/telemetry/Histograms.json:5017
(Diff revision 3)
>      "high": 86400000,
>      "n_buckets": 50,
>      "bug_numbers": [1338082],
>      "description": "Negative cache duration (ms) received in fullhash response from any v4 provider"
>    },
> +  "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": {

Let's shorten that to `URLCLASSIFIER_THREATHIT_NETWORK_ERROR`.

::: toolkit/components/telemetry/Histograms.json:5020
(Diff revision 3)
>      "description": "Negative cache duration (ms) received in fullhash response from any v4 provider"
>    },
> +  "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail], [e-mail]"],
> +    "expires_in_version": "never",

Also add:

    "releaseChannelCollection": "opt-out",

so that we also collect this Category 1 data on release.

::: toolkit/components/telemetry/Histograms.json:5024
(Diff revision 3)
> +    "alert_emails": ["[e-mail], [e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 30,
> +    "bug_numbers": [1351147],
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)"

"Whether or not an error was encountered while sending a Safe Browsing ThreatHit report..."

::: toolkit/components/telemetry/Histograms.json:5026
(Diff revision 3)
> +    "kind": "enumerated",
> +    "n_values": 30,
> +    "bug_numbers": [1351147],
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)"
> +  },
> +  "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS": {

`URLCLASSIFIER_THREATHIT_REMOTE_STATUS`

::: toolkit/components/telemetry/Histograms.json:5029
(Diff revision 3)
> +    "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)"
> +  },
> +  "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail], [e-mail]"],
> +    "expires_in_version": "never",

Add: `"releaseChannelCollection": "opt-out",`

::: toolkit/components/telemetry/Histograms.json:5033
(Diff revision 3)
> +    "alert_emails": ["[e-mail], [e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 16,
> +    "bug_numbers": [1351147],
> +    "description": "Server HTTP status code from SafeBrowsing threat hit report. (0=1xx, 1=200, 2=2xx, 3=204, 4=3xx, 5=400, 6=4xx, 7=403, 8=404, 9=408, 10=413, 11=5xx, 12=502|504|511, 13=503, 14=505, 15=Other)"

nit: "Safe Browsing" and "ThreatHit"

::: toolkit/components/url-classifier/nsIUrlClassifierUtils.idl:139
(Diff revision 3)
>                                       [array, size_is(aPrefixCount)] in string aPrefixes,
>                                       in uint32_t aListCount,
>                                       in uint32_t aPrefixCount);
>  
>    /**
> +   * Make threat hit report request body.

nit: "ThreatHit"

::: toolkit/components/url-classifier/nsIUrlClassifierUtils.idl:147
(Diff revision 3)
> +   * @param aListName listname represented in string.
> +   * @param aHashBase64 hash-based hit represented in base64.
> +   *
> +   * @returns A base64 encoded string.
> +   */
> +  ACString makeThreatHitReportV4(in nsIChannel aChannel,

There's only a single ThreatHit API, we may as well drop the `V4` in the name of the function.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1936
(Diff revision 3)
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>    return NS_OK;
>  }
>  
> +class nsThreatHitReportListener final

No `ns` prefix on new classes.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2061
(Diff revision 3)
> +
> +  nsString urlStr;
> +  rv = formatter->FormatURLPref(NS_ConvertUTF8toUTF16(reportUrlPref), urlStr);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (urlStr.IsEmpty() || NS_LITERAL_STRING("about:blank").Equals(urlStr)) {

Why are you comparing this with "about:blank"? In case a user decides to put "about:blank" in the pref?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2062
(Diff revision 3)
> +  nsString urlStr;
> +  rv = formatter->FormatURLPref(NS_ConvertUTF8toUTF16(reportUrlPref), urlStr);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (urlStr.IsEmpty() || NS_LITERAL_STRING("about:blank").Equals(urlStr)) {
> +    return NS_OK;

We should have a `LOG()` here:

    <provider> is missing a ThreatHit data reporting URL.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2117
(Diff revision 3)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  RefPtr<nsThreatHitReportListener> listener = new nsThreatHitReportListener();
> +  rv = reportChannel->AsyncOpen2(listener);
> +  if (NS_FAILED(rv)) {
> +    LOG(("Failure to send Safe Browsing threat hit report"));

nit: "ThreatHit"

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:478
(Diff revision 3)
>  
> +// Remove ref, query, userpass, anypart which may contain sensitive data
> +static nsresult
> +GetSpecWithoutSensitiveData(nsIURI* aUri, nsACString &aSpec)
> +{
> +  nsCOMPtr<nsIURI> clone;

This should have a runtime check to make sure that `aUri` is not NULL.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:500
(Diff revision 3)
> +  return NS_OK;
> +}
> +
> +// Add a single threatsource from channel to threatHit message
> +static nsresult
> +SetThreatSourceFromChannel(ThreatHit& aHit, nsIChannel *aChannel,

I would rename this function to `AddThreatSourceFromChannel` and then you can get rid of the comment above it.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:544
(Diff revision 3)
> +  }
> +  return NS_OK;
> +}
> +// Add a single threatsource from redirect entry to threatHit message
> +static nsresult
> +SetThreatSourceFromRedirectEntry(ThreatHit& aHit,

`AddThreatSourceFromRedirectEntry`, comment not needed

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:545
(Diff revision 3)
> +  return NS_OK;
> +}
> +// Add a single threatsource from redirect entry to threatHit message
> +static nsresult
> +SetThreatSourceFromRedirectEntry(ThreatHit& aHit,
> +                                 nsIRedirectHistoryEntry *aRediretEntry,

typo: `aRedirectEntry` (missing the `c`)

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:548
(Diff revision 3)
> +static nsresult
> +SetThreatSourceFromRedirectEntry(ThreatHit& aHit,
> +                                 nsIRedirectHistoryEntry *aRediretEntry,
> +                                 ThreatHit_ThreatSourceType aType)
> +{
> +  NS_ENSURE_ARG_POINTER(aRediretEntry);

That should probably be a runtime check.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:585
(Diff revision 3)
> +  return NS_OK;
> +}
> +
> +// Add top level tab url and redirect threatsources to threatHit message
> +static nsresult
> +SetTabThreatSources(ThreatHit& aHit, nsIChannel *aChannel)

`AddTabThreatSources()`

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:598
(Diff revision 3)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = thirdPartyUtil->GetTopWindowForChannel(aChannel, getter_AddRefs(win));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  auto* pwin = nsPIDOMWindowOuter::From(win);

I suspect this might leak if not manually `delete`d later. Probably better to use an explicit smart pointer here.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:623
(Diff revision 3)
> +  bool isTopUri = false;
> +  rv = topUri->Equals(uri, &isTopUri);
> +  if (NS_SUCCEEDED(rv) && !isTopUri) {
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +    if (loadInfo && loadInfo->RedirectChain().Length()) {
> +      SetThreatSourceFromRedirectEntry(aHit, loadInfo->RedirectChain()[0],

Maybe we should guard against `loadInfo->RedirectChain()` not returning any elements and check the length before looking at the first element.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:656
(Diff revision 3)
> +nsUrlClassifierUtils::MakeThreatHitReportV4(nsIChannel *aChannel,
> +                                            const nsACString& aListName,
> +                                            const nsACString& aHashBase64,
> +                                            nsACString &aRequest)
> +{
> +  NS_ENSURE_ARG_POINTER(aChannel);

That should probably be a runtime check, unless you're already it in the caller.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:666
(Diff revision 3)
> +  nsresult rv;
> +
> +  uint32_t threatType;
> +  rv = ConvertListNameToThreatType(aListName, &threatType);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  hit.set_threat_type((ThreatType)threatType);

Can you use a C++ cast here instead?

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:672
(Diff revision 3)
> +
> +  hit.set_platform_type(GetPlatformType());
> +
> +  nsCString hash;
> +  Unused << Base64Decode(aHashBase64, hash);
> +  auto threatEntry = hit.mutable_entry();

Let's add an assert like "hash.length <= size-of-a-full-hash".

That will help if we ever get passed a bad base64 string.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:690
(Diff revision 3)
> +  std::string s;
> +  hit.SerializeToString(&s);
> +
> +  nsCString out;
> +  rv = Base64URLEncode(s.size(),
> +                       (const uint8_t*)s.c_str(),

C++ cast (`reinterpret_cast` I imagine)

::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:60
(Diff revision 3)
> +  return hasher.finish(false);
> +}
> +
> +var testDatas = [
> +  { url: "itisaphishingsite.org/phishing.html",
> +    list: "test-phish-proto",

Let's make this `mochitest-phish-proto` to prevent any future conflicts if we ever get rid of the `-simple` lists.

::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:61
(Diff revision 3)
> +}
> +
> +var testDatas = [
> +  { url: "itisaphishingsite.org/phishing.html",
> +    list: "test-phish-proto",
> +    provider: "test",

Maybe `mochitest` here too?

::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:62
(Diff revision 3)
> +
> +var testDatas = [
> +  { url: "itisaphishingsite.org/phishing.html",
> +    list: "test-phish-proto",
> +    provider: "test",
> +    // The base64 of protobuf binary represention of response:

typo: "representation"

nit: "binary protobuf representation" instead of "protobuf binary representation"

::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:210
(Diff revision 3)
> +      let progressListener = {
> +        onSecurityChange(aWebProgress, aRequest, aState) {
> +          let utils = Cc["@mozilla.org/url-classifier/utils;1"].
> +            getService(Ci.nsIUrlClassifierUtils);
> +          expected = aTestData.reportUrl + "&$req=" +
> +            utils.makeThreatHitReportV4(aRequest,

If I understand this correctly, it's checking that what is sent to the server matches the output of the function that is used to generate the reports.

So it's a bit like comparing a function with itself. It's not likely to fail very often :)

Maybe we should instead, in the `checkResults()` function, unpack the protobuf and confirm that the various URLs and attributes are the ones we expect to see?

::: toolkit/components/url-classifier/tests/mochitest/threathit.sjs:1
(Diff revision 3)
> +const CC = Components.Constructor;

This should also have a public domain header.

It feels like we have a couple of very similar SJS files that basically just store and replay some strings. No need to do it in this patch, but perhaps we should merge them into a single generic one at some stage.

Comment 29

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187768

::: toolkit/components/url-classifier/tests/mochitest/threathit.sjs:1
(Diff revision 3)
> +const CC = Components.Constructor;

Ah, we have a sort of similar sjs files. They do not just store strings, but store and parse V2 content (encoded base64). In V4, I think should only store/save binary file and do not need ( or it's hard to do)  to parse the binary protobuf request.
Obviously we could merge V2 and V4 to an unified update.sjs, but it looks like we have to modify several old tests which use V2 -simple list. Seems it would be better to do in a follow up bug.

Comment 30

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187324

> If I understand this correctly, it's checking that what is sent to the server matches the output of the function that is used to generate the reports.
> 
> So it's a bit like comparing a function with itself. It's not likely to fail very often :)
> 
> Maybe we should instead, in the `checkResults()` function, unpack the protobuf and confirm that the various URLs and attributes are the ones we expect to see?

Yes, you are right. I thought about that before writing the test, but the protobuf and its class interfaces are only available in cpp. I did not try to add that, because I think we may not want to add a new testing xpcom component to access cpp only protobuf classes (the test xpcom component will be also created and added to the build package manifest but never be used). Indeed, we only test this sort of unpacking in Gtest. 
It's a closed mind, I've thought that I may add a new interface to nsIUrlClassifierUtils.idl to parse/unpack the request, but for testing only

Comment 31

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187324

> Yes, you are right. I thought about that before writing the test, but the protobuf and its class interfaces are only available in cpp. I did not try to add that, because I think we may not want to add a new testing xpcom component to access cpp only protobuf classes (the test xpcom component will be also created and added to the build package manifest but never be used). Indeed, we only test this sort of unpacking in Gtest. 
> It's a closed mind, I've thought that I may add a new interface to nsIUrlClassifierUtils.idl to parse/unpack the request, but for testing only

Ok, that's something we should solve because our protobuf tests aren't that great otherwise.

Let's land this patch without it and fix the test in a follow-up bug. Can you file one please?

Comment 32

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187768

> Ah, we have a sort of similar sjs files. They do not just store strings, but store and parse V2 content (encoded base64). In V4, I think should only store/save binary file and do not need ( or it's hard to do)  to parse the binary protobuf request.
> Obviously we could merge V2 and V4 to an unified update.sjs, but it looks like we have to modify several old tests which use V2 -simple list. Seems it would be better to do in a follow up bug.

Fair enough, I was probably thinking of the V2 tests. No need to merge the V2/V4 test cases. They are pretty different as you point out.

Comment 33

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

https://reviewboard.mozilla.org/r/146022/#review187312

> Actually, looking at what I wrote last time I reviewed this patch, let's expand this:
> 
> "In order to optionally report the full hash back to Google, we need to keep it around in the callback. While a prefix is not the same as a full hash (multiple full hashes can map to the same prefix), in this case, the callback will only be called when the full hash matches."
> 
> Does that sound right to you Thomas?

Thanks you for rephasing it.

Comment 34

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146020/diff/3-4/

Comment 35

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146022/diff/3-4/

Comment 36

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/3-4/

Comment 37

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187324

Thanks Francois for the detailed review

> Why are you comparing this with "about:blank"? In case a user decides to put "about:blank" in the pref?

It may return "about:blank" when the pref is missing. 
http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/urlformatter/nsIURLFormatter.idl#44

> I suspect this might leak if not manually `delete`d later. Probably better to use an explicit smart pointer here.

This is only a static_cast from nsCOMPtr<mozIDOMWindowProxy> win, it does not new a pointer

> Maybe we should guard against `loadInfo->RedirectChain()` not returning any elements and check the length before looking at the first element.

topLoadInfo->RedirectChain() returns an array in which IsEmpty() is exactly the same Length() == 0. I think it's not necessary, the for loop indexing will do that

> Let's add an assert like "hash.length <= size-of-a-full-hash".
> 
> That will help if we ever get passed a bad base64 string.

I guess you say "==", we only send a 32 bytes fullhash

> Let's make this `mochitest-phish-proto` to prevent any future conflicts if we ever get rid of the `-simple` lists.

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#264

We defined a test-phish-proto table for v4 test and we have to use the correct table. That's a limitation that we have to add a new table there to support mochitest-phish-proto

Comment 38

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review187324

> This is only a static_cast from nsCOMPtr<mozIDOMWindowProxy> win, it does not new a pointer

Sorry I misread that code.

> http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#264
> 
> We defined a test-phish-proto table for v4 test and we have to use the correct table. That's a limitation that we have to add a new table there to support mochitest-phish-proto

I forgot about that. You're right, let's use the name we've already defined.

Comment 39

[François Marier [:francois]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review188556

Looks great! r+ and datareview+

::: netwerk/base/nsChannelClassifier.cpp:1174
(Diff revision 4)
> +{
> +
> +  nsAutoCString provider(aProvider);
> +  nsPrintfCString reportEnablePref("browser.safebrowsing.provider.%s.dataSharing.enabled",
> +                                   provider.get());
> +  if (!Preferences::GetBool(reportEnablePref.get())) {

Let's be explicit about the default value of this pref if it's not set:

    GetBool(reportEnablePref.get(), false)

Comment 40

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/4-5/

Comment 41

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/4-5/

Comment 42

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146020/diff/4-5/

Comment 43

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8874778 [details]
Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146022/diff/4-5/

Comment 44

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/5-6/

Comment 45

[Thomas Nguyen[:tnguyen] ni? plz]

Fixed lint errors after running ESLint and rebased lastest m-c code

Comment 46

[Thomas Nguyen[:tnguyen] ni? plz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4442b36e97e1e5af3721a2fd4a4f37937de3a2d1&selectedJob=133245365

Comment 47

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/6-7/

Comment 48

[Thomas Nguyen[:tnguyen] ni? plz]

Try failed because the test url itisaphishingsite.org conflicted with previous test test_reporturl.html
Change it to itisaphishingsite1.org and change update/gethash binary respectively (the attach file "Dump response" is what I used to generate binary)

Comment 49

[Jan Keromnes [:janx]]

Comment on attachment 8874777 [details]
Bug 1351147 - Update protobuf to support threatHit api

https://reviewboard.mozilla.org/r/146020/#review188660

Just a few minor comments in passing: Our static analysis bot found that you could be using `nullptr` instead of many `NULL`s in this code. (6 occurrences in your patch, and 76 others in the same file which could be fixed drive-by).

::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4030
(Diff revision 5)
> +}
> +const ThreatHit_UserInfo& ThreatHit_UserInfo::default_instance() {
> +#ifdef GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER
> +  protobuf_AddDesc_safebrowsing_2eproto();
> +#else
> +  if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto();

Quick note: Our static analysis bot found a potential improvement on this line:

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

```
  if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto();
                           ^
                           nullptr
```

::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4035
(Diff revision 5)
> +  if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto();
> +#endif
> +  return *default_instance_;
> +}
> +
> +ThreatHit_UserInfo* ThreatHit_UserInfo::default_instance_ = NULL;

Same quick note as above:

```
ThreatHit_UserInfo* ThreatHit_UserInfo::default_instance_ = NULL;
                                                            ^
                                                            nullptr
```

::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4261
(Diff revision 5)
>    _cached_size_ = 0;
>    threat_type_ = 0;
>    platform_type_ = 0;
>    entry_ = NULL;
> +  client_info_ = NULL;
> +  user_info_ = NULL;

Same quick note as above:

```
  client_info_ = NULL;
                 ^
                 nullptr
  user_info_ = NULL;
               ^
               nullptr
```

::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4319
(Diff revision 5)
>      ZR_(threat_type_, platform_type_);
>      if (has_entry()) {
>        if (entry_ != NULL) entry_->::mozilla::safebrowsing::ThreatEntry::Clear();
>      }
> +    if (has_client_info()) {
> +      if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear();

Same quick note as above:

```
      if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear();
                          ^
                          nullptr
```

::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4322
(Diff revision 5)
>      }
> +    if (has_client_info()) {
> +      if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear();
> +    }
> +    if (has_user_info()) {
> +      if (user_info_ != NULL) user_info_->::mozilla::safebrowsing::ThreatHit_UserInfo::Clear();

Same quick note as above:

```
      if (user_info_ != NULL) user_info_->::mozilla::safebrowsing::ThreatHit_UserInfo::Clear();
                        ^
                        nullptr
```

Comment 50

[Jan Keromnes [:janx]]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

https://reviewboard.mozilla.org/r/164822/#review188662

Two more small suggestions from our static analysis bot.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1940
(Diff revision 7)
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIREQUESTOBSERVER
> +  NS_DECL_NSISTREAMLISTENER
> +
> +  ThreatHitReportListener() {}

Quick note: Our static analysis bot found a potential improvement for this line:

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

```
  ThreatHitReportListener() {}
  ^
                            = default;
```

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1943
(Diff revision 7)
> +  NS_DECL_NSISTREAMLISTENER
> +
> +  ThreatHitReportListener() {}
> +
> +private:
> +  ~ThreatHitReportListener() {}

Similar quick note:

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

```
  ~ThreatHitReportListener() {}
  ^
                             = default;
```

Comment 51

[Thomas Nguyen[:tnguyen] ni? plz]

safebrowsing.pb.cc is automatically generated file from running protobuf. We may not want to change this file. Is there any way to exclude the file from running static analysis?
Static analysis running seems interesting, which tool are we using now?

Comment 52

[Thomas Nguyen[:tnguyen] ni? plz]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #51)
> Static analysis running seems interesting, which tool are we using now?

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]
Looks like that is clang-tidy

Comment 53

[Thomas Nguyen[:tnguyen] ni? plz]

Comment on attachment 8893745 [details]
Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164822/diff/7-8/

Comment 54

[Thomas Nguyen[:tnguyen] ni? plz]

Updated the commit, only changed as suggestions in nsUrlClassifierDBService, but I still keep the safebrowsing.pb.cc
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e668f9c3c99020bac1d8b021bf112db4927bd705

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2b1824179f8
Update protobuf to support threatHit api r=francois
https://hg.mozilla.org/integration/autoland/rev/7c7be12b43be
Use fullhash instead of prefix in OnClassifyComplete r=francois
https://hg.mozilla.org/integration/autoland/rev/d10aeb884922
Support ThreatHit requests in SafeBrowsing V4 r=francois

Comment 56

[Jan Keromnes [:janx]]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #51)
> safebrowsing.pb.cc is automatically generated file from running protobuf. We
> may not want to change this file. Is there any way to exclude the file from
> running static analysis?

Ah, indeed if the file is generated automatically, we wouldn't want our tools to change it.

We can simply add it to /tools/rewriting/ThirdPartyPaths.txt. I can do that, but which of these paths should I make our tools ignore?

- toolkit/components/url-classifier/protobuf/safebrowsing.pb.*
- toolkit/components/url-classifier/protobuf/*
- toolkit/components/url-classifier/*

Additionally, is there any way to make protobuf generate C++11 files, e.g. that use `nullptr` instead of `NULL`? (Not a big issue, but nice to have if it's easy to do.)

> Static analysis running seems interesting, which tool are we using now?

For this analysis we're indeed using clang-tidy. And you can now easily run it too by using:

./mach static-analysis check path/to/file.cpp

Comment 57

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #56)
> We can simply add it to /tools/rewriting/ThirdPartyPaths.txt. I can do that,
> but which of these paths should I make our tools ignore?
> 
> - toolkit/components/url-classifier/protobuf/safebrowsing.pb.*
> - toolkit/components/url-classifier/protobuf/*
> - toolkit/components/url-classifier/*

I've added toolkit/components/url-classifier/chromium/* and toolkit/components/url-classifier/protobuf/* to ThirdPartyPaths.txt in bug 1403527.

Please confirm that this looks reasonable.

Comment 58

[François Marier [:francois]]

(In reply to Jan Keromnes [:janx] from comment #57)
> I've added toolkit/components/url-classifier/chromium/* and
> toolkit/components/url-classifier/protobuf/* to ThirdPartyPaths.txt in bug
> 1403527.
> 
> Please confirm that this looks reasonable.

To ignore all of the protobuf-related code:

devtools/shared/heapsnapshot/CoreDump.pb.*
gfx/layers/protobuf/*
toolkit/components/downloads/chromium/*
toolkit/components/protobuf/src/*
toolkit/components/url-classifier/chromium/*
toolkit/components/url-classifier/protobuf/*

Comment 59

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f2b1824179f8
https://hg.mozilla.org/mozilla-central/rev/7c7be12b43be
https://hg.mozilla.org/mozilla-central/rev/d10aeb884922