post json params displayed as text in params>request payload

RESOLVED FIXED in Firefox 45

Status

--
enhancement
RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: bulforce, Assigned: Gijs)

Tracking

35 Branch
Firefox 45

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8549730 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8549731 [details]
Screen Shot 2015-01-15 at 7.34.39 PM.png
(Assignee)

Updated

4 years ago
Component: Untriaged → Developer Tools: Netmonitor
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All

Comment 2

4 years ago
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.
Flags: needinfo?(geoff)
I have nothing to add.
Flags: needinfo?(geoff)

Comment 4

4 years ago
(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.
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.
(Reporter)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8675630 [details] [diff] [review]
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?)
Attachment #8675630 - Flags: feedback?(past)
(Assignee)

Comment 9

3 years ago
Created attachment 8675631 [details] [diff] [review]
Patch -w because that might be more clear
(Assignee)

Updated

3 years ago
Attachment #8675631 - Attachment is patch: true
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?
Attachment #8675630 - Flags: feedback?(vporof)
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.
Attachment #8675630 - Flags: feedback?(past)
(Assignee)

Comment 12

3 years ago
(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 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.
Attachment #8675630 - Flags: feedback?(vporof) → feedback+
(Assignee)

Comment 14

3 years ago
Created attachment 8680026 [details]
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
Attachment #8680026 - Flags: review?(vporof)
(Assignee)

Updated

3 years ago
Attachment #8675630 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
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
Attachment #8675631 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
(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 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.
Attachment #8675631 - Flags: review+
(Assignee)

Comment 19

3 years ago
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.
Attachment #8680026 - Flags: review?(vporof) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ea8997d2ed8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.