Cpp bindings: Return false from Validator::Accept() on unrecognized message or invalid flags

Cpp bindings: Modifies template to return false from Validator::Accept() on unrecognized message or invalid flags.

In RequestValidator::Accept() and ResponseValidatorAccept() we now check two additional things about the message header:
(i) That the flags are correct
(ii) That the method index is in range.


BUG=455295
TEST=None
R=hansmuller@google.com, yzshen@chromium.org, hansmuller@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/81563bfe14ea7ccff637e51df9d05ebf96e01365

[rudominer, 2015-02-13 17:38:15]

Would you like separate CLs for each language or a single one containing all
languages?

Also I'm having trouble figuring out if we have any testing infrastructure that
would allow me to unit test this. The functional behavior in Mojo is not changed
since we already reject in the dispatch layer. So I think an app test won't
help. I would need a test that generates the CPP and then lets me invoke the
Validator::Accept() method directly.

[qsr, 2015-02-16 16:34:53]

I think the validation tests might do what you want:
mojo/public/cpp/bindings/tests/validation_unittest.cc

[yzshen1, 2015-02-17 19:38:18]

On 2015/02/16 16:34:53, qsr wrote:
> I think the validation tests might do what you want:
> mojo/public/cpp/bindings/tests/validation_unittest.cc

+1 for what qsr said.

I think the best way is to add a new validation error type, and some validation
tests.
Please see
https://docs.google.com/document/d/1Zx5tWZOLvMch9SPAHMOeVzFonhHq9KmO9hrg-Bwfs...
(And see "testing", especially the part related to "conformance_".)

[yzshen1, 2015-02-17 19:39:35]

https://codereview.chromium.org/921223002/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/921223002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:278:
return false;
You also need to do the same for RequestValidator.

Besides, we now can reject messages if the flags are unexpected, too.

[rudominer, 2015-02-23 19:02:41]

On 2015/02/16 16:34:53, qsr wrote:
> I think the validation tests might do what you want:
> mojo/public/cpp/bindings/tests/validation_unittest.cc

Thank you. I learned a lot by understanding this test.

[yzshen1, 2015-02-23 19:08:02]

https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/validation_errors.h:52:
VALIDATION_ERROR_UNKOWN_METHOD
Please add comments.

https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data
(right):

https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data:3:
[u4]11                 // There is no Method11
Please use a huge number.

