[webkit-reviews] review denied: [Bug 169945] Web Inspector: add context menu item to log content of WebSocket frame : [Attachment 305144] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 169945: Web Inspector: add context menu item to log content of WebSocket
frame
https://bugs.webkit.org/show_bug.cgi?id=169945

Attachment 305144: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 305144
  --> https://bugs.webkit.org/attachment.cgi?id=305144
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.esca
peCharacters("\"")}\"`, 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._dat
a.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(...)