Bug 169945 – Web Inspector: add context menu item to log content of WebSocket frame

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Would be useful for JSON debugging.  Possibly "Log Frame" and "Log Frame as JSON Object" context menu items.

[Devin Rousso]

Created attachment 305144 [details]
Patch

[Devin Rousso]

Created attachment 305145 [details]
[Image] After Patch is applied

[Devin Rousso]

Comment on attachment 305144 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:62
> +        const options = {
> +            objectGroup: WebInspector.RuntimeManager.ConsoleObjectGroup,
> +            generatePreview: true,
> +        };

I'm not really sure how these work, but I think I understand what these two do.  Suggestions would be appreciated here as to whether any other keys (or the ones already here) are needed.

objectGroup
includeCommandLineAPI
doNotPauseOnExceptionsAndMuteConsole
returnByValue
generatePreview
saveResult
generatePreview
sourceURLAppender

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75
> +            try {
> +                if (JSON.parse(this._data.data)) {
> +                    contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => {
> +                        WebInspector.runtimeManager.evaluateInInspectedWindow(`JSON.parse(\`${this._data.data}\`);`, options, logResult);
> +                    });
> +                }
> +            } catch (error) { }

This is done so that the "Log Frame as JSON" item is only visible when the data in the frame is actually JSON parsable.

[Joseph Pecoraro]

Comment on attachment 305144 [details]
Patch

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

This is great, but a few suggestions above to really tighten this up.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:52
> +        let logResult = (result, wasThrown, savedResultIndex) => {

This should be handling wasThrown or guarantee it is not possible to throw. With my suggestions below, we should be able to `console.assert(!wasThrown)`.

>> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:62
>> +        };
> 
> I'm not really sure how these work, but I think I understand what these two do.  Suggestions would be appreciated here as to whether any other keys (or the ones already here) are needed.
> 
> objectGroup
> includeCommandLineAPI
> doNotPauseOnExceptionsAndMuteConsole
> returnByValue
> generatePreview
> saveResult
> generatePreview
> sourceURLAppender

• objectGroup - this is used to group RemoteObject values retained inside of the InjectedScript. The group is used to manage the lifetime of these values. If they don't get released they are essentially leaked
  - ConsoleObjectGroup is correct here, since you're logging to the Console its lifetime can match the console.
• includeCommandLineAPI - this tells InjectedScript whether or not to include the CommandLineAPI ($$, $x, dir(), values(), $_, $0, $1, ...) for use by the expression being evaluted
  - You do not want it here because you are not using any of the CommandLineAPI
• doNotPauseOnExceptionsAndMuteConsole
  - I don't really know for certain. You could pass either but since you don't expect an exception it doesn't matter
• returnByValue - this specifies whether you want a RemoteObject or a JSON value back
  - Console expects remote objects, so that probably makes now even though you know it will be JSON.
• generatePreview - this specifies whether the RemoteObject you get back previews its properties
  - You will want this here, we want it pretty much everywhere
• saveResult - this specifies whether or not the result will get a $n ($1, $2, ...) saved result index
  - You will want this here (I'm surprised your screenshots show $n but this code doesn't have this?!)
• sourceURLAppender - this is used by the frontend, if it wanted to inject a sourceURL into the expression (e.g. to hide it in inspector, or treat it specially)
  - You will not want this here. By default you will get the `appendWebInspectorSourceURL` appender which is what you want for any non-user-typed evaluation we perform in the page

So I'd expect:

  options = {
    objectGroup: WebInspector.RuntimeManager.ConsoleObjectGroup,
    generatePreview: true,
    saveResult: true,
  }

Maybe the doNotPauseOnExceptionsAndMuteConsole, because you are evaluating `JSON.parse(...)` and the Page may have replaced JSON.parse.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:67
> +            contextMenu.appendItem(WebInspector.UIString("Log Frame"), () => {
> +                WebInspector.runtimeManager.evaluateInInspectedWindow(`\"${this._data.data.escapeCharacters("\"")}\"`, options, logResult);
> +            });

Heh, this is more complex than it needs to be:

    1. You know its a String and you have the string. Trying to evaluate the string is error prone and unnecessary. Primitives are the same everywhere so just having the string is enough. Evaluating in the page and getting back the same string is not particularly useful.
    2. You want a $n value for this to be useful in the console. You can use Runtime.saveResult to get it without doing evaluateInInspectedWindow.
    3. escapeCharacters is hacky. JSON.stringify would get you what you want and be faster.

So, I'd suggest:

    let remoteObject = WebInspector.RemoteObject.fromPrimitiveValue(this._data.data);
    contextMenu.appendItem(WebInspector.UIString("Log Frame"), () => {
        WebInspector.runtimeManager.saveResult(remoteObject, (savedResultIndex) => {
            logResult(remoteObject, false, savedResultIndex);
        });
    });

