[Merge m67][Extensions Bindings] Tweak messaging argument parsing

There was a bug where if the ID were passed as null or undefined (rather
than completely omitted), the message would fail to parse. Fix this, and
add a unittest.

Bug: 828664

TBR=rdevlin.cronin@chromium.org

(cherry picked from commit 7c723eb)

Change-Id: I92b0f071a1627322e12e8eaf0f5787ccd0793138
Reviewed-on: https://chromium-review.googlesource.com/1012601
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550718}
Reviewed-on: https://chromium-review.googlesource.com/1015412
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#50}
Cr-Branched-From: 9ef2aa8-refs/heads/master@{#550428}

Author: rdcronin

extensions/renderer/messaging_util.cc

@@ -270,18 +270,21 @@ void MassageSendMessageArguments(
       // Argument must be the message.
       message = arguments[0];
       break;
-    case 2:
-      // Assume the meaning is (id, message) if id would be a string, or if
-      // the options argument isn't expected.
-      // Otherwise the meaning is (message, options).
-      if (!allow_options_argument || arguments[0]->IsString()) {
+    case 2: {
+      // Assume the first argument is the ID if we don't expect options, or if
+      // the argument could match the ID parameter.
+      // ID could be either a string, or null/undefined (since it's optional).
+      bool could_match_id =
+          arguments[0]->IsString() || arguments[0]->IsNullOrUndefined();
+      if (!allow_options_argument || could_match_id) {
         target_id = arguments[0];
         message = arguments[1];
-      } else {
+      } else {  // Otherwise, the meaning is (message, options).
         message = arguments[0];
         options = arguments[1];
       }
       break;
+    }
     case 3:
       DCHECK(allow_options_argument);
       // The meaning in this case is unambiguous.

extensions/renderer/runtime_hooks_delegate_unittest.cc

@@ -254,6 +254,21 @@ TEST_F(RuntimeHooksDelegateTest, SendMessage) {
                          R"("string message")", other_target, false,
                          SendMessageTester::CLOSED);
 
+  // The sender could omit the ID by passing null or undefined explicitly.
+  // Regression tests for https://crbug.com/828664.
+  tester.TestSendMessage("null, {data: 'hello'}, function() {}",
+                         kStandardMessage, self_target, false,
+                         SendMessageTester::OPEN);
+  tester.TestSendMessage("null, 'test', function() {}",
+                         R"("test")", self_target, false,
+                         SendMessageTester::OPEN);
+  tester.TestSendMessage("null, 'test'",
+                         R"("test")", self_target, false,
+                         SendMessageTester::CLOSED);
+  tester.TestSendMessage("undefined, 'test', function() {}",
+                         R"("test")", self_target, false,
+                         SendMessageTester::OPEN);
+
   // Funny case. The only required argument is `message`, which can be any type.
   // This means that if an extension provides a <string, object> pair for the
   // first three arguments, it could apply to either the target id and the