Add a missing ZipArchiver operation define: READ_METADATA

READ_METADATA was defined in the NaCL C++ but wasn't defined in the JS
code. It is a requirement of the ZipArchiver design that the operation
values are defined, and have the same values, in the C++ and JS code.

This is likely causing crashes in the field: it certainly caused CHECK
stop crashes in DEBUG C++ NaCL code in integration test (ZipArchiver's
unit_tests did not exhibit the bug at all).

Define READ_METADATA in the JS, add protective code to check operation
validity (make it throw, if not) when constructing pack messages in JS
that will be sent to the NaCL module C++ code.

Re-enable the zip integration tests that were disabled due to this bug
in issue 867738. (Those tests discovered bug 867842).

Test: browser_test --gtest-filter="Zip*FilesApp*"
No-try: true
Bug: 867842, 867738
Change-Id: Ide44e60a47d9e87322115dc66017e8e0313b2162
Reviewed-on: https://chromium-review.googlesource.com/1169485
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582123}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -214,14 +214,8 @@ WRAPPED_INSTANTIATE_TEST_CASE_P(
                       TestCase("imageOpenGalleryOpenDrive"),
                       TestCase("imageOpenGalleryOpenDrive").EnableDriveFs()));
 
-// NaCl crashes in DEBUG in the zipFileOpen cases crbug.com/867738
-#if !defined(NDEBUG)
-#define MAYBE_ZipFiles DISABLED_ZipFiles
-#else
-#define MAYBE_ZipFiles ZipFiles
-#endif
 WRAPPED_INSTANTIATE_TEST_CASE_P(
-    MAYBE_ZipFiles, /* zip_files.js */
+    ZipFiles, /* zip_files.js */
     FilesAppBrowserTest,
     ::testing::Values(ZipCase("zipFileOpenDownloads").InGuestMode(),
                       ZipCase("zipFileOpenDownloads"),

chrome/browser/resources/chromeos/zip_archiver/cpp/request.h

@@ -9,11 +9,11 @@
 #include "ppapi/cpp/var_dictionary.h"
 
 // Defines the protocol messsage used to communicate between JS and NaCL.
-// This should be consistent with js/request.h.
+// This must be consistent with the JS side js/request.js.
 namespace request {
 
-// Defines requests keys. Every key should be unique and the same as the keys
-// on the JS side.
+// Defines request keys. The keys should be unique and must be the same as the
+// keys defined on the JS side.
 namespace key {
 
 // Mandatory keys for all unpacking requests.
@@ -60,9 +60,10 @@ const char kSrcFunc[] = "src_func";  // Should be a string.
 const char kMessage[] = "message";   // Should be a string.
 }  // namespace key
 
-// Defines request operations. These operations should be the same as the
-// operations on the JavaScript side.
+// Defines request operations. These operations must be the same as the
+// operations defined on the JS side (js/request.js).
 enum Operation {
+  // Unpack operations.
   READ_METADATA = 0,
   READ_METADATA_DONE = 1,
   READ_CHUNK = 2,
@@ -80,6 +81,8 @@ enum Operation {
   READ_FILE_DONE = 14,
   CONSOLE_LOG = 15,
   CONSOLE_DEBUG = 16,
+
+  // Pack operations.
   CREATE_ARCHIVE = 17,
   CREATE_ARCHIVE_DONE = 18,
   ADD_TO_ARCHIVE = 19,
@@ -93,6 +96,8 @@ enum Operation {
   CANCEL_ARCHIVE = 27,
   CANCEL_ARCHIVE_DONE = 28,
   RELEASE_COMPRESSOR = 29,
+
+  // Errors.
   FILE_SYSTEM_ERROR = -1,  // Errors specific to a file system.
   COMPRESSOR_ERROR = -2    // Errors specific to a compressor.
 };

chrome/browser/resources/chromeos/zip_archiver/js/decompressor.js

@@ -236,7 +236,9 @@ unpacker.Decompressor.prototype.processMessage = function(
     default:
       console.error('Invalid NaCl operation: ' + operation + '.');
       requestInProgress.onError('FAILED');
+      break;
   }
+
   delete this.requestsInProgress[requestId];
 };
 

chrome/browser/resources/chromeos/zip_archiver/js/request.js

@@ -6,13 +6,13 @@
 
 /**
  * Defines the protocol used to communicate between JS and NaCL.
- * This should be consistent with cpp/request.h.
+ * This must be consistent with NaCL C++ side cpp/request.h.
  * @namespace
  */
 unpacker.request = {
   /**
-   * Defines request ids. Every key should be unique and the same as the keys
-   * on the NaCL side.
+   * Defines request ids. The keys should be unique and must be the same as the
+   * keys defined on the NaCL C++ side.
    * @enum {string}
    */
   Key: {
@@ -59,14 +59,17 @@ unpacker.request = {
   },
 
   /**
-   * Defines request operations. These operation should be the same as the
-   * operations on the NaCL side. FILE_SYSTEM_ID and REQUEST_ID are mandatory
-   * for all unpack requests, while COMPRESSOR_ID is required for all pack
-   * requests. All the values of unpacking operations must be smaller than any
-   * packing operation (except errors).
+   * Defines request |operation|. The operation values must be the same as the
+   * operation values defined on the NaCL C++ side (cpp/request.h).
+   *
+   * Also, the unpack operation values must be smaller than the pack operation
+   * values (see the isPackRequest() helper below).
+   *
    * @enum {number}
    */
   Operation: {
+    // Unpack operations.
+    READ_METADATA: 0,
     READ_METADATA_DONE: 1,
     READ_CHUNK: 2,
     READ_CHUNK_DONE: 3,
@@ -83,6 +86,8 @@ unpacker.request = {
     READ_FILE_DONE: 14,
     CONSOLE_LOG: 15,
     CONSOLE_DEBUG: 16,
+
+    // Pack operations.
     CREATE_ARCHIVE: 17,
     CREATE_ARCHIVE_DONE: 18,
     ADD_TO_ARCHIVE: 19,
@@ -96,8 +101,10 @@ unpacker.request = {
     CANCEL_ARCHIVE: 27,
     CANCEL_ARCHIVE_DONE: 28,
     RELEASE_COMPRESSOR: 29,
-    FILE_SYSTEM_ERROR: -1,
-    COMPRESSOR_ERROR: -2
+
+    // Errors.
+    FILE_SYSTEM_ERROR: -1,  // Errors specific to a file system.
+    COMPRESSOR_ERROR: -2    // Errors specific to a compressor.
   },
 
   /**
@@ -126,6 +133,18 @@ unpacker.request = {
    * @return {!Object} A new request with mandatory fields.
    */
   createBasic_: function(operation, fileSystemId, requestId) {
+    // Protect from ill-defined or invalid |operation|, crbug.com/867842
+    if (Object.values(unpacker.request.Operation).indexOf(operation) === -1)
+      throwInvalidOperation(operation);
+    if (operation < unpacker.request.Operation.READ_METADATA)
+      throwInvalidOperation(operation);
+    if (operation > unpacker.request.Operation.CONSOLE_DEBUG)
+      throwInvalidOperation(operation);
+
+    function throwInvalidOperation(operation) {
+      throw new Error('invalid operation: [' + operation + ']');
+    }
+
     var basicRequest = {};
     basicRequest[unpacker.request.Key.OPERATION] = operation;
     basicRequest[unpacker.request.Key.FILE_SYSTEM_ID] = fileSystemId;