Validate incoming JS Message Headers

Validate incoming JS Message Headers

This is an incremental step towards incoming message validation for JS in general.

BUG=395801

[hansmuller, 2014-07-21 22:50:59]

Most of this patch's bulk is the test message ".data" parser and its internal
tests.

It still may make sense to break the patch into smaller parts.

[yzshen1, 2014-07-21 23:41:16]

On 2014/07/21 22:50:59, hansmuller wrote:
> Most of this patch's bulk is the test message ".data" parser and its internal
> tests.
> 
> It still may make sense to break the patch into smaller parts.

I think we could consider breaking this CL into a few CLs:

- the parser (and parser unittest). The C++ parser unittest is here:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
(Please note that the unittest doesn't read files from disk.)

- the message header validator and changes to router/connector.
The C++ impl designs those validators as "filters", chain them together, and
attach them to the connector. If possible, I wish the JS impl could follow a
similar design so that it is easier for us to maintain both of them.

- the file IO change and validation tests.

What do you think? Thanks!

[hansmuller, 2014-07-22 00:13:53]

On 2014/07/21 23:41:16, yzshen1 wrote:
> On 2014/07/21 22:50:59, hansmuller wrote:
> > Most of this patch's bulk is the test message ".data" parser and its
internal
> > tests.
> > 
> > It still may make sense to break the patch into smaller parts.
> 
> I think we could consider breaking this CL into a few CLs:
> 
> - the parser (and parser unittest). The C++ parser unittest is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
> (Please note that the unittest doesn't read files from disk.)

My version of the ValidationTest from validation_unittest.cc - see
checkMessageFileParser() in message_file_parser.js, line 384 - doesn't read
files from disk either.  It would be nice to put the parser test in a separate
file however that would mean exporting more symbols from message_file_parser.js.
I will give that a try.

> - the message header validator and changes to router/connector.
> The C++ impl designs those validators as "filters", chain them together, and
> attach them to the connector. If possible, I wish the JS impl could follow a
> similar design so that it is easier for us to maintain both of them.

I was going to manage the TBD type-specific validators with a similar filter
list.

> - the file IO change and validation tests.

OK.  That will make 3 CLs.

[abarth-chromium, 2014-07-22 03:51:01]

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc
File mojo/bindings/js/file_io.cc (right):

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:15: #include
"mojo/public/cpp/test_support/test_support.h"
This module shouldn't need anything from Mojo.  I can be in //gin/test

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:43: size_t size_read = fread(&result.at(0), 1, size,
fp);
I thought there was a function in base that did all this work for you.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:51: gin::WrapperInfo g_wrapper_info = {
gin::kEmbedderNativeGin };
Bad indent

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:55: const char FileIO::kModuleName[] =
"mojo/public/js/bindings/fileio";
This path should match the location of the C++ code.  In particular, this module
isn't part of the public API and therefore should not have "public" in its name.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
File mojo/bindings/js/message_file_parser.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:9: ], function (console, expect, fileio)
{
If this module is for testing, should it be in a "test" directory?

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:19: })();
This code looks copy/pasted from codec.js.  Should we factor it into a module so
they can share?

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/rout...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/rout...
mojo/public/js/bindings/router.js:6: "console",
Production code shouldn't use |console|

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:5:
define("mojo/public/js/bindings/validator", [
No need to name the module.  The module loader will imply the name from the
path.

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:13: }
Normally we'd just make this an anonymous function in the forEach call

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:14: ["NONE",
Blank line after [

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:22: "MESSAGE_HEADER_MISSING_REQUEST_ID"
Missing , at the end of this line.

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:25: })();
Why not just write out the code?  It seems unnecessary to do all this work at
module load time.

[hansmuller, 2014-07-22 15:49:07]

I'm breaking this CL into 3 parts per Yuzhu's feedback and will include the
changes you suggested.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc
File mojo/bindings/js/file_io.cc (right):

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:15: #include
"mojo/public/cpp/test_support/test_support.h"
On 2014/07/22 03:51:00, abarth wrote:
> This module shouldn't need anything from Mojo.  I can be in //gin/test

I'm loading mojo data files from a source relative pathname and so I used
test::OpenSourceRootRelativeFile(). It looks like I can get the root "src"
pathname from  PathService::Get(base::DIR_SOURCE_ROOT, &path). I could add
support for looking up the root src path to a //gin/test FileIO module as well,
and eliminate this dependency.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:43: size_t size_read = fread(&result.at(0), 1, size,
fp);
On 2014/07/22 03:51:00, abarth wrote:
> I thought there was a function in base that did all this work for you.

You are correct: base::ReadFileToString(). I'll use that here.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:51: gin::WrapperInfo g_wrapper_info = {
gin::kEmbedderNativeGin };
On 2014/07/22 03:51:00, abarth wrote:
> Bad indent
Will fix.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/file_io.cc#...
mojo/bindings/js/file_io.cc:55: const char FileIO::kModuleName[] =
"mojo/public/js/bindings/fileio";
On 2014/07/22 03:51:00, abarth wrote:
> This path should match the location of the C++ code.  In particular, this
module
> isn't part of the public API and therefore should not have "public" in its
name.

OK. And if I'm reading you correctly, the FileIO module should end up in
gin/test, not in mojo at all.

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
File mojo/bindings/js/message_file_parser.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:9: ], function (console, expect, fileio)
{
On 2014/07/22 03:51:00, abarth wrote:
> If this module is for testing, should it be in a "test" directory?

Do you mean a test subdirectory below /mojo/bindings/js? Should the unit tests
from this directory move there as well?

https://codereview.chromium.org/406993002/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:19: })();
On 2014/07/22 03:51:00, abarth wrote:
> This code looks copy/pasted from codec.js.  Should we factor it into a module
so
> they can share?

Yes. I'd wanted to do as much in a separate CL since it will impact the classes
in codec.js as well.

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/rout...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/rout...
mojo/public/js/bindings/router.js:6: "console",
On 2014/07/22 03:51:00, abarth wrote:
> Production code shouldn't use |console|

OK, I'll remove that.

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:5:
define("mojo/public/js/bindings/validator", [
On 2014/07/22 03:51:00, abarth wrote:
> No need to name the module.  The module loader will imply the name from the
> path.
OK.

https://codereview.chromium.org/406993002/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validator.js:25: })();
On 2014/07/22 03:51:00, abarth wrote:
> Why not just write out the code?  It seems unnecessary to do all this work at
> module load time.

Sorry, I got carried away. I'll write out the constants (which should address
the other comments about this enum.