Web Inspector: backend command promises are not rejected when a proto…

…col error occurs

https://bugs.webkit.org/show_bug.cgi?id=141403

Reviewed by Joseph Pecoraro.

Source/WebInspectorUI:

Fix a few corner cases for how InspectorBackend delivers command failures.

* UserInterface/Protocol/InspectorBackend.js:
(InspectorBackend.Command.prototype.deliverFailure): Added.
(InspectorBackend.Command.prototype._invokeWithArguments):

    If argument-checking fails, return a rejected promise or invoke the supplied callback
    on a zero-delay setTimeout to ensure that the reply is asynchronous.

LayoutTests:

Expand coverage of an existing protocol layer test to cover success and failure modes.

* inspector/protocol/inspector-backend-invocation-return-value-expected.txt:
* inspector/protocol/inspector-backend-invocation-return-value.html:

Canonical link: https://commits.webkit.org/167244@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189761 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,15 @@
+2015-09-14  Brian Burg  <bburg@apple.com>
+
+        Web Inspector: backend command promises are not rejected when a protocol error occurs
+        https://bugs.webkit.org/show_bug.cgi?id=141403
+
+        Reviewed by Joseph Pecoraro.
+
+        Expand coverage of an existing protocol layer test to cover success and failure modes.
+
+        * inspector/protocol/inspector-backend-invocation-return-value-expected.txt:
+        * inspector/protocol/inspector-backend-invocation-return-value.html:
+
 2015-09-14  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Test gardening after r189670

LayoutTests/inspector/protocol/inspector-backend-invocation-return-value-expected.txt

@@ -1,5 +1,41 @@
 Testing the inspector backend's return values when invoking a protocol command in various ways.
 
-PASS: RuntimeAgent.evaluate should not return a promise when invoked with a function as the last parameter.
-PASS: RuntimeAgent.evaluate should return a Promise when invoked without a callback.
+
+== Running test suite: Protocol.BackendInvocationReturnValues
+-- Running test case: ResolveCommandPromiseOnSuccess
+PASS: A backend command should return a Promise when invoked without a callback.
+PASS: A successful command invocation's promise should be resolved.
+
+-- Running test case: RejectCommandPromiseWithInvalidArguments
+ERROR: Protocol Error: Invalid type of argument 'expression' for command 'Runtime.evaluate' call. It must be 'string' but it is 'number'.
+PASS: A backend command should return a Promise when invoked without a callback.
+PASS: An invalid command invocation's promise should be rejected.
+
+-- Running test case: RejectCommandPromiseWithMissingArguments
+ERROR: Protocol Error: Invalid number of arguments for command 'Runtime.evaluate'.
+PASS: A backend command should return a Promise when invoked without a callback.
+PASS: An invalid command invocation's promise should be rejected.
+
+-- Running test case: ReturnNothingIfCallback
+PASS: A backend command should not have a return value when invoked with a callback.
+
+-- Running test case: InvokeCallbackWithResultOnSuccess
+PASS: A backend command should not return anything when invoked with a callback.
+PASS: A backend command should always invoke its callback asynchronously.
+PASS: A successful command should invoke the callback with a 'null' first parameter.
+PASS: A successful command should invoke the callback with one or more result parameters.
+
+-- Running test case: InvokeCallbackWithErrorForInvalidArguments
+ERROR: Protocol Error: Invalid type of argument 'expression' for command 'Runtime.evaluate' call. It must be 'string' but it is 'number'.
+PASS: A backend command should not return anything when invoked with a callback.
+PASS: A backend command should always invoke its callback asynchronously.
+PASS: A failed command should invoke the callback with a string error message as its first parameter.
+PASS: A failed command should invoke the callback with only an error parameter.
+
+-- Running test case: InvokeCallbackWithErrorForMissingArguments
+ERROR: Protocol Error: Invalid number of arguments for command 'Runtime.evaluate'.
+PASS: A backend command should not return anything when invoked with a callback.
+PASS: A backend command should always invoke its callback asynchronously.
+PASS: A failed command should invoke the callback with a string error message as its first parameter.
+PASS: A failed command should invoke the callback with only an error parameter.
 

LayoutTests/inspector/protocol/inspector-backend-invocation-return-value.html

