1384753 - Move csd.proto into a common Safe Browsing directory and update it

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

Currently csd.proto is only used by download protection and lives in https://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/components/downloads/generate_csd.sh

Since it will be also required by the password protection feature, we should probably create a place for common Safe Browsing files and utilities.

It will also have to be updated to the latest upstream version:

https://cs.chromium.org/chromium/src/components/safe_browsing/csd.proto

Comment 1

[François Marier [:francois]]

The latest version of csd.proto requires version 3 of the protobuf compiler: https://github.com/google/protobuf/issues/1747

We'll need to update the version we bundle in toolkit/components/protobuf

Comment 2

[François Marier [:francois]]

Attachment: generate_csd.diff

Comment 3

[François Marier [:francois]]

It's possible we can just hack the upstream csd.proto file and comment-out the reserved field.

Comment 4

[Thomas Nguyen (:tnguyen)]

Is there any diff in output between using protobuf 3.0.0 and 2.6.1? Basically safebrowsing.proto use protobuf 2 (syntax = "proto2"). I am concerning if SB still works when we compile and generate output using v3.0 (although, it said that v3.0 supports both proto2 and proto3).
Do you know whether Google wants to upgrade protobuf completely to 3.0? It would be good because JS will be supported from 3.0 and we can use JS generated files in our test (parsing for example)

Comment 5

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #4)
> Is there any diff in output between using protobuf 3.0.0 and 2.6.1?
> Basically safebrowsing.proto use protobuf 2 (syntax = "proto2"). I am
> concerning if SB still works when we compile and generate output using v3.0
> (although, it said that v3.0 supports both proto2 and proto3).

It seems like every version of protobuf changes the output slightly. Not necessarily in a meaningful way though.

And you're right, version 3 of the library supports both the proto2 and proto3 specifications, so it's backwards-compatible.

> Do you know whether Google wants to upgrade protobuf completely to 3.0?

I don't know, but Chrome has upgraded the library to 3.3.0 at least.

Comment 6

[François Marier [:francois]]

Attachment: move-apprep-files-1384753.patch

Here's my WIP patch for moving the application reputation code into a new reputation service where we can put both application reputation and login reputation.

Comment 7

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #6)
> Created attachment 8913847 [details] [diff] [review]
> move-apprep-files-1384753.patch
> 
> Here's my WIP patch for moving the application reputation code into a new
> reputation service where we can put both application reputation and login
> reputation.

Great, thanks. BTW, I also forgot to ask you if you want to keep working on this one.
If yes, please feel free to take it back.

Comment 8

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> Great, thanks. BTW, I also forgot to ask you if you want to keep working on
> this one.
> If yes, please feel free to take it back.

Feel free to finish this patch. I'm working on the protobuf library upgrade right now.

Comment 9

[Dimi Lee [:dimi]]

Attachment: Bug 1384753 - Move Application Reputation files into a new component.

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

Comment 10

[Dimi Lee [:dimi]]

Attachment: Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review197890


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/reputationservice/LoginReputation.cpp:31
(Diff revision 1)
> +  }
> +
> +  return gLoginReputationService;
> +}
> +
> +LoginReputationService::LoginReputationService()

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

::: toolkit/components/reputationservice/LoginReputation.cpp:35
(Diff revision 1)
> +
> +LoginReputationService::LoginReputationService()
> +{
> +}
> +
> +LoginReputationService::~LoginReputationService()

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

Comment 12

[François Marier [:francois]]

Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

https://reviewboard.mozilla.org/r/192718/#review198254

::: commit-message-a124f:1
(Diff revision 1)
> +Bug 1384753 - P1. Move csd.proto into a common Safe Browsing directory and update it. r?francois

You didn't actually update the CSD file, which is fine, we should do that in a different commit.

This commit message should be simply:

    Move Application Reputation files into a new component.

Also, we don't really need the "P1" since the order will be preserved automatically by MozReview.

::: toolkit/components/reputationservice/test/unit/test_app_rep_maclinux.js:7
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * This file tests signature extraction using Windows Authenticode APIs of

This comment is a lie and comes from a copy/paste error I made a long time ago.

We may as well remove it.

Comment 13

