1287172 - Console XHR inspection breaks when loading POST tab on a file upload, can't load any other tabs afterwards

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[usermirss]

Attachment: windowsfirewall_logo.jpg

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

Steps to reproduce:

0. Page with the code to reproduce the issue: https://jsfiddle.net/macqmgp5/
It just allows you to select a file and then Post it thru XHR, the issue is probably not a file POST upload specific issue, but the data that is being sent in the headers that breaks the firefox devtools code when trying to view it.

1. Open devtools console
2. Select a file (happens with images mosty, tested with JPG)
3. Click on upload selected file
4. See the XHR ajax call in the console, open it by clicking on the arrow (so it opens in the console)
5. Go to the POST tab
6. BUG: Now all tabs of the XHR Response (including the one you just entered) are empty and the browser toolbox shows the error:

chrome://devtools/content/webconsole/webconsole.xul : Unable to run script because scripts are blocked internally.(unknown) 
not well-formed



Actual results:

6. BUG: Now all tabs of the XHR Response (including the one you just entered) are empty and the browser toolbox shows the error:



Expected results:

All tabs should show its data, Headers, POST, and Response.
The NETWORK Tab of the devtools DOES show this data when inspecting the XHR POST (so this one does not have the bug)

Comment 1

[Brian Grinstead [:bgrins]]

Can confirm the problem, thanks for reporting.  Honza, do you have bandwidth to look at this?

Comment 2

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

I can reproduce the problem in the current Nightly (Built from https://hg.mozilla.org/mozilla-central/rev/cde56ead650fd302be1d440507485b9abf7c163a), but not in my own build (fx-team), Win OS, uploading the attached image.

Brian, can you try it on your build?

Honza

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> I can reproduce the problem in the current Nightly (Built from
> https://hg.mozilla.org/mozilla-central/rev/
> cde56ead650fd302be1d440507485b9abf7c163a), but not in my own build
> (fx-team), Win OS, uploading the attached image.
> 
> Brian, can you try it on your build?

What I'm seeing on both Nightly and fx-team now is the POST tab spinning and never finishing, along with the console error message "chrome://devtools/content/webconsole/webconsole.xul : Unable to run script because scripts are blocked internally." 

Although the other net tabs are opening fine in this case.  I think it depends on what image you use - I'll upload one that causes the behavior I'm describing but also send a needinfo to the reporter to see if there's a particular image that causes the breakage described in Comment 0.

Comment 4

[Brian Grinstead [:bgrins]]

Clearing needinfo as I see an image is already attached to the bug.  Honza, were you using the attached image in the test?

Comment 5

[Jarda Snajdr [:jsnajdr]]

I can reproduce the bug, going to work on it now.

Comment 6

[Jarda Snajdr [:jsnajdr]]

This happens because the PostTab tries to render the raw binary data of the POSTed image as a text markup at [1]. This happens when the POST data are long enough to be a longString actor. The code for short strings at least tries to sanitize() the binary data, although the sanitize function looks questionable, too.

Responses are rendered OK, because getResponseContent returns the data as a base64 encoded string (with an encoding=base64 property), while getRequestPostData returns a binary string.

Comment 7

[Jarda Snajdr [:jsnajdr]]

Attachment: Console netlogging: sanitize POST body data when it's a longString

Minimal steps to reproduce:
1. Create a file that contains the 0x19 ASCII character (end-of-medium) and is big enough to be a longString (i.e., 10000 bytes). Like this:
printf '\x19%010000d' 0 > upload.bin
2. Upload the file as a POST body (see the jsfiddle above)

The patch just adds a sanitize call to post-tab.js component in case where the body is a longStringActor.

Asking :bgrins for review, as Honza is on vacation.

I'm wondering what's the best way to test this. It's a view-only change and it seems that the console netlogging doesn't have any mochitests that would test the view markup. Or should it be a unit test for the post-tab.js React component?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #7)
> Created attachment 8775505 [details] [diff] [review]
> Console netlogging: sanitize POST body data when it's a longString
> 
> Minimal steps to reproduce:
> 1. Create a file that contains the 0x19 ASCII character (end-of-medium) and
> is big enough to be a longString (i.e., 10000 bytes). Like this:
> printf '\x19%010000d' 0 > upload.bin
> 2. Upload the file as a POST body (see the jsfiddle above)
> 
> The patch just adds a sanitize call to post-tab.js component in case where
> the body is a longStringActor.
> 
> Asking :bgrins for review, as Honza is on vacation.
> 
> I'm wondering what's the best way to test this. It's a view-only change and
> it seems that the console netlogging doesn't have any mochitests that would
> test the view markup. Or should it be a unit test for the post-tab.js React
> component?

I was looking at tests for this and realized somehow the patch with tests never got landed in Bug 1211525 (that is, https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1211525&attachment=8735424).  I think the right thing to do here is rebase that patch and then add a test case on top of the new browser_net_post.js test.

Comment 9

[Jarda Snajdr [:jsnajdr]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> I was looking at tests for this and realized somehow the patch with tests
> never got landed in Bug 1211525 (that is,
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=1211525&attachment=8735424).  I think the right thing to do here is
> rebase that patch and then add a test case on top of the new
> browser_net_post.js test.

Oh, good catch! Unfortunately, the tests are failing for nontrivial reasons. I filed separate bug 1290437 for that.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > I was looking at tests for this and realized somehow the patch with tests
> > never got landed in Bug 1211525 (that is,
> > https://bugzilla.mozilla.org/page.cgi?id=splinter.
> > html&bug=1211525&attachment=8735424).  I think the right thing to do here is
> > rebase that patch and then add a test case on top of the new
> > browser_net_post.js test.
> 
> Oh, good catch! Unfortunately, the tests are failing for nontrivial reasons.
> I filed separate bug 1290437 for that.

OK, in that case I will test this patch and make sure it fixes STR for me and if so we can land it before the merge for 50

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8775505 [details] [diff] [review]
Console netlogging: sanitize POST body data when it's a longString

Review of attachment 8775505 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me, let's get this fix landed in time for 50 and follow up with tests in Bug 1290437

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fc777f423569
Console netlogging: sanitize POST body data when it's a longString. r=bgrins

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc777f423569