New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 900298 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

~InspectorPostBodyParser should base64 encode before invoking the RequestPostData callback.

Project Member Reported by johannes@chromium.org, Oct 30

Issue description

Owner: jarhar@chromium.org
Status: Assigned (was: Untriaged)
Thanks for the report.  Our PDL does say it should be base64 encoded.  jarhar@, could you please take a look?
Yes, I see that there is a mismatch between the comment in browser_protocol.pdl's getRequestPostData and what it actually does. I could base64 encode and then decode again in the frontend where it is expected to not be base64 encoded, but I wonder if that would make the performance concerns worse (http://crbug.com/869840) or if there are any other applications out there that are expecting the getRequestPostData protocol message to not be base64 encoded.

Perhaps we could add an optional getRequestPostData boolean request parameter to enable base64 encoding if needed?

+dgozman@ what do you think about these concerns?
Cc: dgozman@chromium.org
If we can pass post data without encoding, we should do that. When the data is binary, we should optionally encode and pass "base64encoded" bit.
When there is a lot of data, how likely is it that you won't need the encoding?

Or perhaps: When you don't need the encoding, could it be that the data is usually small enough so that encoding it anyway won't make much of a difference?

===

I'm asking because not having a base64encoded bit is slightly simpler since in that case the 'binary' data type as implemented in the protocol can handle it (it always base64encodes / decodes).
Cc: johannes@chromium.org caseq@chromium.org
After talking with johannes@ and caseq@, it sounds like the getRequestPostData protocol method does not represent multipart post data very well since it just puts everything in a string with unknown encoding and that it would be better to respond with separate parts each with a mime type, which is what a multipart post actually does.

eostroukhov@ made a bug a while ago which is now assigned to me to do this very thing: http://crbug.com/810554

Upon further investigation, it turns out that the backend doesn't include files at all when responding to the getRequestPostData method: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/inspector/inspector_network_agent.cc?rcl=523cf50a9d77e4745b52e06d6c8c7ffa44fe3f69&l=249

For now I will just remove the comment about base64 in the protocol definition and say that it does not include any files.

If anyone believes there is a need for a new protocol method for multipart post data, I'd be happy to implement it.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f4def77830e423c48b55a20f87c93f28f16c141

commit 8f4def77830e423c48b55a20f87c93f28f16c141
Author: Joey Arhar <jarhar@chromium.org>
Date: Sat Nov 24 21:22:09 2018

[DevTools] Correct comment about base64 in getRequestPostData method

Bug:  900298 
Change-Id: I63f1d78010094e0bd1a9c05244a9e1c61cee3970
Reviewed-on: https://chromium-review.googlesource.com/c/1343512
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610711}
[modify] https://crrev.com/8f4def77830e423c48b55a20f87c93f28f16c141/third_party/blink/renderer/core/inspector/browser_protocol.pdl

Status: Fixed (was: Assigned)

Sign in to add a comment