Bug 169649

Summary:

Web Inspector: WebSockets: Don't store binary frames in memory since they are never shown

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, inspector-bugzilla-changes, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	buildbot: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2	none
Patch	bburg: review+, bburg: commit-queue-
Patch	none

Nikita Vasilyev

Reported 2017-03-14 18:03:43 PDT

Web Socket binary frames are never displayed in the UI. There's no reason to clog Web Inspector's front-end memory with them.

Attachments
Patch (2.09 KB, patch) 2017-03-14 18:05 PDT, Nikita Vasilyev	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (742.15 KB, application/zip) 2017-03-14 19:35 PDT, Build Bot	no flags	Details
Patch (2.08 KB, patch) 2017-03-14 20:35 PDT, Nikita Vasilyev	bburg: review+ bburg: commit-queue-	Details Formatted Diff Diff
Patch (1.73 KB, patch) 2017-04-03 15:32 PDT, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-03-14 18:04:21 PDT

<rdar://problem/31053069>

Nikita Vasilyev

Comment 2 2017-03-14 18:05:15 PDT

Created attachment 304454 [details] Patch

Joseph Pecoraro

Comment 3 2017-03-14 18:19:59 PDT

Comment on attachment 304454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304454&action=review > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78 > + // Binary data is never shown in the UI, don't clog memory with it. > + if (opcode === WebInspector.WebSocketResource.OpCodes.BinaryFrame) > + return ""; Can we show the binary data? A hex view would be neat. Instead of the empty string maybe we can show a stylized localized string, such as "(Binary Data)" or at least an emDash? > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:79 > + else Style: We normally do not include an else when the previous branch always returns.

Nikita Vasilyev

Comment 4 2017-03-14 18:36:58 PDT

Comment on attachment 304454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304454&action=review >> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78 >> + return ""; > > Can we show the binary data? A hex view would be neat. Instead of the empty string maybe we can show a stylized localized string, such as "(Binary Data)" or at least an emDash? We can show binary data, but there could be several megabytes of it. If we make a UI for showing binary data, I'd make it available for Resources as well, not just WebSocketContentView. I'm curious to explore this but I think it's a low priority now. We currently show "Binary Frame", not an empty string: https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js#L95-L98

Build Bot

Comment 5 2017-03-14 19:35:12 PDT

Comment on attachment 304454 [details] Patch Attachment 304454 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3326029 New failing tests: imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html

Build Bot

Comment 6 2017-03-14 19:35:14 PDT

Created attachment 304463 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Nikita Vasilyev

Comment 7 2017-03-14 20:35:53 PDT

Created attachment 304466 [details] Patch The test fail seems unrelated to my patch.

Blaze Burg

Comment 8 2017-04-01 10:18:10 PDT

Comment on attachment 304466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304466&action=review r=me > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:74 > + _dataForWebSocketFrame(data, opcode) I don't see any reason to make this a private member. Let's just inline this into addFrame. > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78 > + return ""; This should be null, not an empty string.

Nikita Vasilyev

Comment 9 2017-04-03 15:32:05 PDT

Created attachment 306130 [details] Patch

WebKit Commit Bot

Comment 10 2017-04-03 16:11:28 PDT

Comment on attachment 306130 [details] Patch Clearing flags on attachment: 306130 Committed r214853: <http://trac.webkit.org/changeset/214853>

WebKit Commit Bot

Comment 11 2017-04-03 16:11:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.