Mojo: validate nullability in Java bindings.

Mojo: validate nullability in Java bindings.

This is the message validation / receiver end side.

BUG=324170
Committed: https://crrev.com/72e2f7bbab8a344bec72d2a915504aaf9ac17448
Cr-Commit-Position: refs/heads/master@{#292652}

[ppi, 2014-08-28 18:37:49]

ppi@chromium.org changed reviewers:
+ qsr@chromium.org

[ppi, 2014-08-28 18:37:49]

Hi Ben,

This is WIP, ptal at the question below.

https://codereview.chromium.org/514293002/diff/1/mojo/public/java/bindings/sr...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/514293002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:413:
DataHeader.HEADER_SIZE + BindingsHelper.SERIALIZED_HANDLE_SIZE * i, true);
I just realized we have these. I guess we can just check the nullability of
kind.kind in DecodeMethod and pass two levels of nullable here (|nullable| and
... |elements_nullable|?). Does that sound reasonable?

[qsr, 2014-08-29 07:56:20]

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/java/bind...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:413:
DataHeader.HEADER_SIZE + BindingsHelper.SERIALIZED_HANDLE_SIZE * i, true);
On 2014/08/28 18:37:49, ppi wrote:
> I just realized we have these. I guess we can just check the nullability of
> kind.kind in DecodeMethod and pass two levels of nullable here (|nullable| and
> ... |elements_nullable|?). Does that sound reasonable?

Hum... The issue I have with this is that we start to accumulate boolean and the
call site is not readable at all.

What about passing an int as a flag, with 2 boolean flags for now -> nullable,
and element_nullable?

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:332:
"is_nullable_kind": mojom.IsNullableKind,
The java_filters is only there to make method available to template. Methods
that you use locally must not be added to the flags, so you do not need either
of those 2.

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:385: return
isinstance(kind, ReferenceKind) and kind.is_nullable
Maybe reuse your new method here.

[ppi, 2014-08-29 13:04:04]

Patchset #3 (id:40001) has been deleted

[ppi, 2014-08-29 13:25:45]

Patchset #5 (id:100001) has been deleted

[ppi, 2014-08-29 14:03:34]

Thanks, ptal.

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/java/bind...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:413:
DataHeader.HEADER_SIZE + BindingsHelper.SERIALIZED_HANDLE_SIZE * i, true);
On 2014/08/29 07:56:20, qsr wrote:
> On 2014/08/28 18:37:49, ppi wrote:
> > I just realized we have these. I guess we can just check the nullability of
> > kind.kind in DecodeMethod and pass two levels of nullable here (|nullable|
and
> > ... |elements_nullable|?). Does that sound reasonable?
> 
> Hum... The issue I have with this is that we start to accumulate boolean and
the
> call site is not readable at all.
> 
> What about passing an int as a flag, with 2 boolean flags for now -> nullable,
> and element_nullable?

Done.

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:332:
"is_nullable_kind": mojom.IsNullableKind,
On 2014/08/29 07:56:20, qsr wrote:
> The java_filters is only there to make method available to template. Methods
> that you use locally must not be added to the flags, so you do not need either
> of those 2.

I do use both of them in macro decode of struct_definition.tmpl.

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://chromiumcodereview.appspot.com/514293002/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:385: return
isinstance(kind, ReferenceKind) and kind.is_nullable
On 2014/08/29 07:56:20, qsr wrote:
> Maybe reuse your new method here.

Done.

[qsr, 2014-08-29 14:17:57]

You probably want to write some tests. Encoding won't be handled by the cross
platform validation tests.

https://chromiumcodereview.appspot.com/514293002/diff/140001/do_not_commit/Fo...
File do_not_commit/Foo.java (right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/do_not_commit/Fo...
do_not_commit/Foo.java:76: result.data = decoder0.readBytes(32,
org.chromium.mojo.bindings.Decoder.ARRAY_NULLABLE);
Should probably be in BindingsHelper and not Decoder -> You will need the same
to test for encoding issues.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/android/jav...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:70: {
true, false, true }, { }, {}, { false }, { true } };
No space inside, or add space on the next one.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:98: public
static final int NOTHING_NULLABLE = 0;
What about putting this flag first?

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:218: final
boolean arrayNullable = (arrayNullability & ARRAY_NULLABLE) > 0;
What do you think about having a static helper method to extract boolean from
the flag?

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:438:
DataHeader.HEADER_SIZE + BindingsHelper.SERIALIZED_HANDLE_SIZE * i, true);
Don't you want to use your flag here instead of true? Here and below?

[qsr, 2014-08-29 14:18:46]

Hum... Ok, I'm stupid. This is the deserialization side. It will be handles by
the cross platform tests.

[ppi, 2014-08-29 14:35:49]

Thanks, ptal.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/android/jav...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:70: {
true, false, true }, { }, {}, { false }, { true } };
On 2014/08/29 14:17:57, qsr wrote:
> No space inside, or add space on the next one.

Done.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:98: public
static final int NOTHING_NULLABLE = 0;
On 2014/08/29 14:17:57, qsr wrote:
> What about putting this flag first?

Done.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:218: final
boolean arrayNullable = (arrayNullability & ARRAY_NULLABLE) > 0;
On 2014/08/29 14:17:57, qsr wrote:
> What do you think about having a static helper method to extract boolean from
> the flag?

Done.

https://chromiumcodereview.appspot.com/514293002/diff/140001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:438:
DataHeader.HEADER_SIZE + BindingsHelper.SERIALIZED_HANDLE_SIZE * i, true);
On 2014/08/29 14:17:57, qsr wrote:
> Don't you want to use your flag here instead of true? Here and below?

Done.

[ppi, 2014-08-29 14:46:49]

Ptal.

https://chromiumcodereview.appspot.com/514293002/diff/140001/do_not_commit/Fo...
File do_not_commit/Foo.java (right):

https://chromiumcodereview.appspot.com/514293002/diff/140001/do_not_commit/Fo...
do_not_commit/Foo.java:76: result.data = decoder0.readBytes(32,
org.chromium.mojo.bindings.Decoder.ARRAY_NULLABLE);
On 2014/08/29 14:17:57, qsr wrote:
> Should probably be in BindingsHelper and not Decoder -> You will need the same
> to test for encoding issues.

Done.

[qsr, 2014-08-29 14:49:46]

lgtm

[ppi, 2014-08-29 14:53:59]

Thanks!

[ppi, 2014-08-29 14:54:03]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-08-29 14:55:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ppi@chromium.org/514293002/220001

[commit-bot: I haz the power, 2014-08-29 15:09:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48149)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53479)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 15:11:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-29 15:11:54]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ppi, 2014-08-29 15:15:27]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-08-29 15:16:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ppi@chromium.org/514293002/230001

[commit-bot: I haz the power, 2014-08-29 16:10:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[commit-bot: I haz the power, 2014-08-29 16:21:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-29 16:21:48]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[qsr, 2014-08-29 16:24:39]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-29 16:25:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ppi@chromium.org/514293002/230001

[commit-bot: I haz the power, 2014-08-29 16:39:34]

Committed patchset #11 (id:230001) as 97f112b52a359bd381d87c225cc733523ad2bb98

[commit-bot: I haz the power, 2014-09-10 03:09:16]

Patchset 11 (id:??) landed as
https://crrev.com/72e2f7bbab8a344bec72d2a915504aaf9ac17448
Cr-Commit-Position: refs/heads/master@{#292652}