Bug 83282 – Web Inspector: Allow inspection of Web Socket Frames

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Konrad Piascik]

Add Web Socket Frame inspection to the network item view.

[Konrad Piascik]

Created attachment 135826 [details]
Prototype

[Konrad Piascik]

Created attachment 135828 [details]
patch

[WebKit Review Bot]

Attachment 135828 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Modules/websockets/WebSocke..." exit_code: 1
Source/WebCore/Modules/websockets/WebSocketChannel.cpp:555:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/inspector/InspectorResourceAgent.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Konrad Piascik]

Created attachment 135832 [details]
updated patch

[Pavel Feldman]

Comment on attachment 135828 [details]
patch

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

Your general approach looks good. Note that you need to provide change logs with your changes per WebKit policy.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:556
> +    InspectorInstrumentation::didReceiveWebSocketFrame(m_document, m_identifier, frame);

I think you should report error frames as well.

> Source/WebCore/inspector/Inspector.json:685
> +                    { "name": "payloadLength", "type": "number", "description": "WebSocke frame payload length." },

Payload data length can be derived from the data in case you are sending the whole body. You should either sent truncated frames or skip this field.

> Source/WebCore/inspector/Inspector.json:891
> +                "name": "webSocketFrameResponseReceived",

webSocketFrameReceived? There are no requests / responses in WebSocket land. What about the webSocketFrameSent? You probably want to instrument frames going in both directions.

> Source/WebCore/inspector/Inspector.json:892
> +                "description": "Fired when WebSocket handshake response becomes available.",

Is this description accurate?

> Source/WebCore/inspector/Inspector.json:896
> +                    { "name": "response", "$ref": "WebSocketFrame", "description": "WebSocket response data." }

name: frame

> Source/WebCore/inspector/InspectorResourceAgent.cpp:460
> +    RefPtr<InspectorObject> responseObject = InspectorObject::create();

You should use the new type builder where possible:
RefPtr<TypeBuilder::Network::WebSocketFrame> frame = TypeBuilder::Network::WebSocketFrame::create()
    .setOpcode()
    .setMask()...

> Source/WebCore/inspector/front-end/NetworkItemView.js:62
> +        this.appendTab("frames", WebInspector.UIString("WebSocket Frames"), frameView);

appendTab("webSocketFrames", ...)

Please add new strings into English.lproj/localizedStrings.js in utf-16 encoding

> Source/WebCore/inspector/front-end/NetworkManager.js:454
> +            resource.payloads = {};

You should use array here:
resource.payloads = []. 
Also "payload" seems to be overloaded in the inspector world, so I would call it "frames" or "framePayloads"

> Source/WebCore/inspector/front-end/NetworkManager.js:455
> +            resource.payloads.size = 0;

size is always resource.frames.length

> Source/WebCore/inspector/front-end/NetworkManager.js:458
> +        resource.payloads[resource.payloads.size++] = response

resource.frames.push(response); This also is unbound growth, so you will drive the front-end out of memory eventually. Consider dropping frames.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:14
> + * You should have received a copy of the GNU Lesser General Public

You should provide BSD license with everything you contribute to the WebKit.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:33
> +        item.innerText = payload.time + '\t' + payload.opcode + ' ' + payload.mask + ' ' + payload.payloadLength + ' ' + payload.payloadData.substring(0, payload.payloadLength);

Please use String.sprintf defined in utilities.js

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:66
> +            content += payload.opcode + ' ' + payload.mask ? 'true' : 'false' + ' ' + payload.payloadLength + ' ' + payload.payloadData + '\t' + payload.time + '<br>';

Please provide screenshots with the patches that contain UI changes.

> Source/WebCore/inspector/front-end/inspector.html:147
> +    <script type="text/javascript" src="ResourceWebSocketFrameView.js"></script>

You should also include this new file into the following projects / lists:
WebKit.qrc,
WebCore.gypi,
WebCore.vcproj,
compile-front-end.py

