New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment
link

Issue 657115: Use a consistent approach for encoding binary data in NetLogs

Reported by eroman@chromium.org, Oct 18 2016 Project Member

Issue description

NetLog parameters (base::Value) do not support the use of base::BinaryValue, because JSON serialization cannot handle these.

This has resulted in a variety of workarounds:

 * Some code incorrectly use base::StringValue (this is expected to be UTF8 string and results in a DCHECK failure)
 * Some code hex encodes and creates a base::StringValue
 * Some code base64 encodes and creates a base::StringValue
 * Some code changes the name of the key to reflect the type, as in "hex_encoded_bytes"

It would be good to have a single recommended way of accomplishing this.
 

Comment 1 by eroman@chromium.org, Oct 18 2016

Some possibilities:

A: Use base::BinaryValue in the C++ side, and hack up the serialization code so it results in something consistent when going to JSON

B: Rely on helper methods to create the equivalent of "base::BinaryValue" for use with NetLog. For instance it could construct something like: {"b64": Base64EncodedString}


Generally speaking I don't think carrying the "type" of the value in the name is a good idea, since this doesn't work with things like arrays, and it also is something else to remember when adding fields to dictionaries.


The Javascript can be taught to recognize our serialization of binary values, and display it accordingly (preferring text if it is all ASCII).

Another option for serialization of binary values is to special case all-ASCII values and just encode them as a string. But if the input contains non-ASCII then represent it as {"b64": XXXX} or some such thing.

Comment 2 by eroman@chromium.org, Dec 6

Cc: b...@chromium.org
Owner: eroman@chromium.org
Status: Assigned (was: Untriaged)

Comment 4 by eroman@chromium.org, Dec 7

 Issue 657116  has been merged into this issue.

Comment 6 by bugdroid1@chromium.org, Dec 10

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2468c53ca1e48b9230edc5c4774254d9e964de4d

commit 2468c53ca1e48b9230edc5c4774254d9e964de4d
Author: Eric Roman <eroman@chromium.org>
Date: Mon Dec 10 19:59:59 2018

Escape non-ASCII strings sent to NetLog from ElideSpdyHeaderBlockForNetLog().

Bug:  800282 ,  657115 
Change-Id: I61ae689ad688aa5282ea0df2850aaf94894787e8
Reviewed-on: https://chromium-review.googlesource.com/c/1368646
Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615208}
[modify] https://crrev.com/2468c53ca1e48b9230edc5c4774254d9e964de4d/net/spdy/spdy_log_util.cc
[modify] https://crrev.com/2468c53ca1e48b9230edc5c4774254d9e964de4d/net/spdy/spdy_log_util_unittest.cc

Comment 7 by bugdroid1@chromium.org, Dec 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b74dd4c029c3244b0c45b954c62251b13bcd8b4

commit 2b74dd4c029c3244b0c45b954c62251b13bcd8b4
Author: Eric Roman <eroman@chromium.org>
Date: Tue Dec 11 02:35:39 2018

Use NetLogStringValue() in place of EscapeExternalHandlerValue().

Bug:  657115 
Change-Id: I70526b30ea51fe77343d1e8783cc01cc84a4c68e
Reviewed-on: https://chromium-review.googlesource.com/c/1369005
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615392}
[modify] https://crrev.com/2b74dd4c029c3244b0c45b954c62251b13bcd8b4/net/spdy/header_coalescer.cc
[modify] https://crrev.com/2b74dd4c029c3244b0c45b954c62251b13bcd8b4/net/spdy/header_coalescer_test.cc

Comment 8 by eroman@chromium.org, Dec 11

Status: Fixed (was: Started)

Comment 9 by bugdroid1@chromium.org, Dec 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e

commit 0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e
Author: Eric Roman <eroman@chromium.org>
Date: Sat Dec 15 04:46:55 2018

Use a wrapper for creating binary NetLog values.

This renames the "hex_encoded_bytes" parameter to "bytes" (Base64 encoded).

The motivation is to move towards a consistent approach for how binary data is encoded.

Bug:  657115 
Change-Id: I4762ee774682a0376979d779ac1faa177d2b8701
Reviewed-on: https://chromium-review.googlesource.com/c/1378845
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616954}
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/log/net_log.cc
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/log/net_log.h
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/log/net_log_event_type_list.h
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/log/net_log_unittest.cc
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/log/net_log_with_source.cc
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/socket/udp_net_log_parameters.cc
[modify] https://crrev.com/0d5fcb56ab8ef97172f6f00ad19b2fc9e17f536e/net/tools/print_certificates.py

Sign in to add a comment