Bug 195310 - Web Inspector: Protocol: add type checking when commands are called via invoke
Summary: Web Inspector: Protocol: add type checking when commands are called via invoke
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-04 20:39 PST by Devin Rousso
Modified: 2019-03-07 12:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2019-03-04 21:20 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (8.85 MB, application/zip)
2019-03-05 03:07 PST, EWS Watchlist
no flags Details
Patch (24.90 KB, patch)
2019-03-05 13:22 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.81 KB, patch)
2019-03-07 11:40 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 EWS Watchlist 2019-03-04 21:22:26 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-03-05 03:07:26 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-05 03:07:28 PST Comment hidden (obsolete)
Comment 6 BJ 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 EWS Watchlist 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 EWS Watchlist 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.