1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly

Status: NEW

(DevTools :: Netmonitor, defect, P3)

Description

[Aquila]

Attachment: RequestPayload.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

I'm testing an application sending and receiving binary data from the server. I need to test if the sent binary data is correct and I wanted to copy the sent binary data from the network panel. I tried to use the context menu on the request "Copy POST Data" and to select the text in the "Request payload" and paste it in a text processor like notepad++.


Actual results:

The binary data is cut, the paste function returns only the text until the char code \u0
I don't know if it is a problem of firefox or of the copy paste function.


Expected results:

Be able to export the whole binary data in the request payload (posted data) as well as in the response.

Comment 1

[Locke Chen]

Hi Honza
I'd like work on this issue. Could you please assign to me?
Thank you.:)

Comment 2

[Locke Chen]

Attachment: Copy POST data.png

Hi Honza
Looks this issue is not occur on netmonitor.
I tried to duplicate it however it hasn't happen. but I found another issue,I think this issue is occurred in copyString method.

Please find the file attached.
This method copied the incomplete data.
In [3], here is the output of the postData.
So far the data is correct, but the copyString is not. see [4].

Comment 3

[Locke Chen]

https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#255

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #2)
> Created attachment 8859038 [details]
> Copy POST data.png
> 
> Hi Honza
> Looks this issue is not occur on netmonitor.
> I tried to duplicate it however it hasn't happen. but I found another
> issue,I think this issue is occurred in copyString method.
> Please find the file attached.
> This method copied the incomplete data.
> In [3], here is the output of the postData.
> So far the data is correct, but the copyString is not. see [4].
Yes, sounds like it could be the problem. Perhaps we need to encode the data first (making sure it is *not* binary) before passing into copyString() ? It should happen only for binary responses I guess.
Is your test page online so, I can also try it?
Honza

Comment 5

[Locke Chen]