[François Marier [:francois]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review198260

I would suggest adding one more thing:

- initialize the login reputation service at startup idle time if `browser.safebrowsing.passwords.enabled = true`.

::: commit-message-088a8:1
(Diff revision 1)
> +Bug 1384753 - P2. Add dummy login reputation service in reputationservice directory. r?francois

I would simply say "Add dummy login reputation service in reputationservice component."

::: toolkit/components/reputationservice/LoginReputation.cpp:40
(Diff revision 1)
> +LoginReputationService::~LoginReputationService()
> +{
> +}
> +
> +NS_IMETHODIMP
> +LoginReputationService::QueryReputation()

Maybe we shouldn't define this yet since we're not sure exactly what parameters it's going to take?

Comment 14

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #12)
> 
> You didn't actually update the CSD file, which is fine, we should do that in
> a different commit.

Yes, I want to update it in a separate one. I just created bug 1411861 for that.

Comment 15

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #13)
> Maybe we shouldn't define this yet since we're not sure exactly what
> parameters it's going to take?

The parameters is filled in Bug 1407878.
I'll remove |QueryReputation| in this patch.

Comment 16

[Dimi Lee [:dimi]]

Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

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

Comment 17

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 18

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review197890

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

Fix this by adding LOG to constructor & destructor which I was going to add them in Bug 1407878

Comment 19

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 20

[François Marier [:francois]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review198658

::: commit-message-088a8:3
(Diff revisions 1 - 3)
> -Bug 1384753 - P2. Add dummy login reputation service in reputationservice directory. r?francois
> +Bug 1384753 - Add dummy login reputation service in reputationservice component. r?francois
>  
> -MozReview-Commit-ID: Ie7PIiHYZ1g
> +Login reputation service is initialized when idle startup.

"during idle statup."

::: toolkit/components/reputationservice/LoginReputation.cpp:10
(Diff revisions 1 - 3)
>  
>  #include "LoginReputation.h"
>  
>  using namespace mozilla;
>  
> +// MOZ_LOG=LoginReputation:5

Adding logging early, great idea!

::: toolkit/components/reputationservice/LoginReputation.cpp:38
(Diff revisions 1 - 3)
>    return gLoginReputationService;
>  }
>  
>  LoginReputationService::LoginReputationService()
>  {
> +  LR_LOG(("Login reputation service started up"));

Maybe we should use "starting up" in order to look the same as the destructor.

::: browser/components/nsBrowserGlue.js:1113
(Diff revision 3)
>      // It's important that SafeBrowsing is initialized reasonably
>      // early, so we use a maximum timeout for it.
>      Services.tm.idleDispatchToMainThread(() => {
>        SafeBrowsing.init();
> +
> +      // Init login reputation service after initializing SafeBrowsing.jsm because

I would say simply:

    // Login reputation depends on the Safe Browsing API.

Comment 21

[Dimi Lee [:dimi]]

Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

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

Comment 22

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 23

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 24

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #23)
> Comment on attachment 8921701 [details]
> Bug 1384753 - Add dummy login reputation service in reputationservice
> component.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/192720/diff/4-5/

Singleton constructors require already-addrefed now (Bug 1409249), fix compile error.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9801aade9511
Move Application Reputation files into a new component. r=francois
https://hg.mozilla.org/integration/autoland/rev/2f0da700e651
Add dummy login reputation service in reputationservice component. r=francois

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently failing chrome's toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html on Android:

https://hg.mozilla.org/integration/autoland/rev/806540586443b1304287811c592cfcc7d90e0cb6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2f0da700e6518876aeefff4e48eb5a0a2e055074&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140220271&repo=autoland

[task 2017-10-27T15:48:47.309Z] 15:48:47     INFO -  90 INFO None91 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
[task 2017-10-27T15:48:57.607Z] 15:48:57     INFO -  Buffered messages logged at 15:48:45
[task 2017-10-27T15:48:57.607Z] 15:48:57     INFO -  92 INFO SpawnTask.js | Entering test setup
[task 2017-10-27T15:48:57.608Z] 15:48:57     INFO -  93 INFO Using file picker download directory /data/data/org.mozilla.fennec_aurora/app_tmpdir/downloads
[task 2017-10-27T15:48:57.609Z] 15:48:57     INFO -  94 INFO Using default download directory /data/data/org.mozilla.fennec_aurora/app_tmpdir/downloads-1
[task 2017-10-27T15:48:57.610Z] 15:48:57     INFO -  95 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | Should be able to distinguish between files saved with or without the file picker
[task 2017-10-27T15:48:57.611Z] 15:48:57     INFO -  Buffered messages logged at 15:48:46
[task 2017-10-27T15:48:57.613Z] 15:48:57     INFO -  96 INFO SpawnTask.js | Leaving test setup
[task 2017-10-27T15:48:57.613Z] 15:48:57     INFO -  97 INFO SpawnTask.js | Entering test test_downloads_saveAs
[task 2017-10-27T15:48:57.614Z] 15:48:57     INFO -  98 INFO Extension loaded
[task 2017-10-27T15:48:57.615Z] 15:48:57     INFO -  Buffered messages logged at 15:48:52
[task 2017-10-27T15:48:57.616Z] 15:48:57     INFO -  99 INFO Testing that saveAs=true uses the file picker as expected
[task 2017-10-27T15:48:57.617Z] 15:48:57     INFO -  100 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | the file should have been cleaned up properly previously
[task 2017-10-27T15:48:57.618Z] 15:48:57     INFO -  Buffered messages finished
[task 2017-10-27T15:48:57.619Z] 15:48:57     INFO -  101 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | downloads.download() works with saveAs=true

Comment 27

[Dimi Lee [:dimi]]

Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

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

Comment 28

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 29

[Dimi Lee [:dimi]]

Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

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

Comment 30

[Dimi Lee [:dimi]]

Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

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

Comment 31

[Dimi Lee [:dimi]]

Fix try error on android, looks good now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5658c679833ef7859fda8fc6405353c1693b778d

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45181edd35f6
Move Application Reputation files into a new component. r=francois
https://hg.mozilla.org/integration/autoland/rev/a6217947aff0
Add dummy login reputation service in reputationservice component. r=francois

Comment 33

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/45181edd35f6
https://hg.mozilla.org/mozilla-central/rev/a6217947aff0

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/97bf29612932
Port bug 1384753 to TB/IB/SM: Move Application Reputation files into a new component. rs=bustage-fix