Closed Bug 1403927 Opened 7 years ago Closed 7 years ago

HTTP inspector do not show parameters of a POST request

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 58

People

(Reporter: nchevobbe, Assigned: Honza)

References

Details

(Whiteboard: [reserve-console-html])

Attachments

(4 files)

Screen Shot 2017-09-28 at 15.45.17.png
295.73 KB, image/png
Details
Fixed
94.00 KB, image/png
Details
Bug 1403927 - Fix Params panel in HTTPi;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1403927 - Test for HTTPi Params panel;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
Steps to reproduce: 
1. go to https://polar-wood.glitch.me/
2. open the netmonitor
3. open the split console, and activate "xhr" filter
4. Enter something in the input and submit it
5. In the netmonitor, click on the newly shown entry, and click on the parameters tab
6. In the console expand on the newly shown message, and click on the parameters tab

Expected results:
The HTTP inspector should show the same thing that in the netmonitor 

Actual results:
The HTTP inspector has an empty parameters panel (see attachment), and the following error is logged to the browser console: 
"TypeError: undefined is not a valid URL.  request-utils.js:146:11"
Has STR: --- → yes
status-firefox57: --- → fix-optional
tracking-firefox57: --- → ?
Priority: -- → P2
Whiteboard: [console-html][triage]
Flags: qe-verify+
Priority: P2 → P3
QA Contact: iulia.cristescu
Whiteboard: [console-html][triage] → [reserve-console-html]
I believe that this will be fixed by bug 1404138.

In any case, this report depends on bug 1404138 and should be retested as soon it's fixed.

Honza
Depends on: 1404138
Remains broken after bug 1404138 was resolved
Andrew, where are you seeing the issue ? i built from mozilla-central and it worked for me.
If you are on nightly, the fix will be in there tomorrow i think
Flags: needinfo?(andrewmoffett67)
Apologies I assumed this would have been merged into todays nightly, I'll test again tomorrow.
Flags: needinfo?(andrewmoffett67)
Was this merged? Its still broken for me in 58.0a1 (2017-10-05) (64-bit)
Icant (In reply to Andrew Moff from comment #6)
> Was this merged?
Yes

I've been testing this on latest Nightly + Win10 and it works for me.

Anyone is able to reproduce this issue?

Honza
Attached image Fixed — 
works for me with the steps to reproduce from Comment 0, on 58.0a1 (2017-10-05) (64-bit) (see attachment)

Andrew, do you see the issue if you follow the steps from comment 0 ? If you do, there must be something weird. And if you don't see the issue for this case, can you provide more information about your application ?
Flags: needinfo?(andrewmoffett67)
The test outlined by the OP isn't a good test its simply appending the url with a query string, its not actually posting any params in the body. 

Try this test here. http://janodvarko.cz/firebug/tests/601/Issue601.htm

https://imgur.com/a/qz12w
Flags: needinfo?(andrewmoffett67)
(In reply to Andrew Moff from comment #10)
> The test outlined by the OP isn't a good test its simply appending the url
> with a query string, its not actually posting any params in the body. 
> 
> Try this test here. http://janodvarko.cz/firebug/tests/601/Issue601.htm
Yes, I can reproduce the problem with my test :-)

Thanks!
Honza
Tracking 57- for this issue.
tracking-firefox57: ? → -
I did analyze the problem and attached a prototype patch.

Some comments:

1) The main problem is that the code responsible for parsing POSTed data is part of MonitorPanel component (Net panel). This component is parent of all the detail panels in the Net panel, but not used in HTTPi.

2) The patch moves POST data parsing logic from MonitorPanel to ParamsPanel, so it's also executed by HTTPi. This is better location for that logic anyway.

3) The patch makes also sure that Console reducer is handling `UPDATE_REQUEST` action.

4) There is one hack related to 'getLongString' API used by the ParamsPanel now. There is currently now way to pass custom `getLongString` method into HTTPi from WebConsole. This is already solved by bug 1005755, so I am marking it as a blocker for this report.

Honza
Depends on: 1005755
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: P3 → P1
Just note for me.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9da10c1eca75825bc2aeda8b2a2a68684d9eff&selectedJob=137518877