Because we will add more methods to this interface for sure (for example, in a
pending CL I am going to add Methd11
https://codereview.chromium.org/951573002/), by using a huge number we don't
need to update this test frequently.

https://codereview.chromium.org/921223002/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/921223002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:231:
if (message->has_flag(mojo::internal::kMessageExpectsResponse) ||
Please remember to reject in this case as well as we discussed.

(Okay to be in a separate CL.)

https://codereview.chromium.org/921223002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:269:
if (!message->has_flag(mojo::internal::kMessageIsResponse))
ditto

[rudominer, 2015-02-23 19:12:23]

PTAL.

It looks like you have already reviewed the latest patch set before I was ready
for you to. I am still learning how to use this code review tool. I had not
thought I published my changes to you yet.

One concern I have is that ValidationTest is testing whether or not
ReportValidationError() was invoked. It is not testing whether or not |false| is
returned.

I have not handled rejecting a messages if the flags are unexpected. I will do
that in the next patchset.

https://codereview.chromium.org/921223002/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/921223002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:278:
return false;
On 2015/02/17 19:39:35, yzshen1 wrote:
> You also need to do the same for RequestValidator.
Done
> 
> Besides, we now can reject messages if the flags are unexpected, too.
Haven't done this part yet.

[rudominer, 2015-03-03 01:54:36]

ptal

I know you are on paternity leave so take your time and if you want somebody
else to do this let me know.

https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/validation_errors.h:52:
VALIDATION_ERROR_UNKOWN_METHOD
On 2015/02/23 19:08:02, yzshen1 wrote:
> Please add comments.

Done.

https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data
(right):

https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data:3:
[u4]11                 // There is no Method11
On 2015/02/23 19:08:02, yzshen1 wrote:
> Please use a huge number.
> 
> Because we will add more methods to this interface for sure (for example, in a
> pending CL I am going to add Methd11
> https://codereview.chromium.org/951573002/), by using a huge number we don't
> need to update this test frequently.
> 

Done.

[viettrungluu, 2015-03-03 02:02:32]

On 2015/03/03 01:54:36, rudominer wrote:
> ptal
> 
> I know you are on paternity leave so take your time and if you want somebody
> else to do this let me know.
> 
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/lib/validation_errors.h (right):
> 
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/validation_errors.h:52:
> VALIDATION_ERROR_UNKOWN_METHOD
> On 2015/02/23 19:08:02, yzshen1 wrote:
> > Please add comments.
> 
> Done.
> 
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
> File
>
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data
> (right):
> 
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
>
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data:3:
> [u4]11                 // There is no Method11
> On 2015/02/23 19:08:02, yzshen1 wrote:
> > Please use a huge number.
> > 
> > Because we will add more methods to this interface for sure (for example, in
a
> > pending CL I am going to add Methd11
> > https://codereview.chromium.org/951573002/), by using a huge number we don't
> > need to update this test frequently.
> > 
> 
> Done.

Drive-by comment: It seems to me that the danger of using a huge number is that
we're then not checking the boundary case, so we're vulnerable to (e.g.)
off-by-one errors. :(

[rudominer, 2015-03-03 02:59:34]

On 2015/03/03 02:02:32, viettrungluu wrote:
> On 2015/03/03 01:54:36, rudominer wrote:
> > ptal
> > 
> > I know you are on paternity leave so take your time and if you want somebody
> > else to do this let me know.
> > 
> >
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
> > File mojo/public/cpp/bindings/lib/validation_errors.h (right):
> > 
> >
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/cpp/bindings...
> > mojo/public/cpp/bindings/lib/validation_errors.h:52:
> > VALIDATION_ERROR_UNKOWN_METHOD
> > On 2015/02/23 19:08:02, yzshen1 wrote:
> > > Please add comments.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
> > File
> >
>
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data
> > (right):
> > 
> >
>
https://codereview.chromium.org/921223002/diff/40001/mojo/public/interfaces/b...
> >
>
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd11_nosuch.data:3:
> > [u4]11                 // There is no Method11
> > On 2015/02/23 19:08:02, yzshen1 wrote:
> > > Please use a huge number.
> > > 
> > > Because we will add more methods to this interface for sure (for example,
in
> a
> > > pending CL I am going to add Methd11
> > > https://codereview.chromium.org/951573002/), by using a huge number we
don't
> > > need to update this test frequently.
> > > 
> > 
> > Done.
> 
> Drive-by comment: It seems to me that the danger of using a huge number is
that
> we're then not checking the boundary case, so we're vulnerable to (e.g.)
> off-by-one errors. :(

I'm happy to replace 9999 with 12 and add a comment saying that this needs to be
updated each time a method is added to the interface. But I'm slightly skeptical
that the benefit outweighs the cost. A boundary error seems unlikely here.

[hansmuller1, 2015-03-03 15:30:39]

hansmuller@google.com changed reviewers:
+ hansmuller@chromium.org

[yzshen1, 2015-03-03 18:05:37]

https://codereview.chromium.org/921223002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:43: case
VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD:
Please use the exact same order as the enum definition.
(May you could move the value in the definition to match this, so that all
"message header" values are together.)

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data
(left):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data:1:
[dist4]message_header  // num_bytes
The name of the test should probably have "msghdr_", just like other message
header tests.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data
(right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data:3:
[u4]9999               // There is no Method9999
Period at the end of a sentence, please.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:262: // TODO(hansmuller): Temporarily
skipping array pointer overflow tests.
This whole if-statement can be removed, because we don't have either "overflow"
or "no_such_method" integration tests.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:226:
mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINATION);
(1) Do you think we should rename this error? -- because the fact that it is
invalid is NOT because the flag "combination" is invalid. The error was designed
to indicate error such as having ExpectsResponse and IsResponse at the same
time.

(2) Please add tests for all the new failures.

[rudominer, 2015-03-04 00:12:01]

ptal

https://codereview.chromium.org/921223002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:43: case
VALIDATION_ERROR_MESSAGE_HEADER_UNKNOWN_METHOD:
On 2015/03/03 18:05:37, yzshen1 (OOO Mar 2 - 22) wrote:
> Please use the exact same order as the enum definition.
> (May you could move the value in the definition to match this, so that all
> "message header" values are together.)

Done.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data
(left):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data:1:
[dist4]message_header  // num_bytes
On 2015/03/03 18:05:37, yzshen1 (OOO Mar 2 - 22) wrote:
> The name of the test should probably have "msghdr_", just like other message
> header tests.

Done.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data
(right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_no_such_method.data:3:
[u4]9999               // There is no Method9999
On 2015/03/03 18:05:37, yzshen1 (OOO Mar 2 - 22) wrote:
> Period at the end of a sentence, please.

Done.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:262: // TODO(hansmuller): Temporarily
skipping array pointer overflow tests.
On 2015/03/03 18:05:37, yzshen1 (OOO Mar 2 - 22) wrote:
> This whole if-statement can be removed, because we don't have either
"overflow"
> or "no_such_method" integration tests.

Done.

https://codereview.chromium.org/921223002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/921223002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:226:
mojo::internal::VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINATION);
On 2015/03/03 18:05:37, yzshen1 (OOO Mar 2 - 22) wrote:
> (1) Do you think we should rename this error? -- because the fact that it is
> invalid is NOT because the flag "combination" is invalid. The error was
designed
> to indicate error such as having ExpectsResponse and IsResponse at the same
> time.
Let's do this in a separate CL. I filed b/463684
> 
> (2) Please add tests for all the new failures.
Done

[hansmuller1, 2015-03-04 21:55:19]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-03-04 21:55:20]

https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:235:
testFiles[i].indexOf("invalid_request_flags") != -1) {
The comment no longer covers all the tests being skipped
(invalid_request_flags).

We don't support the overflow tests because JS numbers are limited to 53 bits.
Why are we skipping anything else?

[rudominer, 2015-03-04 23:05:48]

Hans I updated the comment in the js test. ptal.

https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:235:
testFiles[i].indexOf("invalid_request_flags") != -1) {
On 2015/03/04 21:55:20, hansmuller1 wrote:
> The comment no longer covers all the tests being skipped
> (invalid_request_flags).
> 
> We don't support the overflow tests because JS numbers are limited to 53 bits.
> Why are we skipping anything else?
> 

I modified the comment. ptal.

[rudominer, 2015-03-06 19:40:27]

Yuzhu and Hans, ping.

[hansmuller1, 2015-03-06 20:20:44]

On 2015/03/04 23:05:48, rudominer wrote:
> Hans I updated the comment in the js test. ptal.
> 
>
https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
> File mojo/public/js/validation_unittests.js (right):
> 
>
https://codereview.chromium.org/921223002/diff/140001/mojo/public/js/validati...
> mojo/public/js/validation_unittests.js:235:
> testFiles[i].indexOf("invalid_request_flags") != -1) {
> On 2015/03/04 21:55:20, hansmuller1 wrote:
> > The comment no longer covers all the tests being skipped
> > (invalid_request_flags).
> > 
> > We don't support the overflow tests because JS numbers are limited to 53
bits.
> > Why are we skipping anything else?
> > 
> 
> I modified the comment. ptal.

OK, thanks for documenting why we're temporarily skipping JS tests.

The JS part of this CL LGTM.

[yzshen1, 2015-03-13 22:55:03]

Sorry for the delay. I was sick for a few days.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_unittest.cc:374: // This test is
similar to the Conformance test but for responses.
"Conformance" means "all binding implementations need to pass these tests to
claim that they conform to the validation algorithm".
"Response" sounds like a different way of categorizing tests. But these tests
are still conformance tests.

One idea is to use conformance_rqst_ (request) and conformance_resp_ (response)
as prefix.

Another issue: I am pretty sure that other language bindings won't run these
test cases (unless you make similar changes) because they use prefix to filter
tests as well.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data
(right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data:3:
[u4]12                  // name
nit: please align the comment with other lines.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data:4:
[u4]0                  // flags: kMessageExpectsResponse is not set but
expected.
80 chars?

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data:4:
[u4]0                  // flags: kMessageExpectsResponse is not set but
expected.
Does it make sense to name this test conformance_mthd12_xxx? Because it is not
something that the message header validator can detect by itself. Instead,
knowledge about the actual interface running is required to detect the error.

Similarly, conformance_msghdr_invalid_request_flags1 may be renamed as
conformance_mthd0_xxx. (etc.)

What do you think?

https://codereview.chromium.org/921223002/diff/160001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:231: // in the name) because ...
... -> the feature is not supported in JS yet.

[rudominer, 2015-03-16 21:24:32]

ptal. I believe I addressed all the comments except for the discussion initiated
by Trung regarding detecting an off-by-one error in the method index. I would
prefer to deal with that in a follow-up CL by adding an additional test.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_unittest.cc:374: // This test is
similar to the Conformance test but for responses.
On 2015/03/13 22:55:03, yzshen1 (OOO Mar 2 - 22) wrote:
> "Conformance" means "all binding implementations need to pass these tests to
> claim that they conform to the validation algorithm".
> "Response" sounds like a different way of categorizing tests. But these tests
> are still conformance tests.
> 
> One idea is to use conformance_rqst_ (request) and conformance_resp_
(response)
> as prefix.

I have settled on the "conformance_" prefix for the current request-oriented
tests and "resp_conformance_" prefix for the response-oriented tests. Notice
that there is no real advantage to having the two sets of files have a common
prefix because in every language we need to have the testing code treat the two
sets differently because they use different interfaces. In other words if we
were to use "conformance_req_" and "conformance_resp_" as the prefixes, that
would involve a lot of work to change the existing file names but no real
advantage because the tests would then no longer be able to look for the prefix
"conformance_" but would have to start looking for the two prefixes
conformance_req_" and conformance_resp" separately.
> 
> Another issue: I am pretty sure that other language bindings won't run these
> test cases (unless you make similar changes) because they use prefix to filter
> tests as well.
Right and that is good for now since the tests would break and we would have to
exclude them as we have done with the JS test.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data
(right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data:3:
[u4]12                  // name
On 2015/03/13 22:55:03, yzshen1 (OOO Mar 2 - 22) wrote:
> nit: please align the comment with other lines.

Done.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_msghdr_invalid_request_flags2.data:4:
[u4]0                  // flags: kMessageExpectsResponse is not set but
expected.
On 2015/03/13 22:55:03, yzshen1 (OOO Mar 2 - 22) wrote:
> 80 chars?

Done.

https://codereview.chromium.org/921223002/diff/160001/mojo/public/js/validati...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/921223002/diff/160001/mojo/public/js/validati...
mojo/public/js/validation_unittests.js:231: // in the name) because ...
On 2015/03/13 22:55:03, yzshen1 (OOO Mar 2 - 22) wrote:
> ... -> the feature is not supported in JS yet.

Done.

[yzshen1, 2015-03-23 17:21:30]

LGTM

Thanks!

[rudominer, 2015-03-30 18:24:04]

On 2015/03/23 17:21:30, yzshen1 wrote:
> LGTM
> 
> Thanks!

Thanks for the review. I created
https://code.google.com/p/chromium/issues/detail?id=471803 to track the one
unresolved comment by Trung.

[rudominer, 2015-03-30 19:23:46]

Committed patchset #11 (id:200001) manually as
81563bfe14ea7ccff637e51df9d05ebf96e01365 (presubmit successful).