(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> (In reply to Locke Chen from comment #2)
> > Created attachment 8859038 [details]
> > Copy POST data.png
> > 
> > Hi Honza
> > Looks this issue is not occur on netmonitor.
> > I tried to duplicate it however it hasn't happen. but I found another
> > issue,I think this issue is occurred in copyString method.
> > Please find the file attached.
> > This method copied the incomplete data.
> > In [3], here is the output of the postData.
> > So far the data is correct, but the copyString is not. see [4].
> Yes, sounds like it could be the problem. Perhaps we need to encode the data
> first (making sure it is *not* binary) before passing into copyString() ? It
> should happen only for binary responses I guess.
> Is your test page online so, I can also try it?
> Honza

Hi Honza
Agree, I'll try to encode the data first.
You can upload some file to MEGA.nz and open the netmonitor.
https://mega.nz/
Thank you. :)

Comment 6

[Locke Chen]

Attachment: test result.txt

I tested encode the binary data to JSON format. 
Looks data is correct. see [1](in file attached - test result.txt).
And I also copied HAR's post data. see [2].

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #6)
> Created attachment 8859428 [details]
> test result.txt
> 
> I tested encode the binary data to JSON format. 
> Looks data is correct. see [1](in file attached - test result.txt).
> And I also copied HAR's post data. see [2].
Yep, looks promising! Any patch(es) available for review?

Honza

Comment 8

[Locke Chen]

I'm thinking about how to verify that postdata is a binary file.
Looks this issue happen only from binary data.
Copied from "copy POST data" method also having quote character if I just using JSON.stringify method.
it will look likes "íí\u0019\u0014ÆUߨòêGm\u001d÷ÌÌXIlu0áÅõ\n\u0016-:!k½dûú¸H\nþSß\u0006üñÓ$G\u0006 ..".

Could you please give some suggestions to me?
Thanks!

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #8)
> Could you please give some suggestions to me?
Good question. We might have to iterate over the data and look for non-text characters. In any case, implementation of any helper function should happen in bug 752288. Also, you can ask Mike Ratcliffe, I recall he had some ideas while back.

Honza

Comment 10

[Locke Chen]

Thank you Honza.
Maybe I can divide it into two cases (or more).
Plain text or binary, but I have to try.

Comment 11

[Locke Chen]

We seem to only need to detect 0x00 symbols.
Binary has many 0x00 symbols, but plain text files are not.
Looks that copyString issue reason is 0x00 symbol, so there not copy.
Could I add method to verify binary or text?

I will be fix at here:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#249-254

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #11)
> We seem to only need to detect 0x00 symbols.
Yes this sounds like a good simple check.

Can we do more than just 0x00?
Perhaps we could search for some other non-printable chars?

See couple of examples in our code base:

http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/devtools/client/debugger/new/integration-tests.js#1735

function nonprintable(code) { return between(code, 0,8) || code == 0xb || between(code, 0xe,0x1f) || code == 0x7f; }

http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/devtools/client/shared/components/reps/reps.js#409

function sanitizeString(text) {
  // Replace all non-printable characters, except of
  // (horizontal) tab (HT: \x09) and newline (LF: \x0A, CR: \x0D),
  // with unicode replacement character (u+fffd).
  // eslint-disable-next-line no-control-regex
  let re = new RegExp("[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]", "g");
  return text.replace(re, "\ufffd");
}

Honza

Comment 13

[Locke Chen]

(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> (In reply to Locke Chen from comment #11)
> > We seem to only need to detect 0x00 symbols.
> Yes this sounds like a good simple check.
> 
> Can we do more than just 0x00?
> Perhaps we could search for some other non-printable chars?
> 
> See couple of examples in our code base:
> 
> http://searchfox.org/mozilla-central/rev/
> ae8c2e2354db652950fe0ec16983360c21857f2a/devtools/client/debugger/new/
> integration-tests.js#1735
> 
> function nonprintable(code) { return between(code, 0,8) || code == 0xb ||
> between(code, 0xe,0x1f) || code == 0x7f; }
> 
> http://searchfox.org/mozilla-central/rev/
> ae8c2e2354db652950fe0ec16983360c21857f2a/devtools/client/shared/components/
> reps/reps.js#409
> 
> function sanitizeString(text) {
>   // Replace all non-printable characters, except of
>   // (horizontal) tab (HT: \x09) and newline (LF: \x0A, CR: \x0D),
>   // with unicode replacement character (u+fffd).
>   // eslint-disable-next-line no-control-regex
>   let re = new RegExp("[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]", "g");
>   return text.replace(re, "\ufffd");
> }
> 
> Honza

Oh, that's great. thank you for your hint.
I'll try it first.

Comment 14

[Locke Chen]

Looks I can't reuse the nonprintable method.
Should I add these method to utils? (like the nonprintable,between .. )
I also need to use Blob and FileReader(to get ArrayBuffer, maybe also async and await.).
It will be a long method if I just put these code on here.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #14)
> Looks I can't reuse the nonprintable method.
> Should I add these method to utils? (like the nonprintable,between .. )
> I also need to use Blob and FileReader(to get ArrayBuffer, maybe also async
> and await.).
> It will be a long method if I just put these code on here.
I think that it's OK if you implement your own method.
(you might want to put it into the utils/request-utils.js file)

Honza

Comment 16

[Locke Chen]

Attachment: Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review commit: https://reviewboard.mozilla.org/r/137074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137074/

Comment 17

[Locke Chen]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137074/diff/1-2/

Comment 18

[Locke Chen]

Changes:
 * fixed copyString not work when postData is binary string. 
 * added method to utils/request-utils.js to verification binary when post data can't parsing to JSON.
 * https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#249-254

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

https://reviewboard.mozilla.org/r/137074/#review140082

Thanks for the patch!

Please see my inline comments.

Honza

::: devtools/client/netmonitor/src/utils/request-utils.js:312
(Diff revision 2)
> +
> +function nonprintable(code) {
> +  return between(code, 0, 8) || code == 0xb || between(code, 0xe, 0x1f) || code == 0x7f;
> +}
> +
> +async function verificationBinaries(string) {

Could we rename the method to `isBinary` ?

::: devtools/client/netmonitor/src/utils/request-utils.js:314
(Diff revision 2)
> +  return between(code, 0, 8) || code == 0xb || between(code, 0xe, 0x1f) || code == 0x7f;
> +}
> +
> +async function verificationBinaries(string) {
> +  let blob = new Blob([string], {type: "application/octet-binary"});
> +  return new Promise((reslove, reject) => {

Typo `reslove` => `resolve`

::: devtools/client/netmonitor/src/utils/request-utils.js:328
(Diff revision 2)
> +      let buff = reader.result;
> +      let view = new Uint8Array(buff);
> +
> +      for (let code in view) {
> +        if (nonprintable(code)) {
> +          reslove(true);

Typo `reslove` => `resolve`

::: devtools/client/netmonitor/src/utils/request-utils.js:333
(Diff revision 2)
> +          reslove(true);
> +          return true;
> +        }
> +      }
> +
> +      reslove(false);

Typo `reslove` => `resolve`

Comment 20

[Locke Chen]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137074/diff/2-3/

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

https://reviewboard.mozilla.org/r/137074/#review140440

LGTM, thanks!

R+

Honza

Comment 22

[Locke Chen]

Looks like some test cases fail. I will try to fix it.

Comment 23

[Locke Chen]

Hi Honza

How can I run this test on my computer? I got test failed, but I have no idea to duplicate this. 
It not happen on my machine. I have tried 'mach mochitest devtools/client/netmonitor', but all test passed. :(
https://treeherder.mozilla.org/logviewer.html#?job_id=97589824&repo=try&lineNumber=4921

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

When I run the following *without* the patch it works

`mach test devtools/client/netmonitor/test/browser_net_copy_params.js`

When I run it *with* the patch I get the same error as try on Linux 64 (I am on Win10)

172 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_params.js | Timed out while polling clipboard for pasted data -
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:1003
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:1022
173 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_params.js | Uncaught exception -
Buffered messages finished
SUITE-END | took 17s

---

Can you try the same?

Perhaps the data copied into the clipboard are too big? 
What if you make is smaller?

Honza

Comment 25

[Locke Chen]

Thank you, now I can see that.
But have we tested copy row data in these test case?
If I removed the verify statement it will pass.
I have no idea why tests enter this scope:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#253-257

Thank you.

Comment 26

[Locke Chen]

Oh, sorry I found bug from my code.
I have changed from
for (let code in view) {
   if (nonprintable(code)) 
to
for (let count = 0 ;count < view.length; count++ )
   if (nonprintable(view[count]))

It's a stupid bug. XD

Should I also add one test for binaries?

Comment 27

[Locke Chen]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137074/diff/3-4/

Comment 28

[Locke Chen]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137074/diff/4-5/

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Locke Chen from comment #26)
> Should I also add one test for binaries?
Yes please!

Honza

Comment 30

[Locke Chen]

Hi Honsa
I have been tried to add one test case for this feature, but looks like this feature can't be pass the 'Timeout'.
http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1022

Currently I have used "Promise" to wait the parsing binary data to Array. (Blob > FileReader > ArrayBuffer > verification )
In this case should be more than 100 milliseconds.
So I can't just use the copyCilpboard method to verify binary data.
Could I checked-in the patch first? :(

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Does it help if you use `requestLongerTimeout`?

An enxample:
http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_addons_debug_bootstrapped.js#6

Honza

Comment 32

[Locke Chen]

Well, thanks. But still the same.
I always got this message:

185 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_params.js | Timed out while polling clipboard for pasted data - 
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:1003
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:1022
GECKO(6203) | Got this value: "R$Y\f$R÷\u0018\u0002\u0018:¼ú̶±,IìÔï ¤üCz"
186 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_params.js | Uncaught exception - 

The string from the clipboard is correct, I still don't know what happened here.

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Did you try to increase the timeout for SimpleTest.waitForClipboard?

You can pass custom timeout into SimpleTest.waitForClipboard and increase also maxPolls number, see here:
http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#988

You might want to also modify 
http://searchfox.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#519

Honza

Comment 34

[Locke Chen]

Attachment: Bug 1332949 - test case for copy binary data.

Review commit: https://reviewboard.mozilla.org/r/142152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142152/

Comment 35

[Locke Chen]

Comment on attachment 8865420 [details]
Bug 1332949 - In the network panel copy posted data or response data in binary format doesn't work correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137074/diff/5-6/

Comment 36

[Locke Chen]

Thanks, but I still got test fail. :(
Cloud you please help to me review the test case?(Maybe I used the wrong method.)

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Michal: can you please take a quick look at this change?

Honza

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Michal: sorry, wrong bug

Setting NI back to me.

Honza

Comment 39

[Aquila]

Attachment: CopyPasteBinaryData.png

Upper part network panel in FF
Lower part paste in notepad++

Comment 40

[Aquila]

Hello, I had to check some other binary requests and I though to give a try to FF if it was working and actually it copies the whole "text" (s. picture), but the pasted content is somehow incorrect with the encoding. In the picture is also visible the content-length, which is 82 chars, which is correct in the network panel of FF, but when pasted in notepad++ they are partially different and many more. I suppose it is a encoding problem, notepad++ support some like utf7 and utf8 but with both wasn't working. The only way to obtain the same chars in a word processor is to save all the requests in json format (HAR) and when you open it in notepad++ the chars are correct ... but the problem is the json encoding of the special chars like \U0001 etc. So also the export in json format is not directly usable, I had to load the file in programming language, I used powershell, which decode the \U0001 encoding to chars and then export them again in a new file. At the end I was able to "copy and paste" the binary data from the network panel, although I had to export all the request in json, import the exported json file in powershell, select the nodes with the requests and export them in plain text file.
It would be great if the encoding mismatching could be solved, it would make the life of few developers easier ;)

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8870686 [details]
Bug 1332949 - test case for copy binary data.

https://reviewboard.mozilla.org/r/142152/#review262480

I am unassigning myself since this needs general rebase.

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Locke: still around?

Honza

Comment 43

[Locke Chen]

Hi Honza,
yap, long time no see. :)

Comment 44

[Hengyeow (:tanhengyeow)]

Hi Locke

Are you still working on this? Do you require any assistance? :)

Comment 45

[Hengyeow (:tanhengyeow)]

Hi Honza

I tried to reproduce this and it seems to be not relevant anymore. Would appreciate if you can verify this :)

STR:

1. Go to https://mega.nz or any websites that allow you to upload files
2. Open the netmonitor panel
3. Upload a file
4. Observe the request of type application/octet-stream
5. Click on the request and copy the data present in the Request Payload section of the Params Panel.
6. Perform right click on the request > Copy > Copy POST Data
7. Compare the value with step 5 and 6, they are similar.

Comment 46

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

1. I created a simple HTML form:

<form action="index.html" method="post" enctype="multipart/form-data">
  First Name: <input type="text" name="fname"><br>
  Last Name: <input type="text" name="last-name  "><br>
  File: <input type="file" name="data"><br>
  <input type="submit" value="Submit">
</form>

2. Uploaded a binary file (favicon.ico) through the form + some textual data.

3. Selected the request and looked at the Params side panel (see the attached screenshot) - I can see the binary data

4. Using Copy POST Data on the request returns only text (no binary data of the file):

-----------------------------191691572411478
Content-Disposition: form-data; name="fname"

n
-----------------------------191691572411478
Content-Disposition: form-data; name="last-name  "

ln
-----------------------------191691572411478
Content-Disposition: form-data; name="data"; filename="favicon.ico"
Content-Type: image/x-icon

So, this doesn't seem to be fixed for me.

Honza

Comment 47

[Aaditya Arora]

Hi Jan,

I am able to reproduce this issue and would like to work on this issue.

thanks
Aaditya

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Aaditya Arora from comment #47)

I am able to reproduce this issue and would like to work on this issue.

Done, assigned to you!

Honza

Comment 49

[Patrick Brosset <:pbro>]

Hi Aaditya, checking quickly if you need more time on this bug (which is totally fine) and still intend to work on it.
If not, please let me know so it can be made available to others.
Thanks!

Comment 50

[Patrick Brosset <:pbro>]

Unassigning this bug.

Comment 51

[:Harald Kirschner :digitarald]

Honza, as it had past attempts from community – is this a good-first-bug?

Comment 52

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Good call, removing the flag.

Honza