This is more direct (you are getting the $n for the value), less traffic (sends the string one way instead of two ways), and simpler (you aren't trying to escape the string on your own).

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:74
> +                if (JSON.parse(this._data.data)) {
> +                    contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => {
> +                        WebInspector.runtimeManager.evaluateInInspectedWindow(`JSON.parse(\`${this._data.data}\`);`, options, logResult);
> +                    });
> +                }

Clever! I like this.

A couple problems:

    1. The page may have overridden JSON.parse and would throw an exception: `JSON.parse = () => { throw "hahaha"; }` and right now Web Inspector would get a "wasThrown = true" result which is not handled above.
    2. This is not escaping any backpacks in the data. So if the data was '{"message": "The ` character is crazy"}' you would get a "wasThrown = true" result which is not handled above:
        >>> JSON.parse(`{"message": "The ` character is crazy"}`)
        Unexpected identifier 'character'. Expected ')' to end an argument list.:1

I think you can avoid both of these problems with another clever trick:

    try {
        if (JSON.parse(this._data.data)) {
            contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => {
                let expression = "(" + this._data.data + ")";
                WebInspector.runtimeManager.evaluateInInspectedWindow(expression, options, logResult);
            });
        }
    } catch (error) { }

For the message above, we are essentially doing this:

    >>> ({"message": "The ` character is crazy"})
    [object Object]

This works because:

    1. We have already done JSON.parse in a trusted context (inside the frontend page) and verified that the result is a valid JSON value. So we know whatever the string is, its contents are some JSON literal and not unsafe to evaluate.
    2. By wrapping in ()s we force the JavaScript parser to treat this as an expression + value and not a program. This avoids the `{a:1}` being parsed as a program with a label "a" and statement "1".
      - Here we are using the language built-ins without opportunity for the page to override anything (like JSON.parse) to construct our object.

So we've avoided all of the problems above.

NOTE: In fact, evaluateInInspectedWindow would have done this for us without modifying the string due to Console Conveniences but we should be explicit here and do the ()s ourselves.

>> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75
>> +            } catch (error) { }
> 
> This is done so that the "Log Frame as JSON" item is only visible when the data in the frame is actually JSON parsable.

Some suggestions:

    • "Log as String", "Log as Object"
    • "Log Frame Text", "Log Value"
    • "Log Frame Text", "Log Number|String|Object..." based on the typeof JSON.parse(...)

[Devin Rousso]

Created attachment 305263 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 305263 [details]
Patch

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

r=me. I'm still iffy on the strings. I think we may want to discuss them. But the patch looks great otherwise.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127
> -            node = new WebInspector.WebSocketDataGridNode({data, time, isOutgoing});
> +            node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data, time}, attributes));
>          else
> -            node = new WebInspector.WebSocketDataGridNode({data, isOutgoing});
> +            node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data}, attributes));

I think we can use Object.assign instead of Object.shallowMerge. I think using builtins might be more natural here. What do you think?

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:66
> +                    const wasThrown = false;
> +                    logResult(remoteObject, wasThrown, savedResultIndex);

Style: I don't actually think the const variable here, since we are calling a local function so close by that its easy to check.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:72
> +                let json = JSON.parse(this._data.data);
> +                if (json) {

This doesn't allow for valid falsey values (undefined, 0, null, ""). Maybe we don't care about those, but it would be odd to allow `true` but not `false`.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75
> +                    if (Array.isArray(json))
> +                        title = WebInspector.UIString("Log Frame as Array");

Could handle type "boolean". The JSON site has a super simple at a glance diagram: <http://www.json.org>

How do all of these customizations feel? Is just `Log Value` enough or do these customizations make it feel nicer?

[Devin Rousso]

Comment on attachment 305263 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127
>> +            node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data}, attributes));
> 
> I think we can use Object.assign instead of Object.shallowMerge. I think using builtins might be more natural here. What do you think?

Personally, I prefer Object.shallowMerge for two reasons:
  1. It doesn't modify either of the objects being passed in.  I realized that this doesn't matter here, but I tend to try to avoid side effects of parameters.
  2. It will assert when both objects have the same key with different values.  Object.assign will just use the key from the last object.  Again, I realize that this probably won't happen here, but it might in the future and could present a debugging annoyance.

>> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75
>> +                        title = WebInspector.UIString("Log Frame as Array");
> 
> Could handle type "boolean". The JSON site has a super simple at a glance diagram: <http://www.json.org>
> 
> How do all of these customizations feel? Is just `Log Value` enough or do these customizations make it feel nicer?

I think just "Log Value" is enough.  It could get confusing otherwise.

[Devin Rousso]

Created attachment 305310 [details]
Patch

[Build Bot]

Comment on attachment 305310 [details]
Patch

Attachment 305310 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3405332

New failing tests:
http/tests/preload/single_download_preload.html

[Build Bot]

Created attachment 305319 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 305310 [details]
Patch

Clearing flags on attachment: 305310

Committed r214371: <http://trac.webkit.org/changeset/214371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.