1122102 - post json params displayed as text in params>request payload

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[bulforce]

Attachment: Screen Shot 2015-01-15 at 7.34.15 PM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

a POST ajax request with json object as params 


Actual results:

In the network tab under Params i see the json object shown as text.


Expected results:

The network tab should parse params and display them as expandable tree(like it is in the response tab)

This of course should happen when the payload is json object.

Comment 1

[bulforce]

Attachment: Screen Shot 2015-01-15 at 7.34.39 PM.png

Comment 2

[mrashkovsky]

I'd like to point out that this functionality exits on non-Linux versions of Firefox. It should not be marked as "enhancement" but rather it is a bug. Furthermore, it makes the Params tab pretty useless for any serious work for debugging network traffic. This issue is forcing me off of Firefox for the time being.

Comment 3

[Geoff Lankow (:darktrojan)]

I have nothing to add.

Comment 4

[mrashkovsky]

(In reply to Geoff Lankow (:darktrojan) from comment #3)
> I have nothing to add.

Hi Geoff,

Sorry if that spammed you, it was not my intent. I was hoping you could re-classify this away from "enhancement" since this isn't consistent with behavior in Firefox under non-Linux operating systems. Those versions of Firefox have a proper tree view for the params. I don't think this issue is getting any attention because it is marked as "enhancement", and I don't think it rightly qualifies as one.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

We don't pay any attention to the "enhancement" categorization, this is just a bug like any other. I also don't see any difference between OS X and Linux in this case: both display the body as text in the Params view. I've used GMail in this test, but if you have another instance of a web site that behaves differently in Linux compared to OS X, I'm happy to investigate.

Comment 6

[bulforce]

I reported this bug about a year ago, Its very sad to see only blabla and blabla, and not even an ownership over the ticket. 

Is there any reason for you guys not to address this?

You dont build this firefox edition for yourselves you build it for other developers to use, having this problem is pain in the ass for me and I think for everybody else that uses the tool.

Comment 7

[:Gijs (he/him)]

(In reply to bulforce from comment #6)
> I reported this bug about a year ago, Its very sad to see only blabla and
> blabla, and not even an ownership over the ticket. 
> 
> Is there any reason for you guys not to address this?

It's very sad to see your comment describing discussion about the nature of the issue as "blabla". I also don't know what ownership would add here - there are thousands of unfixed bugs on bugzilla that are assigned to someone other than nobody. That doesn't really accomplish anything.

As for not addressing this, yes: there are lots of other bugs on file as well. People are busy fixing those, which makes it extra sad to see you basically yell at us as if we're incompetent, just because we happen not to be working on *this particular* issue.

> You dont build this firefox edition for yourselves you build it for other
> developers to use,

You are implying that people are being selfish in not addressing this bugreport, without any evidence (and without really elaborating on what behaviour makes you say that or how Mozilla are just making things "for ourselves"). That is not a very nice thing to do.

> having this problem is pain in the ass for me and I think
> for everybody else that uses the tool.

Not everyone posts JSON in the request body, so this is trivially not true.

Now, I know almost nothing about the devtools, but I had a poke about, and I wrote a piece of concept patch, so I'll put that up in a bit and we can hopefully move this forward more constructively.

Comment 8

[:Gijs (he/him)]

Attachment: Patch v0.1

This probably breaks tests, but before I get into fixing them, Panos, is this the right thing?

(is there a way to remove all the __proto__ stuff from the JSON views both here and in the response view? It's probably not really helpful for JSON views... if necessary I can add an option for this to populate() and the setSingleVariable helper?)

Comment 9

[:Gijs (he/him)]

Attachment: Patch -w because that might be more clear

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8675630 [details] [diff] [review]
Patch v0.1

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

I know that we have a way to tell the VariablesView widget to hide __proto__, but I can't remember off the top of my head what it is. Maybe Victor remembers?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8675630 [details] [diff] [review]
Patch v0.1

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

::: devtools/client/netmonitor/netmonitor-view.js
@@ +2457,5 @@
>          searchPlaceholder: L10N.getStr("paramsFilterText")
>        }));
>      this._json = new VariablesView($("#response-content-json"),
>        Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, {
>          onlyEnumVisible: true,

Ah, I remember now! This is the option that makes the VariablesView hide stuff like __proto__. Adding this option to the initialization block for this._params above should do the trick.

You probably want to use two separate VariablesViewController instances to control the two views. I don't believe we ever reuse a controller for more than one views and I'm not sure it was designed with that use case in mind. Victor wrote it, so he might remember.

Comment 12

[:Gijs (he/him)]

(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 8675630 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8675630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/netmonitor/netmonitor-view.js
> @@ +2457,5 @@
> >          searchPlaceholder: L10N.getStr("paramsFilterText")
> >        }));
> >      this._json = new VariablesView($("#response-content-json"),
> >        Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, {
> >          onlyEnumVisible: true,
> 
> Ah, I remember now! This is the option that makes the VariablesView hide
> stuff like __proto__. Adding this option to the initialization block for
> this._params above should do the trick.
> 
> You probably want to use two separate VariablesViewController instances to
> control the two views. I don't believe we ever reuse a controller for more
> than one views and I'm not sure it was designed with that use case in mind.
> Victor wrote it, so he might remember.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/VariablesViewController.jsm#718

seems to indicate that the "static" .attach method here creates a new controller instance whenever called on a view that has no controller, so I believe that the current patch would be OK in that respect. I'll leave the feedback? for Victor in case he has other comments/concerns.

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 8675630 [details] [diff] [review]
Patch v0.1

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

This LGTM but it'll probably break some tests. I think it's fine to use the same controller for multiple variable views, although I can't think of a place where we actually do it already. It's also been a very long time since I looked at the code.

Let me know if you need any help with the tests.

::: devtools/client/netmonitor/netmonitor-view.js
@@ +2864,5 @@
> +        paramsScope.locked = true;
> +
> +        $("#request-post-data-textarea-box").hidden = false;
> +        let editor = yield NetMonitorView.editor("#request-post-data-textarea");
> +

Nit: extra newline here.

Comment 14

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1122102 - post json params shouldn't be displayed as text in params>request payload, r?vporof

Bug 1122102 - post json params shouldn't be displayed as text in params>request payload, r?vporof

Comment 15

[:Gijs (he/him)]

Comment on attachment 8675631 [details] [diff] [review]
Patch -w because that might be more clear

The mozreview version passes all netmonitor/ tests locally. I realized that my previous approach was actually hiding the request params from the URL, so now I added a separate scope for the posted JSON to the variables view instead... let's hope try agrees:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c96922edb52f

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 8675631 [details] [diff] [review]
> Patch -w because that might be more clear
> 
> The mozreview version passes all netmonitor/ tests locally. I realized that
> my previous approach was actually hiding the request params from the URL, so
> now I added a separate scope for the posted JSON to the variables view
> instead... let's hope try agrees:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c96922edb52f

This is green besides pre-existing intermittents, and memory leaks from parent fx-team csets fixed by fx-team's 8edcaa544e24.

Comment 17

[Victor Porof [:vporof][:vp]]

Comment on attachment 8675631 [details] [diff] [review]
Patch -w because that might be more clear

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

::: devtools/client/netmonitor/netmonitor-view.js
@@ +2858,3 @@
>        // This is really awkward, but hey, it works. Let's show an empty
>        // scope in the params view and place the source editor containing
>        // the raw post data directly underneath.

All of this should be properly indented.

Comment 18

[Victor Porof [:vporof][:vp]]

https://reviewboard.mozilla.org/r/23549/#review21513

Comment 19

[:Gijs (he/him)]

Comment on attachment 8680026 [details]
MozReview Request: Bug 1122102 - post json params shouldn't be displayed as text in params>request payload, r?vporof

Per IRC, Victor says this looks good.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4ea8997d2ed8

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ea8997d2ed8