Mojo: validate nullability in Java serialization.

Mojo: validate nullability in Java serialization.

This patch adds nullability validation of handles and
pointers upon struct serialization in the Java bindings.

BUG=324170
Committed: https://crrev.com/1cb929f1138d7c052913966943c39cc8030a846c
Cr-Commit-Position: refs/heads/master@{#293120}

[ppi, 2014-09-01 19:00:44]

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

[ppi, 2014-09-01 19:00:45]

Hi Ben,

ptal.

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

https://codereview.chromium.org/524703004/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:238: if (v
== null || !v.isValid()) {
What do we do if the field is not nullable, v != null and !v.isValid() ?

[qsr, 2014-09-02 07:41:44]

This time, having some tests would be nice.

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

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:229: final
int arrayNullability = nullable ? BindingsHelper.ARRAY_NULLABLE : 0;
You have a constant instead of this 0 in BindingsHelper, no?

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:238: if (v
== null || !v.isValid()) {
On 2014/09/01 19:00:45, ppi wrote:
> What do we do if the field is not nullable, v != null and !v.isValid() ?

Non nullable for a handle means valid, so this case is an exception.

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

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:220:
params.append('%s.MANAGER' % GetJavaType(context, kind.kind))
It might be time to refactor the append part in a method an use it in both
encode and decode.

[ppi, 2014-09-02 16:50:43]

ptal

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

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:229: final
int arrayNullability = nullable ? BindingsHelper.ARRAY_NULLABLE : 0;
On 2014/09/02 07:41:44, qsr wrote:
> You have a constant instead of this 0 in BindingsHelper, no?

Done.

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:238: if (v
== null || !v.isValid()) {
On 2014/09/02 07:41:44, qsr wrote:
> On 2014/09/01 19:00:45, ppi wrote:
> > What do we do if the field is not nullable, v != null and !v.isValid() ?
> 
> Non nullable for a handle means valid, so this case is an exception.

Acknowledged.

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

https://chromiumcodereview.appspot.com/524703004/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:220:
params.append('%s.MANAGER' % GetJavaType(context, kind.kind))
On 2014/09/02 07:41:44, qsr wrote:
> It might be time to refactor the append part in a method an use it in both
> encode and decode.

Done.

[qsr, 2014-09-03 07:56:28]

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
File
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java
(right):

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:33:
assertTrue(exceptionThrown);
Instead of this, you can do:

try {
    struct.serialize(null);
    fail("Serialization should have thrown an exception.");
} catch (SerializationException ex) {
    // Expected.
}

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:116:
struct.str = new String();
= "" instead?

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:121:
* Verifies that a struct with an invalid nullable handle and a null nullable
pointer serializes
nit: There is more than one null nullable pointer

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/public/tools...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:187: return params
This is modifying its argument, you might not want to return something, because
otherwise user might think that the first argument is not modified.

Or if you indeed prefer to return something, then make a copy of the array at
the beginning.

[ppi, 2014-09-03 08:53:46]

Thanks, ptal.

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
File
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java
(right):

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:33:
assertTrue(exceptionThrown);
On 2014/09/03 07:56:28, qsr wrote:
> Instead of this, you can do:
> 
> try {
>     struct.serialize(null);
>     fail("Serialization should have thrown an exception.");
> } catch (SerializationException ex) {
>     // Expected.
> }

Done.

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:116:
struct.str = new String();
On 2014/09/03 07:56:28, qsr wrote:
> = "" instead?

Done.

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/SerializationTest.java:121:
* Verifies that a struct with an invalid nullable handle and a null nullable
pointer serializes
On 2014/09/03 07:56:28, qsr wrote:
> nit: There is more than one null nullable pointer

Done.

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/public/tools...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/524703004/diff/40001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:187: return params
On 2014/09/03 07:56:28, qsr wrote:
> This is modifying its argument, you might not want to return something,
because
> otherwise user might think that the first argument is not modified.
> 
> Or if you indeed prefer to return something, then make a copy of the array at
> the beginning. 

Done.

[qsr, 2014-09-03 09:41:05]

lgtm

[ppi, 2014-09-03 09:43:26]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-09-03 09:43:49]

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

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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-09-03 11:31:14]

Committed patchset #4 (id:60001) as f8b34456a6807bf41b4958f924ec1e597da45f50

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

Patchset 4 (id:??) landed as
https://crrev.com/1cb929f1138d7c052913966943c39cc8030a846c
Cr-Commit-Position: refs/heads/master@{#293120}