[Pavel Feldman]

Comment on attachment 135832 [details]
updated patch

As per comments above.

[Gustavo Noronha (kov)]

Comment on attachment 135828 [details]
patch

Attachment 135828 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12341367

[Build Bot]

Comment on attachment 135828 [details]
patch

Attachment 135828 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12339341

[WebKit Review Bot]

Comment on attachment 135828 [details]
patch

Attachment 135828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12337326

[Build Bot]

Comment on attachment 135828 [details]
patch

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

[Gyuyoung Kim]

Comment on attachment 135828 [details]
patch

Attachment 135828 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12340362

[Konrad Piascik]

Created attachment 135864 [details]
Screenshot.

I'm open to suggestions as to how else this can be visualized.

[Konrad Piascik]

is there a template for testing web inspector front-end changes?

[Alexander Pavlov (apavlov)]

(In reply to comment #13)
> is there a template for testing web inspector front-end changes?

All web inspector tests reside in LayoutTests/inspector, LayoutTests/http/tests/inspector, and LayoutTests/http/tests/inspector-enabled. The latter two are for testing against a web server (which I guess is what you need), the former one is for the code that can deal with a local page. Find a test that is close to what you need and copy it! However, I'm not sure if our test web server supports web sockets...

[Nikita Vasilyev]

Preview and Response tabs always show "This request has no preview available" and "This request has no response data available" respectively. Perhaps we can remove them.

> (In reply to comment #12)
> Created an attachment (id=135864) [details]
> Screenshot.
> 
> I'm open to suggestions as to how else this can be visualized.

How does it distinguish sent/received data?

[Konrad Piascik]

(In reply to comment #15)
> Preview and Response tabs always show "This request has no preview available" and "This request has no response data available" respectively. Perhaps we can remove them.
> 
> > (In reply to comment #12)
> > Created an attachment (id=135864) [details] [details]
> > Screenshot.
> > 
> > I'm open to suggestions as to how else this can be visualized.
> 
> How does it distinguish sent/received data?

The above example, and code has only received data.  I've updated my patch locally to show sent data as well, but am still looking as to how to distinguish them.

I'll post another screenshot with an example soon...I'm also still working on tests.

[Konrad Piascik]

Created attachment 137144 [details]
Updated Screenshot

Bi-directional text frames tracked.

Currently I don't distinguish between what is a sent vs received frame.  However the sent frames all seem to be masked while the received do not.
Will post updated patch with tests once I figure out how to write them.

[Konrad Piascik]

Created attachment 137905 [details]
Patch

[Konrad Piascik]

Created attachment 137919 [details]
Patch

[Konrad Piascik]

Created attachment 137922 [details]
Patch

rebased patch so ews and style checker will apply/run correctly

[Pavel Feldman]

Comment on attachment 137919 [details]
Patch

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

Looks good overall. A bunch of style nits.

> Source/WebCore/ChangeLog:11
> +        * English.lproj/localizedStrings.js:

You should describe what has changed and why either via putting a paragraph of text above or inline methods summary below.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:558
> +    InspectorInstrumentation::didReceiveWebSocketFrame(m_document, m_identifier, frame);

I wonder if you should move this below or into the call site: there is a number of guards and I am not sure you want inspector to have entries for the invalid data entries without declaring them invalid.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:926
> +    InspectorInstrumentation::didSendWebSocketFrame(m_document, m_identifier, frame);

Same as above, you could fail below and inspector would show the frame as successfully sent.

> Source/WebCore/inspector/Inspector.json:901
> +                "name": "webSocketFrameErrorReceived",

webSocketFrameError ?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:993
> +        if (InspectorResourceAgent* resourceAgent = instrumentingAgents->inspectorResourceAgent())

Here and below in the file: wrong indent.

> Source/WebCore/inspector/front-end/NetworkManager.js:468
> +        if (!networkRequest.frames)

You should not create fields on the object of another class. You should instead introduce NetworkRequest.prototype.addFrame() and frames() methods and annotate them for the compiler.

> Source/WebCore/inspector/front-end/NetworkManager.js:474
> +        if (networkRequest.frames.length >= 100)

You could encapsulate this into addFrame as well.

> Source/WebCore/inspector/front-end/NetworkManager.js:475
> +            networkRequest.frames.shift();

This looks inefficient. You should shift array in chunks, otherwise it'll be called for each frame.

> Source/WebCore/inspector/front-end/NetworkManager.js:524
> +        var errorFrame = {};

encapsulating it into NetworkRequest and creating a small class for the error frames there is the preferred way.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:25
> +    WebInspector.View.call(this);

This looks dense. Could you group the lines below via inserting blank lines?

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:29
> +    var table = document.createElement('table');

Here and below: please use double quotations.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:33
> +    headerRow.appendChild(empty);

Note that DOMExtensions declares convenient Element.prototype.createChild for code like below.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:50
> +    for (var i = 0; i < this.resource.frames.length; i++) {

blank line above

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:55
> +        count.innerText = (i + 1);

No need for ()

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:61
> +        dateCell.innerHTML = String.sprintf("%s %s", (payload.sent ? "&rarr;" : (payload.errorMessage ? "" : "&larr;")), date.toISOString());

Assign to textContent, not innerHTML, otherwise, web socket would be able to inject javascript into the front-end page.

[Pavel Feldman]

Comment on attachment 137922 [details]
Patch

Clearing r? to remove this from the dashboard. I reviewed the previous version of the patch already.

[Build Bot]

Comment on attachment 137922 [details]
Patch

Attachment 137922 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12437264

[Konrad Piascik]

>> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:61
>> +        dateCell.innerHTML = String.sprintf("%s %s", (payload.sent ? "→" : (payload.errorMessage ? "" : "←")), date.toISOString());
> 
> Assign to textContent, not innerHTML, otherwise, web socket would be able to inject javascript into the front-end page.

This isn't really a valid concern since the string being created is not comprised of user input of any kind.  I'm simply putting a left arrow (←) or right arrow (→) to indicate whether it is a sent or received frame and then putting the date in an ISOString format.  The frame playload isn't being set as innerHTML.  It is set as innerText below.

[WebKit Review Bot]

Comment on attachment 137922 [details]
Patch

Attachment 137922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12345355

New failing tests:
http/tests/inspector/web-socket-frame.html

[WebKit Review Bot]

Created attachment 137943 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Konrad Piascik]

Created attachment 137969 [details]
Patch

[Build Bot]

Comment on attachment 137969 [details]
Patch

Attachment 137969 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12428717

[WebKit Review Bot]

Comment on attachment 137969 [details]
Patch

Attachment 137969 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12297444

New failing tests:
http/tests/inspector/web-socket-frame.html

[WebKit Review Bot]

Created attachment 137988 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 137969 [details]
Patch

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

Few nits and it is good to go.

> Source/WebCore/inspector/front-end/NetworkRequest.js:810
> +    /**

You could narrow the API surface down via introducing single
frames() method.

> Source/WebCore/inspector/front-end/NetworkRequest.js:830
> +    getFrame: function(position)

WebKit guidelines discourages use of get prefixes. frame: function(position). I would strongly encourage you to simply expose frames: array here even if it returns live collection.

> Source/WebCore/inspector/front-end/NetworkRequest.js:863
> +            this._frames.splice(0,10);

Space before 10.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:67
> +        dateCell.innerHTML = String.sprintf("%s %s", (payload.sent ? "→" : (payload.errorMessage ? "" : "←")), date.toISOString());

It does not really matter whether this is user data or not, you should only use innerHTML = for the (a) sanitized (b) HTML content. Otherwise it instantiates HTML parser, works slowly and results in vulnerabilities. You should look up \u symbol codes for the arrows and append them to your text content.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:73
> +            spanCell.innerText = payload.errorMessage;

innerText is not standard, uses weird semantics. You want .textContent = here.

[Konrad Piascik]

Created attachment 138080 [details]
Patch

[Build Bot]

Comment on attachment 138080 [details]
Patch

Attachment 138080 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12476169

[WebKit Review Bot]

Comment on attachment 138080 [details]
Patch

Attachment 138080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12470236

New failing tests:
http/tests/inspector/web-socket-frame.html

[WebKit Review Bot]

Created attachment 138094 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 138080 [details]
Patch

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

One nit and I think it is ready.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:932
> +    InspectorInstrumentation::didSendWebSocketFrame(m_document, m_identifier, frame);

The m_deflateFramer.deflate call above will mutate frame's data and, as a result, you will get garbled data instrumented. You could move this instrument call back to before deflate and assume that deflate is always successful. When we get a bug report claiming otherwise, we'll start instrumenting raw data*.

[Konrad Piascik]

Created attachment 138389 [details]
Patch

[Konrad Piascik]

Comment on attachment 138389 [details]
Patch

Needs to be rebased against ToT

[Konrad Piascik]

Created attachment 138396 [details]
Patch

[Build Bot]

Comment on attachment 138396 [details]
Patch

Attachment 138396 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12490016

[WebKit Review Bot]

Comment on attachment 138396 [details]
Patch

Attachment 138396 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12524231

New failing tests:
http/tests/inspector/web-socket-frame.html

[WebKit Review Bot]

Created attachment 138527 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Konrad Piascik]

Created attachment 138548 [details]
Patch

[Build Bot]

Comment on attachment 138548 [details]
Patch

Attachment 138548 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12522229

[Jessie Berlin]

Comment on attachment 138548 [details]
Patch

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

> Source/WebCore/inspector/InspectorAllInOne.cpp:26
> +// This all-in-one cpp file cuts down on template bloat to allow us to build our Windows release builds.

You shouldn't need to do this once you fix the actual causes of the build errors (see below).

> Source/WebCore/inspector/InspectorInstrumentation.h:74
> +class WebSocketFrame;

This is causing the build errors on Windows and I am not sure why it isn't causing build errors on other platforms.

WebSocketFrame is a struct, not a class, so the linker was trying to find a function that took a *class* WebSocketFrame when there was only one that took a *struct* WebSocketFrame.

Please forward declare it as a struct.

> Source/WebCore/inspector/InspectorResourceAgent.h:70
> +class WebSocketFrame;

This is causing the build errors on Windows. WebSocketFrame is a struct, not a class.

[Jessie Berlin]

Comment on attachment 138548 [details]
Patch

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

> Source/WebCore/inspector/InspectorAllInOne.cpp:26
> +// This all-in-one cpp file cuts down on template bloat to allow us to build our Windows release builds.

You shouldn't need to do this once you fix the actual causes of the build errors (see below).

> Source/WebCore/inspector/InspectorInstrumentation.h:74
> +class WebSocketFrame;

This is causing the build errors on Windows and I am not sure why it isn't causing build errors on other platforms.

WebSocketFrame is a struct, not a class, so the linker was trying to find a function that took a *class* WebSocketFrame when there was only one that took a *struct* WebSocketFrame.

Please forward declare it as a struct.

> Source/WebCore/inspector/InspectorResourceAgent.h:70
> +class WebSocketFrame;

This is causing the build errors on Windows. WebSocketFrame is a struct, not a class.

[Konrad Piascik]

Created attachment 139053 [details]
Patch

[WebKit Review Bot]

Comment on attachment 139053 [details]
Patch

Attachment 139053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12551091

New failing tests:
fast/images/gif-large-checkerboard.html

[WebKit Review Bot]

Created attachment 139130 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 139053 [details]
Patch

Clearing flags on attachment: 139053

Committed r115427: <http://trac.webkit.org/changeset/115427>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.