Fix js conformance tests. (protocolbuffers#3604)

* Fix js conformance tests. * Remove old incorrect compatibility tests
ibmsoe · Sep 12, 2017 · 2bd55a9 · 2bd55a9
1 parent 6a4ffb2
commit 2bd55a9
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 93 deletions.
diff --git a/conformance/failure_list_js.txt b/conformance/failure_list_js.txt
@@ -1,19 +0,0 @@
-Required.Proto3.ProtobufInput.RepeatedScalarSelectsLast.INT32.ProtobufOutput
-Required.Proto3.ProtobufInput.RepeatedScalarSelectsLast.UINT32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.BOOL.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.DOUBLE.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.FIXED32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.FIXED64.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.FLOAT.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.INT32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.INT64.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.SFIXED32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.SFIXED64.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.SINT32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.SINT64.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.UINT32.ProtobufOutput
-Required.Proto3.ProtobufInput.ValidDataRepeated.UINT64.ProtobufOutput
-Required.Proto2.ProtobufInput.RepeatedScalarSelectsLast.INT32.ProtobufOutput
-Required.Proto2.ProtobufInput.RepeatedScalarSelectsLast.UINT32.ProtobufOutput
-Required.Proto2.ProtobufInput.ValidDataRepeated.INT32.ProtobufOutput
-Required.Proto2.ProtobufInput.ValidDataRepeated.UINT32.ProtobufOutput

diff --git a/js/binary/decoder.js b/js/binary/decoder.js
@@ -582,27 +582,24 @@ jspb.BinaryDecoder.prototype.readUnsignedVarint32 = function() {
   x |= (temp & 0x0F) << 28;
   if (temp < 128) {
     // We're reading the high bits of an unsigned varint. The byte we just read
-    // also contains bits 33 through 35, which we're going to discard. Those
-    // bits _must_ be zero, or the encoding is invalid.
-    goog.asserts.assert((temp & 0xF0) == 0);
+    // also contains bits 33 through 35, which we're going to discard.
     this.cursor_ += 5;
     goog.asserts.assert(this.cursor_ <= this.end_);
     return x >>> 0;
   }
 
-  // If we get here, we're reading the sign extension of a negative 32-bit int.
-  // We can skip these bytes, as we know in advance that they have to be all
-  // 1's if the varint is correctly encoded. Since we also know the value is
-  // negative, we don't have to coerce it to unsigned before we return it.
-
-  goog.asserts.assert((temp & 0xF0) == 0xF0);
-  goog.asserts.assert(bytes[this.cursor_ + 5] == 0xFF);
-  goog.asserts.assert(bytes[this.cursor_ + 6] == 0xFF);
-  goog.asserts.assert(bytes[this.cursor_ + 7] == 0xFF);
-  goog.asserts.assert(bytes[this.cursor_ + 8] == 0xFF);
-  goog.asserts.assert(bytes[this.cursor_ + 9] == 0x01);
+  // If we get here, we need to truncate coming bytes. However we need to make
+  // sure cursor place is correct.
+  var i = 5;
+  do {
+    goog.asserts.assert(i < 10);
+    if (bytes[this.cursor_ + i] < 128) {
+      break;
+    }
+    i++;
+  } while (1);
 
-  this.cursor_ += 10;
+  this.cursor_ += i + 1;
   goog.asserts.assert(this.cursor_ <= this.end_);
   return x;
 };

diff --git a/js/binary/decoder_test.js b/js/binary/decoder_test.js
@@ -270,24 +270,7 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-
-    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
-    // should trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 0x1F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x2F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x4F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    // Negative 32-bit varints encoded with non-1 bits in the high dword should
-    // trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
+    decoder.reset();
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

diff --git a/js/compatibility_tests/v3.0.0/binary/decoder_test.js b/js/compatibility_tests/v3.0.0/binary/decoder_test.js
@@ -228,24 +228,7 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-
-    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
-    // should trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 0x1F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x2F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x4F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    // Negative 32-bit varints encoded with non-1 bits in the high dword should
-    // trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
+    decoder.reset();
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

diff --git a/js/compatibility_tests/v3.1.0/binary/decoder_test.js b/js/compatibility_tests/v3.1.0/binary/decoder_test.js
@@ -228,24 +228,7 @@ describe('binaryDecoderTest', function() {
     assertThrows(function() {decoder.readSignedVarint64()});
     decoder.reset();
     assertThrows(function() {decoder.readZigzagVarint64()});
-
-    // Positive 32-bit varints encoded with 1 bits in positions 33 through 35
-    // should trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 0x1F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x2F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 0x4F]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    // Negative 32-bit varints encoded with non-1 bits in the high dword should
-    // trigger assertions.
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 0, 255, 255, 1]);
-    assertThrows(function() {decoder.readUnsignedVarint32()});
-
-    decoder.setBlock([255, 255, 255, 255, 255, 255, 255, 255, 255, 0]);
+    decoder.reset();
     assertThrows(function() {decoder.readUnsignedVarint32()});
   });
 

diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc
@@ -2876,6 +2876,29 @@ void Generator::GenerateClassDeserializeBinaryField(
                         "Group" : "Message",
           "grpfield", (field->type() == FieldDescriptor::TYPE_GROUP) ?
                       (SimpleItoa(field->number()) + ", ") : "");
+    } else if (field->is_repeated() &&
+               field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
+      printer->Print(
+          "      if (reader.getWireType() == 2) {\n"
+          "        var value = /** @type {$fieldtype_packed$} */ "
+          "(reader.readPacked$reader$());\n"
+          "        msg.set$list_name$(value);\n"
+          "      } else {\n"
+          "        var value = /** @type {$fieldtype$} */ "
+          "(reader.read$reader$());\n"
+          "        msg.add$name$(value);\n"
+          "      }\n",
+          "fieldtype_packed", JSFieldTypeAnnotation(options, field, false, true,
+                                             /* singular_if_not_packed */ false,
+                                             BYTES_U8),
+          "fieldtype", JSFieldTypeAnnotation(options, field, false, true,
+                                             /* singular_if_not_packed */ true,
+                                             BYTES_U8),
+          "reader", JSBinaryReaderMethodType(field),
+          "list_name", JSGetterName(options, field),
+          "name", JSGetterName(options, field,
+                               BYTES_DEFAULT, /* drop_list = */ true)
+      );
     } else {
       printer->Print(
           "      var value = /** @type {$fieldtype$} */ "
@@ -2887,14 +2910,15 @@ void Generator::GenerateClassDeserializeBinaryField(
           JSBinaryReadWriteMethodName(field, /* is_writer = */ false));
     }
 
-    if (field->is_repeated() && !field->is_packed()) {
+    if (field->is_repeated() &&
+        (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE ||
+         field->cpp_type() == FieldDescriptor::CPPTYPE_STRING)) {
       printer->Print(
           "      msg.add$name$(value);\n", "name",
           JSGetterName(options, field, BYTES_DEFAULT, /* drop_list = */ true));
-    } else {
-      // Singular fields, and packed repeated fields, receive a |value| either
-      // as the field's value or as the array of all the field's values; set
-      // this as the field's value directly.
+    } else if (!field->is_repeated()) {
+      // Singular fields, receive a |value| as the field's value ; set this as
+      // the field's value directly.
       printer->Print(
           "      msg.set$name$(value);\n",
           "name", JSGetterName(options, field));