1606612 - Pretty-print WebSocket packets when verbose logging is turned on

Status: RESOLVED FIXED

(Remote Protocol :: Agent, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

When remote.log.level is Log.Level.Info or above, verbose logging is enabled and we pretty-print JSON payloads to the HTTPD. We should do the same for the JSON payloads that are transmitted across the WebSocket connections.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606612: remote: move payload sanitization to Protocol

Moving the JSON payload sanitisation function to Protocol.jsm
means we can share it across modules.

The patch also adds new tests.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606612: remote: pretty-print WebSocket JSON payloads when verbose logging

When remote.log.level is Log.Level.Info or above, verbose logging
is enabled and we pretty-print JSON payloads in requests to the
HTTPD in JSONHandler.

This patch matches the behaviour with JSON payloads being transmitted
across WebSocket connections.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4368466b495b
remote: move payload sanitization to Protocol r=remote-protocol-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/d0ff02a17ad9
remote: pretty-print WebSocket JSON payloads when verbose logging r=remote-protocol-reviewers,whimboo

Comment 4

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1606612) for 'remote' failures at browser_scriptToEvaluateOnNewDocument.js.
https://hg.mozilla.org/integration/autoland/rev/7835846bfe1b91a1d219ac63cd766d21ceb23fab

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=d0ff02a17ad9ebb586130f0399ffc18fd633ec84&selectedJob=283216018

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283216018&repo=autoland&lineNumber=3930

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606612: remote: preserve empty strings in JSON payloads

Certain CDP domains, such as IO.read, rely on returning empty strings.
This removes the filtering of empty strings from JSON payloads so
that return types from the protocol are consistent.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Filtering out empty strings in JSON payloads was not a great idea. Removing that should fix the failing tests.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606612: remote: stop sanitizing JSON payloads from the HTTPD

The CDP protocol expects consistent types to be returned. By filtering
out null values and strings of zero length we break this promise.

Comment 8

[Andreas Tolfsen ❲:ato❳]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #6)

Filtering out empty strings in JSON payloads was not a great idea. Removing that should fix the failing tests.

I’ve reworked the changeset so that we drop this behaviour from JSONHandler also for consistency.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d00e6b043f0a
remote: stop sanitizing JSON payloads from the HTTPD r=remote-protocol-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/6b0f8701cfc5
remote: pretty-print WebSocket JSON payloads when verbose logging r=remote-protocol-reviewers,whimboo

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d00e6b043f0a
https://hg.mozilla.org/mozilla-central/rev/6b0f8701cfc5