Bug 195310

Summary: Web Inspector: Protocol: add type checking when commands are called via invoke
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, ews, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch none

Description Devin Rousso 2019-03-04 20:39:55 PST
Right now we only do type checking for when it is called like a normal function.
Comment 1 Radar WebKit Bug Importer 2019-03-04 20:41:48 PST
<rdar://problem/48588679>
Comment 2 Devin Rousso 2019-03-04 21:20:24 PST
Created attachment 363596 [details]
Patch
Comment 3 Build Bot 2019-03-04 21:22:26 PST Comment hidden (obsolete)
Comment 4 Build Bot 2019-03-05 03:07:26 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-03-05 03:07:28 PST Comment hidden (obsolete)
Comment 6 Brian Burg 2019-03-05 09:18:59 PST
Comment on attachment 363596 [details]
Patch

Good change but.. it needs to be tested. Please update inspector-backend-invocation-return-value.html to exercise the invoke() code paths.
Comment 7 Devin Rousso 2019-03-05 13:22:02 PST
Created attachment 363678 [details]
Patch
Comment 8 Build Bot 2019-03-05 13:34:41 PST Comment hidden (obsolete)
Comment 9 Joseph Pecoraro 2019-03-07 11:28:58 PST
Comment on attachment 363678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363678&action=review

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:428
> +            if (typeof commandArguments !== "object") {
> +                if (!optional)
> +                    return deliverFailure(`Invalid number of arguments for command '${instance.qualifiedName}'.`);
> +                continue;
> +            }
> +
> +            if (!(name in commandArguments) && !optional)
> +                return deliverFailure(`Missing argument '${name}' for command '${instance.qualifiedName}'.`);

Could we just require the first argument is always an object to simplify this a bit:

    if (typeof commandArguments !== "object")
        return deliverFailure(`invoke expects an object for command arguments but it was a '${typeof commandArguments}'.`);

    let parameters = {};
    for (let {name, type, optional} of instance.callSignature) {
        if ((!name in commandArguments && !optional)
            return deliverFailure(`Missing argument '${name}' for command '${instance.qualifiedName}'.`);
        ...
    }

> LayoutTests/inspector/protocol/inspector-backend-invocation-return-value.html:122
> +            let result = RuntimeAgent.evaluate.invoke();

In other words I don't think we should allow calling invoke with no object (even if there are no required arguments).
Comment 10 Devin Rousso 2019-03-07 11:30:47 PST
Comment on attachment 363678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363678&action=review

>> LayoutTests/inspector/protocol/inspector-backend-invocation-return-value.html:122
>> +            let result = RuntimeAgent.evaluate.invoke();
> 
> In other words I don't think we should allow calling invoke with no object (even if there are no required arguments).

I'm fine with that.  I wasn't sure which approach we wanted to take, so I just matched the existing functionality.
Comment 11 Devin Rousso 2019-03-07 11:40:19 PST
Created attachment 363905 [details]
Patch
Comment 12 Build Bot 2019-03-07 11:42:14 PST Comment hidden (obsolete)
Comment 13 Joseph Pecoraro 2019-03-07 11:53:42 PST
Comment on attachment 363905 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2019-03-07 12:40:19 PST
Comment on attachment 363905 [details]
Patch

Clearing flags on attachment: 363905

Committed r242606: <https://trac.webkit.org/changeset/242606>
Comment 15 WebKit Commit Bot 2019-03-07 12:40:21 PST
All reviewed patches have been landed.  Closing bug.