Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567483002:
Add proto for TLS error assistant, refactor proto generator code. (Closed)
DescriptionAdd proto for TLS error assistant, refactor proto generator code.
This CL adds binary_proto_generator.py which contains code to generate
binary protobufs from ascii protobufs. SafeBrowsing download file types
list and TLS error assistant use this generator to generate their
respective binary protos.
BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/657c5e7bbb7ea1ae591d97de9adf46a03410a37c
Cr-Commit-Position: refs/heads/master@{#439292}
Patch Set 1 #
Total comments: 26
Patch Set 2 : nparker comments #
Total comments: 4
Patch Set 3 : nparker comment #Patch Set 4 : Add OWNERS for c/b/r/protobufs and c/b/r/ssl #Messages
Total messages: 40 (18 generated)
Description was changed from ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 ========== to ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
meacer@chromium.org changed reviewers: + nparker@chromium.org
Nathan, I refactored proto generation code and split it into binary_proto_generator.py. Can you please take a look? Thanks.
https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/protobufs/binary_proto_generator.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:33: print paths I think you'll want to remove any "print"s since they'll show up in people's ninja builds https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:41: self.ImportProtoModule() What does this do? Is this so a child class can override? https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:57: @abc.abstractmethod I'm not familiar with @abc.. is this to ensure it doesn't get called w/o a real impl? https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:95: # TODO(nparker): Remove this once the bug is fixed. That bug is actually fixed, but we still need this work around (remove ref to that bug). I proposed a better fix in http://crbug.com/614082, which I'd be glad for you to try. Could be in this CL, or a later one. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:102: help='The ASCII DownloadFileType-proto file to read.') remove reference to the DownloadFileType file name. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:110: 'the known_certs_pb2.py and ' + Maybe generalize the known_certs_pb2.py to "your_proto_definition_pb2.py" or some such https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:18: *[os.path.pardir] * 5 + ['chrome/browser/resources/protobufs'])) Add a comment as to what this does, and what is the magic 5? https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:171: def AddExtraCommandLineArgsForVirtualEnvRun(self, opts, command): Maybe this could be automatic... like you could just copy all the command line args. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn:5: # TODO(nparker): reduce the duplication between these two, somehow. rm comment, since there's only one action here. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn:7: # Generate the binary proto form of "file_types" from the ascii proto. remove references to "file_types" https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py:13: proto_generator_path = os.path.normpath(os.path.join(os.path.abspath(__file__), Add a comment describing this magic incantation https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py:28: assert pb.version_id > 0; How about verifying that there's at least one entry in the proto? That'll help ensure some failure of parsing or a failure to reach the ascii file doesn't end up with a successful (but wrong) binary proto. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/tls_error_assistant.asciipb (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/tls_error_assistant.asciipb:12: sha256_hash: "testcert" Are these binary strings, or hex? Do you want them in sorted order? Maybe not... maybe you could just check in the python that there are no duplicates, and that all are 32 bytes. Expect typos here... :-) Maybe each hash should have some # comments as to what it is and why it was added? Something to think about. You could add instructions here, or in a .md file like I did.
https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/protobufs/binary_proto_generator.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:33: print paths On 2016/12/10 00:56:50, Nathan Parker wrote: > I think you'll want to remove any "print"s since they'll show up in people's > ninja builds Eeek, of course. Debugging artifact :) https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:41: self.ImportProtoModule() On 2016/12/10 00:56:50, Nathan Parker wrote: > What does this do? Is this so a child class can override? Yes, it's an abstract method to be implemented by the child class. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:57: @abc.abstractmethod On 2016/12/10 00:56:50, Nathan Parker wrote: > I'm not familiar with @abc.. is this to ensure it doesn't get called w/o a real > impl? abc stands for abstract base class, and yes the annotation is to ensure the child class implements the method. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:95: # TODO(nparker): Remove this once the bug is fixed. On 2016/12/10 00:56:50, Nathan Parker wrote: > That bug is actually fixed, but we still need this work around (remove ref to > that bug). I proposed a better fix in http://crbug.com/614082, which I'd be > glad for you to try. Could be in this CL, or a later one. Updated the reference. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:102: help='The ASCII DownloadFileType-proto file to read.') On 2016/12/10 00:56:50, Nathan Parker wrote: > remove reference to the DownloadFileType file name. Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/protobufs/binary_proto_generator.py:110: 'the known_certs_pb2.py and ' + On 2016/12/10 00:56:50, Nathan Parker wrote: > Maybe generalize the known_certs_pb2.py to "your_proto_definition_pb2.py" or > some such Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:18: *[os.path.pardir] * 5 + ['chrome/browser/resources/protobufs'])) On 2016/12/10 00:56:50, Nathan Parker wrote: > > Add a comment as to what this does, and what is the magic 5? Added a comment. 5 is because we need to go 5 levels up to the root of the source directory. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/sa... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:171: def AddExtraCommandLineArgsForVirtualEnvRun(self, opts, command): On 2016/12/10 00:56:50, Nathan Parker wrote: > Maybe this could be automatic... like you could just copy all the command line > args. Do you mean copying the extra options we add in AddCommandLineOptions? I guess it's possible, but it seems quite a bit of work to do that, and I'm not sure how to reliably differentiate between arguments with a value (-t) and those that don't (-a). https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn:5: # TODO(nparker): reduce the duplication between these two, somehow. On 2016/12/10 00:56:50, Nathan Parker wrote: > rm comment, since there's only one action here. Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/BUILD.gn:7: # Generate the binary proto form of "file_types" from the ascii proto. On 2016/12/10 00:56:50, Nathan Parker wrote: > remove references to "file_types" Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py:13: proto_generator_path = os.path.normpath(os.path.join(os.path.abspath(__file__), On 2016/12/10 00:56:50, Nathan Parker wrote: > Add a comment describing this magic incantation Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/gen_tls_error_assistant_proto.py:28: assert pb.version_id > 0; On 2016/12/10 00:56:50, Nathan Parker wrote: > How about verifying that there's at least one entry in the proto? That'll help > ensure some failure of parsing or a failure to reach the ascii file doesn't end > up with a successful (but wrong) binary proto. Done. https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... File chrome/browser/resources/ssl/tls_error_assistant/tls_error_assistant.asciipb (right): https://codereview.chromium.org/2567483002/diff/1/chrome/browser/resources/ss... chrome/browser/resources/ssl/tls_error_assistant/tls_error_assistant.asciipb:12: sha256_hash: "testcert" On 2016/12/10 00:56:50, Nathan Parker wrote: > Are these binary strings, or hex? Do you want them in sorted order? Maybe > not... maybe you could just check in the python that there are no duplicates, > and that all are 32 bytes. Expect typos here... :-) > > Maybe each hash should have some # comments as to what it is and why it was > added? Something to think about. You could add instructions here, or in a .md > file like I did. Added some comments to describe the format. I'm planning to add the readme once I have better validation code and the component lands. Does that sgty?
lgtm Can you try running the following to ensure it still works? chrome/browser/resources/safe_browsing/push_file_type_proto.py -d out-gn/Debug You can go as far as hitting 'y' when it asks for confirmation, since it should then fail on an ACL. And if it succeeded, it would realize you didn't have a new version_id and it'd be a NOOP. https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/protobufs/binary_proto_generator.py (right): https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... chrome/browser/resources/protobufs/binary_proto_generator.py:45: ifile = open(opts.infile, 'r') nit: Could this be one line, ascii_pb_str = open(..).read() https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right): https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:21: *[os.path.pardir] * 5 + ['chrome/browser/resources/protobufs'])) Is it non-standard to just do '../../../../../chrome/browser...." That's easier to read, and is copy-pastable.
I ran the command and said Y to the question, and got this: http://go/crrev_2567483002 Looks like the file is generated successfully? https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/protobufs/binary_proto_generator.py (right): https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... chrome/browser/resources/protobufs/binary_proto_generator.py:45: ifile = open(opts.infile, 'r') On 2016/12/15 00:54:30, Nathan Parker wrote: > nit: Could this be one line, > ascii_pb_str = open(..).read() Converted to a with block (so that the file is always closed, even when there is a read error) https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right): https://codereview.chromium.org/2567483002/diff/20001/chrome/browser/resource... chrome/browser/resources/safe_browsing/gen_file_type_proto.py:21: *[os.path.pardir] * 5 + ['chrome/browser/resources/protobufs'])) On 2016/12/15 00:54:30, Nathan Parker wrote: > Is it non-standard to just do '../../../../../chrome/browser...." > That's easier to read, and is copy-pastable. I took this pattern from other python files (see https://cs.chromium.org/search/?q=%22*%5Bos.path.pardir%5D%22&sq=package:chro... for examples). I think the reason it's done this way is because the separator character is different between Windows and other OS (though Windows should accept both forward and backward slashes). It would also not completely be pastable since the number of parent references would be different between this file and the other (one has 5 "../", the other has 6)
meacer@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: Can you please take a look as owner? Thanks.
Yup, LGTM.
why are you adding these tools to chrome/browser/resources?
To clarify, there is only one new tool being in this CL. It's added under c/b/r/tls_error_reporter because it has the same purpose and shares code with the existing tool at c/b/r/safe_browsing (to generate binary protobufs that will be loaded as resources). The other file at c/b/r/protobufs is a refactor of the existing tool at c/b/r/safe_browsing. Are you suggesting moving these somewhere else? That would mean moving existing code at c/b/r/safe_browsing too.
On 2016/12/15 21:48:46, Mustafa Emre Acer wrote: > To clarify, there is only one new tool being in this CL. It's added under > c/b/r/tls_error_reporter because it has the same purpose and shares code with > the existing tool at c/b/r/safe_browsing (to generate binary protobufs that will > be loaded as resources). > > The other file at c/b/r/protobufs is a refactor of the existing tool at > c/b/r/safe_browsing. > > Are you suggesting moving these somewhere else? That would mean moving existing > code at c/b/r/safe_browsing too. chrome/browser/resources has traditionally been for resources backing web-based UIs with access to the browser process (as far as I know). do either of these scripts fit that charter? Could these .py files go in tools/ instead, maybe?
On 2016/12/16 01:11:10, Dan Beam wrote: > On 2016/12/15 21:48:46, Mustafa Emre Acer wrote: > > To clarify, there is only one new tool being in this CL. It's added under > > c/b/r/tls_error_reporter because it has the same purpose and shares code with > > the existing tool at c/b/r/safe_browsing (to generate binary protobufs that > will > > be loaded as resources). > > > > The other file at c/b/r/protobufs is a refactor of the existing tool at > > c/b/r/safe_browsing. > > > > Are you suggesting moving these somewhere else? That would mean moving > existing > > code at c/b/r/safe_browsing too. > > chrome/browser/resources has traditionally been for resources backing web-based > UIs with access to the browser process (as far as I know). do either of these > scripts fit that charter? > > Could these .py files go in tools/ instead, maybe? These resources aren't shown in any UI. nparker: WDYT about moving them and the existing SafeBrowsing resources to src/tools?
Moving to tools/ is fine, but these script do create resources under chrome/browser/resources/*. So it makes sense to me that the code and data live near each other, ya? Longer term we could get rid of the need for generating/loading a Resource Bundle by switching it to a Bundled Component (http://crbug.com/654037). That doesn't really answer where the code should live though.
On 2016/12/16 21:22:11, Nathan Parker wrote: > Moving to tools/ is fine, but these script do create resources under > chrome/browser/resources/*. So it makes sense to me that the code and data live > near each other, ya? What's created in chrome/browser/resources? > > Longer term we could get rid of the need for generating/loading a Resource > Bundle by switching it to a Bundled Component (http://crbug.com/654037). That > doesn't really answer where the code should live though.
In both the existing download_file_types and this component, it's the binary proto that's stored as a resource and fetched into RAM at startup. e.g.: https://cs.chromium.org/chromium/src/chrome/common/safe_browsing/file_type_po...
if that's the case (and it is a resource loaded by chrome browser), chrome/browser/resources/ is probably fine. so, lgtm stamp without really looking into (because it's unlikely i'd understand much) the logic of the script maybe add some per-file owners for this?
On 2016/12/16 21:57:22, Dan Beam wrote: > if that's the case (and it is a resource loaded by chrome browser), > chrome/browser/resources/ is probably fine. > > so, lgtm stamp without really looking into (because it's unlikely i'd understand > much) the logic of the script Thanks! > > maybe add some per-file owners for this? Added OWNERS. Nathan, I added you and I to chrome/browser/resources/protobufs/OWNERS. Let me know I should add others.
lgtm
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2567483002/#ps60001 (title: "Add OWNERS for c/b/r/protobufs and c/b/r/ssl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481936262319390,
"parent_rev": "d27427881507b16a5ddd72a886bd18bb4ac07034", "commit_rev":
"056f38c15acecdc68285936cbbc7ac1263b47001"}
Message was sent while issue was closed.
Description was changed from ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567483002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567483002 ========== to ========== Add proto for TLS error assistant, refactor proto generator code. This CL adds binary_proto_generator.py which contains code to generate binary protobufs from ascii protobufs. SafeBrowsing download file types list and TLS error assistant use this generator to generate their respective binary protos. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/657c5e7bbb7ea1ae591d97de9adf46a03410a37c Cr-Commit-Position: refs/heads/master@{#439292} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/657c5e7bbb7ea1ae591d97de9adf46a03410a37c Cr-Commit-Position: refs/heads/master@{#439292} |