@@ -5,14 +5,140 @@
 <script>
 function test()
 {
-    let dummyCallback = function(){};
-    let invokeCallbackResult = RuntimeAgent.evaluate("41", dummyCallback);
-    InspectorTest.expectThat(!(invokeCallbackResult instanceof Promise), "RuntimeAgent.evaluate should not return a promise when invoked with a function as the last parameter.");
 
-    let invokePromiseResult = RuntimeAgent.evaluate("42");
-    InspectorTest.expectThat(invokePromiseResult instanceof Promise, "RuntimeAgent.evaluate should return a Promise when invoked without a callback.");
+    let suite = InspectorTest.createAsyncSuite("Protocol.BackendInvocationReturnValues");
+    let dummyCallback = () => {};
 
-    InspectorTest.completeTest();
+    // Test behavior for promise-based callbacks.
+
+    suite.addTestCase({
+        name: "ResolveCommandPromiseOnSuccess",
+        description: "Backend command's returned promise should be resolved if the command is successful.",
+        test: (resolve, reject) => {
+            let returnValue = RuntimeAgent.evaluate("42");
+
+            InspectorTest.expectThat(returnValue instanceof Promise, "A backend command should return a Promise when invoked without a callback.");
+            // If a promise wasn't returned, we can't test the rest so just die.
+            if (!(returnValue instanceof Promise))
+                reject();
+
+            returnValue.then(function resolved(result) {
+                InspectorTest.log("PASS: A successful command invocation's promise should be resolved.");
+                resolve();
+            }, function rejected(result) {
+                InspectorTest.log("FAIL: A successful command invocation's promise should be resolved.");
+                reject();
+            });
+        }
+    });
+
+    suite.addTestCase({
+        name: "RejectCommandPromiseWithInvalidArguments",
+        description: "Backend command's returned promise should be rejected if the command has invalid arguments.",
+        test: (resolve, reject) => {
+            let result = RuntimeAgent.evaluate(42);
+
+            InspectorTest.expectThat(result instanceof Promise, "A backend command should return a Promise when invoked without a callback.");
+            // If a promise wasn't returned, we can't test the rest so just die.
+            if (!(result instanceof Promise))
+                reject();
+
+            result.then(function resolved(result) {
+                InspectorTest.log("FAIL: An invalid command invocation's promise should be rejected.");
+                reject();
+            }, function rejected(result) {
+                InspectorTest.log("PASS: An invalid command invocation's promise should be rejected.");
+                resolve();
+            });
+        }
+    });
+
+    suite.addTestCase({
+        name: "RejectCommandPromiseWithMissingArguments",
+        description: "Backend command's returned promise should be rejected if the command lacks required arguments.",
+        test: (resolve, reject) => {
+            let result = RuntimeAgent.evaluate();
+
+            InspectorTest.expectThat(result instanceof Promise, "A backend command should return a Promise when invoked without a callback.");
+            // If a promise wasn't returned, we can't test the rest so just die.
+            if (!(result instanceof Promise))
+                reject();
+
+            result.then(function resolved(result) {
+                InspectorTest.log("FAIL: An invalid command invocation's promise should be rejected.");
+                reject();
+            }, function rejected(result) {
+                InspectorTest.log("PASS: An invalid command invocation's promise should be rejected.");
+                resolve();
+            });
+        }
+    });
+
+    // Test behavior for function-based callbacks.
+
+    suite.addTestCase({
+        name: "ReturnNothingIfCallback",
+        description: "Backend commands should not return anything if a callback is supplied.",
+        test: (resolve, reject) => {
+            let returnValue = RuntimeAgent.evaluate("41", dummyCallback);
+            InspectorTest.expectThat(returnValue === undefined, "A backend command should not have a return value when invoked with a callback.");
+            resolve();
+        }
+    });
+
+    suite.addTestCase({
+        name: "InvokeCallbackWithResultOnSuccess",
+        description: "Backend command callback should be invoked with a result if the command is successful.",
+        test: (resolve, reject) => {
+            let initialState = 1;
+            let returnValue = RuntimeAgent.evaluate("42", function(error, result) {
+                InspectorTest.expectThat(error === null, "A successful command should invoke the callback with a 'null' first parameter.");
+                InspectorTest.expectThat(arguments.length > 1, "A successful command should invoke the callback with one or more result parameters.");
+                initialState++;
+                resolve();
+            });
+
+            InspectorTest.expectThat(returnValue === undefined, "A backend command should not return anything when invoked with a callback.");
+            InspectorTest.expectThat(initialState === 1, "A backend command should always invoke its callback asynchronously.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "InvokeCallbackWithErrorForInvalidArguments",
+        description: "Backend command callback should be invoked with an error if the command has invalid arguments.",
+        test: (resolve, reject) => {
+            let initialState = 1;
+            let returnValue = RuntimeAgent.evaluate(42, function callback(error) {
+                InspectorTest.expectThat(typeof error === "string", "A failed command should invoke the callback with a string error message as its first parameter.");
+                InspectorTest.expectThat(arguments.length === 1, "A failed command should invoke the callback with only an error parameter.");
+                initialState++;
+                resolve();
+            });
+
+            InspectorTest.expectThat(returnValue === undefined, "A backend command should not return anything when invoked with a callback.");
+            InspectorTest.expectThat(initialState === 1, "A backend command should always invoke its callback asynchronously.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "InvokeCallbackWithErrorForMissingArguments",
+        description: "Backend command callback should be invoked with an error if the command lacks required arguments.",
+        test: (resolve, reject) => {
+            let initialState = 1;
+            let returnValue = RuntimeAgent.evaluate(function callback(error) {
+                InspectorTest.expectThat(typeof error === "string", "A failed command should invoke the callback with a string error message as its first parameter.");
+                InspectorTest.expectThat(arguments.length === 1, "A failed command should invoke the callback with only an error parameter.");
+
+                initialState++;
+                resolve();
+            });
+
+            InspectorTest.expectThat(returnValue === undefined, "A backend command should not return anything when invoked with a callback.");
+            InspectorTest.expectThat(initialState === 1, "A backend command should always invoke its callback asynchronously.");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
 }
 </script>
 </head>

Source/WebInspectorUI/ChangeLog

@@ -1,3 +1,19 @@
+2015-09-14  Brian Burg  <bburg@apple.com>
+
+        Web Inspector: backend command promises are not rejected when a protocol error occurs
+        https://bugs.webkit.org/show_bug.cgi?id=141403
+
+        Reviewed by Joseph Pecoraro.
+
+        Fix a few corner cases for how InspectorBackend delivers command failures.
+
+        * UserInterface/Protocol/InspectorBackend.js:
+        (InspectorBackend.Command.prototype.deliverFailure): Added.
+        (InspectorBackend.Command.prototype._invokeWithArguments):
+
+            If argument-checking fails, return a rejected promise or invoke the supplied callback
+            on a zero-delay setTimeout to ensure that the reply is asynchronous.
+
 2015-09-14  Brian Burg  <bburg@apple.com>
 
         Web Inspector: middle-clicking a tab in the tab bar should close it

Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js

@@ -475,35 +475,35 @@ InspectorBackend.Command.prototype = {
         let commandArguments = Array.from(arguments);
         let callback = typeof commandArguments.lastValue === "function" ? commandArguments.pop() : null;
 
+        function deliverFailure(message) {
+            console.error(`Protocol Error: ${message}`);
+            if (callback)
+                setTimeout(callback.bind(null, message), 0);
+            else
+                return Promise.reject(new Error(message));
+        }
+
         let parameters = {};
         for (let parameter of instance.callSignature) {
             let parameterName = parameter["name"];
             let typeName = parameter["type"];
             let optionalFlag = parameter["optional"];
 
-            if (!commandArguments.length && !optionalFlag) {
-                console.error("Protocol Error: Invalid number of arguments for method '" + instance.qualifiedName + "' call. It must have the following arguments '" + JSON.stringify(instance.callSignature) + "'.");
-                return;
-            }
+            if (!commandArguments.length && !optionalFlag)
+                return deliverFailure(`Invalid number of arguments for command '${instance.qualifiedName}'.`);
 
             let value = commandArguments.shift();
             if (optionalFlag && value === undefined)
                 continue;
 
-            if (typeof value !== typeName) {
-                console.error("Protocol Error: Invalid type of argument '" + parameterName + "' for method '" + instance.qualifiedName + "' call. It must be '" + typeName + "' but it is '" + typeof value + "'.");
-                return;
-            }
+            if (typeof value !== typeName)
+                return deliverFailure(`Invalid type of argument '${parameterName}' for command '${instance.qualifiedName}' call. It must be '${typeName}' but it is '${typeof value}'.`);
 
             parameters[parameterName] = value;
         }
 
-        if (commandArguments.length === 1 && !callback) {
-            if (commandArguments[0] !== undefined) {
-                console.error("Protocol Error: Optional callback argument for method '" + instance.qualifiedName + "' call must be a function but its type is '" + typeof args[0] + "'.");
-                return;
-            }
-        }
+        if (!callback && commandArguments.length === 1 && commandArguments[0] !== undefined)
+            return deliverFailure(`Protocol Error: Optional callback argument for command '${instance.qualifiedName}' call must be a function but its type is '${typeof args[0]}'.`);
 
         if (callback)
             instance._backend._sendCommandToBackendWithCallback(instance, parameters, callback);