Failures:
FAIL | devtools/client/netmonitor/test/browser_net_complex-params.js | Test timed out
FAIL | devtools/client/netmonitor/test/browser_net_post-data-01.js | Test timed out
Comment on attachment 8916521 [details]
Bug 1403927 - Fix Params panel in HTTPi;

https://reviewboard.mozilla.org/r/187654/#review195472

thanks honza. I'm not sure about the connection part on the ParamsPanel, could you answer my comment ?

::: devtools/client/netmonitor/src/components/params-panel.js:38
(Diff revision 3)
> -/*
> +/**
>   * Params panel component
>   * Displays the GET parameters and POST data of a request
>   */
> -function ParamsPanel({
> +const ParamsPanel = createClass({

This sound like it can make things a bit slower since we turn this into a stateful component (and monitor-panel.js is still a staetful one too)
There might be no solution to this, just wanted to point that out.

::: devtools/client/netmonitor/src/components/params-panel.js:183
(Diff revision 3)
> -module.exports = ParamsPanel;
> +module.exports = connect(
> +  (state) => ({
> +  }),
> +  (dispatch) => ({
> +    updateRequest: (id, data, batch) => dispatch(Actions.updateRequest(id, data, batch)),
> +  }),
> +)(ParamsPanel);

do we really want to connect the component ? it only seems to be fore getting the dispatch function.
If we don't want to render on redux changes here (it might already be the case because of parent components), we may only pass dispatch as a prop ? (or updateRequest)
Comment on attachment 8919271 [details]
Bug 1403927 - Test for HTTPi Params panel;

https://reviewboard.mozilla.org/r/190164/#review195760

The test looks good to me. I have a minor comment, but I'm fine if it lands as is. Thanks !

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_expand.js:105
(Diff revision 2)
>        resolve();
>      });
>    });
>  }
>  
> -async function waitForSourceEditor(messageNode) {
> +async function waitForSourceEditor(messageNode, selector) {

maybe we could pass it the tab (paramsTab, responseTab), and query on it ?

```
function waitForSourceEditor(tabNode) {
  return waitFor(() => tabNode.querySelector(".CodeMirror"));
}
```

I find it easier than to deal with selectors. Unless we can have multiple CodeMirror instances in one panel ?


Also, I think we can remove `async` since we don't use await here.
Attachment #8919271 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #21)
> > -function ParamsPanel({
> > +const ParamsPanel = createClass({
> 
> This sound like it can make things a bit slower since we turn this into a
> stateful component (and monitor-panel.js is still a staetful one too)
> There might be no solution to this, just wanted to point that out.

I need 'componentDidMount' and 'componentWillReceiveProps',
so I can't use a function, I guess.


> > +module.exports = connect(
> > +  (state) => ({
> > +  }),
> > +  (dispatch) => ({
> > +    updateRequest: (id, data, batch) => dispatch(Actions.updateRequest(id, data, batch)),
> > +  }),
> > +)(ParamsPanel);
> do we really want to connect the component ? it only seems to be fore
> getting the dispatch function.
> If we don't want to render on redux changes here (it might already be the
> case because of parent components), we may only pass dispatch as a prop ?
> (or updateRequest)

Yeah, I kept it there eventually since there is only one instance of that
panel and passing explicit dispatch prop is weird. But, let me know in case.

Try push green!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b24a37fdbe413ada3aa72e759c690620bd63befd

I still hope we can remove the NUMBER_OF_NETWORK_UPDATE thing,
but in another bug I guess.

Honza
Comment on attachment 8916521 [details]
Bug 1403927 - Fix Params panel in HTTPi;

https://reviewboard.mozilla.org/r/187654/#review196190
Attachment #8916521 - Flags: review?(nchevobbe) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b88d856e00f
Fix Params panel in HTTPi; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ac5ebd3c7e82
Test for HTTPi Params panel; r=nchevobbe
Backed out for failing mochitest-clipboard's devtools/client/netmonitor/test/browser_net_copy_params.js:

https://hg.mozilla.org/integration/autoland/rev/e8acb0a73375d4b08fa0152c03f15b640c81fa52
https://hg.mozilla.org/integration/autoland/rev/879e6055ac0927eca638945c371d0959b594fab0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ac5ebd3c7e8299b479167760d0ec0b277c6b3065&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=138140891&repo=autoland

[task 2017-10-19T09:09:07.585Z] 09:09:07     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_copy_params.js | The "Copy URL Parameters" context menu item should not be hidden. - 
[task 2017-10-19T09:09:07.586Z] 09:09:07     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_copy_params.js | Clipboard has the given value - 
[task 2017-10-19T09:09:07.586Z] 09:09:07     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_copy_params.js | The url query string copied from the selected item is correct. - 
[task 2017-10-19T09:09:07.586Z] 09:09:07     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_copy_params.js | The "Copy POST Data" context menu item should not be hidden. - 
[task 2017-10-19T09:09:07.587Z] 09:09:07     INFO - Buffered messages finished
[task 2017-10-19T09:09:07.587Z] 09:09:07     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_params.js | Test timed out -
Flags: needinfo?(odvarko)
Interesting is that I didn't see that failure in my try push:

Patch updated, fixing the failure.

Nicolas: the logic that parses urlencoded POST data needs to happen at both places:

1) MonitorPanel: when the user click on a request in the Net panel, the context menu offers "Copy POST DATA". The side bar (MonitorPanel) needs to parse the data at that moment, so the context menu works.

2) ParamsPanel: MonitorPanel is not used in the Console panel, so ParamsPanel needs to care.

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> Interesting is that I didn't see that failure in my try push

I think that's because it was in the mochitest-clipboard job. If you change your try push syntax from `-u mochitest-dt` to `-u mochitests` it will catch issues like that.
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b263ad73a219
Fix Params panel in HTTPi; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/37da7b7c6a40
Test for HTTPi Params panel; r=nchevobbe
I'm trying to verify this issue on the latest nightly, but I couldn't reproduce it because while trying to access the first website from comment 0 (https://polar-wood.glitch.me/) a message was displayed that the Project Not Found.

Could you please provide another website so I could verify this bug?

Thanks.
Flags: needinfo?(nchevobbe)
Sorry Hani, you can go to https://console-html-network-requests.glitch.me/ , click the button and expand the GET & POST requests and check that there are indeed params shown there.
Flags: needinfo?(nchevobbe)
Verified as fixed on Firefox Nightly 59.0a1 and Firefox 58.0b3 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: --- → verified
Ubuntu 16.04 just upgraded my Firefox to 57.0 and now I can't see any POST params in the inspector.  Is this actually fixed now? Should I chase Ubuntu to release a patch? Is there a workaround - apart from Use Chrome ;-) ?
Still not resolved in nightly 19.10.2017. See 1410705. If the request takes long enough to allow you to open the params tab before the server has responded, no headers, cookies, params, response, timings are shown.
(In reply to gvl from comment #43)
> Still not resolved in nightly 19.10.2017. See 1410705. If the request takes
> long enough to allow you to open the params tab before the server has
> responded, no headers, cookies, params, response, timings are shown.

I'm reposting that comment here since 1410705 was marked as a duplicate:

I've been testing it out, and here's what I've found:
The console is now showing information correctly on all tabs (Headers, Cookies, Params, Response, and Timings) EXCEPT when I try to open any of this information before the request is complete. If I attempt to view any tab before the request has resolved, then none of it will populate after it has resolved. If I leave it alone until it resolves, then all data is populated properly.

I'm running Windows 10 and Firefox 58.0b4 (64-bit) (Dev edition).
(In reply to meir_anolick from comment #44)
> (In reply to gvl from comment #43)
> > Still not resolved in nightly 19.10.2017. See 1410705. If the request takes
> > long enough to allow you to open the params tab before the server has
> > responded, no headers, cookies, params, response, timings are shown.
> 
> I'm reposting that comment here since 1410705 was marked as a duplicate:
> 
> I've been testing it out, and here's what I've found:
> The console is now showing information correctly on all tabs (Headers,
> Cookies, Params, Response, and Timings) EXCEPT when I try to open any of
> this information before the request is complete. If I attempt to view any
> tab before the request has resolved, then none of it will populate after it
> has resolved. If I leave it alone until it resolves, then all data is
> populated properly.
> 
> I'm running Windows 10 and Firefox 58.0b4 (64-bit) (Dev edition).
Please create a new report for it, this bug is already closed.
Also, having a test case (can be an online page) helps a lot.
You can cc my on that new bug.

Thanks!
Honza
Is it possible to backport it to Firefox 57? It is kind of a bug and regression.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: