1309605 - in devtools network panel, json post missing in params tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Tonio]

Attachment: Screenshot from 2016-10-12 17-40-59.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161010004016

Steps to reproduce:

Inspect a post request with json content in the network panel, then choose params tab.


Actual results:

Even with data posted (I check with right-click «copy POST data», there is content), json content looks empty.

Reproduced in developer edition & nightly.


Expected results:

See JSON content in params tools (as it works in FF49, see screenshot for similar request)

Comment 1

[Loic]

Do you have a simple testcase to reproduce the issue?

Comment 2

[Tonio]

Attachment: test case

Test case attached.
No access to my Linux machine right now, can’t reproduce on OSX/developerEdition but I can reproduce on browserstack/windows/FF51

Comment 3

[Tonio]

Attachment: Screenshot of the bug on win 8.1 FF 51 (with browserstack)

Comment 4

[Loic]

Ok, I see the bug when loading the testcase locally in FF52, thanks.

Comment 5

[Loic]

Attachment: screenshot-json.png

In fact, it's hidden by default but visible if you click on the empty white line under "JSON".

Reg range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=08e9ded26ada149385af32e1c1d89f30e3d8279c&tochange=3e537d8eb88c440fd0b7aa88deec56aa688715b0

Oriol — Bug 996691 - Stop ignoring empty string property names when inspecting an object. r=fitzgen

Comment 6

[Loic]

Oriol, could you check this regression, please.

Comment 7

[Oriol Brufau [:Oriol]]

I can't test this right now, but I suspect

> let jsonItem = jsonScope.addItem("", { enumerable: true });

(https://dxr.mozilla.org/mozilla-central/rev/26f2e65163e9f3cfab071a9823293217b1e1de8d/devtools/client/netmonitor/netmonitor-view.js#3137)

should now be

> let jsonItem = jsonScope.addItem(undefined, { enumerable: true });

Comment 8

[Oriol Brufau [:Oriol]]

And I see there is another `addItem(""` in Storage:
https://dxr.mozilla.org/mozilla-central/rev/26f2e65163e9f3cfab071a9823293217b1e1de8d/devtools/client/storage/ui.js#687

Effectively, using
> sessionStorage.setItem('a', '[[1],2]');

and clicking the value in Storage produces an empty top level in variablesview.

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: scope-init.patch

This fixes the problem.

But probably some test should be added. I won't have time for that.

Comment 10

[Gerry Chang [:gchang]]

Hi :jwalker,
Since this is related to devtools, I was wondering if you can help find someone to follow up with this issue?

Comment 11

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

Attachment: bug1309605.patch

@Jarda: can I ask for quick review please? This part will be refactored at some point, but it's good to have it fixed anyways.

@Mike: can you please provide a test for the Storage panel related fix?

Honza

Comment 12

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57dedef21b99f19838106ad2c48d7c91f6cbb50

Honza

Comment 13

[Marcia Knous [:marcia]]

Tracking 52+ for this regression which has a testcase.

Comment 14

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8802935 [details] [diff] [review]
bug1309605.patch

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

Looks acceptable as a temporary fix, until the code is completely refactored. If this was intended as a permanent fix, I would have the following objections:

- the root cause is that the VariablesView API is inconsistent and error-prone. VariablesView.addScope omits the header if the aName parameter is falsy - see [1]. Scope.addItem omits the header only if aName === undefined - see [2]. That's the thing that should be fixed, not the usage sites.

[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/VariablesView.jsm#140
[2] http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/VariablesView.jsm#1333

- the new mochitest is checking contents of a VariablesView. It does it by relying on a lot of implementation details, like CSS class names. Many other tests do the same thing. But instead of factoring the logic out into several functions that are easy to use, it's copy and pasted many times over. This makes the tests much more complex and intimidating than they really need to be. And replacing the VariablesView with something else will need a very laborious and boring update of all the tests, instead of just updating the implementation details in a few shared functions.

- the html_post-json-text-page.html file also makes me sad. Instead of having just one "html_generic-post.html" that can do any post it's asked for and can be reused across many tests, there is a lot of almost identical HTML files that differ only in the parameters they pass to XHR.

Comment 15

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

(In reply to Jarda Snajdr [:jsnajdr] from comment #14)
> Comment on attachment 8802935 [details] [diff] [review]
> bug1309605.patch
> 
> Review of attachment 8802935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks acceptable as a temporary fix, until the code is completely
> refactored.
Correct, it's temp.

> If this was intended as a permanent fix, I would have the
> following objections:
> 
> - the root cause is that the VariablesView API is inconsistent and
> error-prone. VariablesView.addScope omits the header if the aName parameter
> is falsy - see [1]. Scope.addItem omits the header only if aName ===
> undefined - see [2]. That's the thing that should be fixed, not the usage
> sites.
> 
> [1]
> http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/
> VariablesView.jsm#140
> [2]
> http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/
> VariablesView.jsm#1333
> 
> - the new mochitest is checking contents of a VariablesView. It does it by
> relying on a lot of implementation details, like CSS class names. Many other
> tests do the same thing. But instead of factoring the logic out into several
> functions that are easy to use, it's copy and pasted many times over. This
> makes the tests much more complex and intimidating than they really need to
> be. And replacing the VariablesView with something else will need a very
> laborious and boring update of all the tests, instead of just updating the
> implementation details in a few shared functions.
> 
> - the html_post-json-text-page.html file also makes me sad. Instead of
> having just one "html_generic-post.html" that can do any post it's asked for
> and can be reused across many tests, there is a lot of almost identical HTML
> files that differ only in the parameters they pass to XHR.

Agreed, and I hope we can tackle all these issues as part of the Netmonitor.html project.

Thanks Jarda!
Honza

Comment 16

[Oriol Brufau [:Oriol]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #14)
> - the root cause is that the VariablesView API is inconsistent and
> error-prone. VariablesView.addScope omits the header if the aName parameter
> is falsy - see [1]. Scope.addItem omits the header only if aName ===
> undefined - see [2].

I attempted to do something consistent when I worked in bug 996691. But the code is a mess, and I didn't see a simple way to initialize scopes with undefined instead of empty string without breaking lots of things. So at the end I gave up and only initialized items with undefined. VariablesView has more serious problems than using empty string in one place and undefined in another, anyways.

Comment 18

[Jarda Snajdr [:jsnajdr]]

(In reply to Oriol from comment #16)
> I attempted to do something consistent when I worked in bug 996691. But the
> code is a mess, and I didn't see a simple way to initialize scopes with
> undefined instead of empty string without breaking lots of things. So at the
> end I gave up and only initialized items with undefined. VariablesView has
> more serious problems than using empty string in one place and undefined in
> another, anyways.

No problem, I just wanted to point out that althought it's a review+, I actually don't like the patch at all :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eade199cd98
Fix JSON Post body preview in the Net panel; r=jsnajdr

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7eade199cd98

Comment 21

[Gerry Chang [:gchang]]

Hi :Honza,
Since this also affects 51, do you think the patch is worth uplifting to 51?

Comment 23

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

(In reply to Jarda Snajdr [:jsnajdr] from comment #18)
> (In reply to Oriol from comment #16)
> > I attempted to do something consistent when I worked in bug 996691. But the
> > code is a mess, and I didn't see a simple way to initialize scopes with
> > undefined instead of empty string without breaking lots of things. So at the
> > end I gave up and only initialized items with undefined. VariablesView has
> > more serious problems than using empty string in one place and undefined in
> > another, anyways.
> 
> No problem, I just wanted to point out that althought it's a review+, I
> actually don't like the patch at all :)

Yes, but spending any more time on analyzing and/or refactoring related code isn't effective since we want to rewrite it anyways (using React).

Honza

Comment 24

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

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Created attachment 8802935 [details] [diff] [review]
> bug1309605.patch
> @Mike: can you please provide a test for the Storage panel related fix?

Mike, I created a follow-up for the inspector part. The Inspector panel isn't fixed as part of this bug.

Bug 1313350

Honza

Comment 25

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

Comment on attachment 8802935 [details] [diff] [review]
bug1309605.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 996691
[User impact if declined]: No JSON preview for URL Params (Developer tools, Network panel)
[Describe test coverage new/current, TreeHerder]: Covered (there is a new test in the bug)
[Risks and why]: Rather smaller risk, one line changed, no external API changed.
[String/UUID change made/needed]: n/a

Honza

Comment 26

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

(In reply to Gerry Chang [:gchang] from comment #21)
> Hi :Honza,
> Since this also affects 51, do you think the patch is worth uplifting to 51?
Good point, done.
Honza

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8802935 [details] [diff] [review]
bug1309605.patch

Fix a regression related to devtools network panel. Take it in 51 aurora.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/059c6aa6f3fd

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for devtools bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4004224&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7ceeddb165c

Comment 30

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

(In reply to Wes Kocher (:KWierso) from comment #29)
> I had to back this out for devtools bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4004224&repo=mozilla-
> aurora
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e7ceeddb165c
Sorry, yes, there are other changes (like e.g. introducing a new 110n.js module) required by this patch. Since this bug isn't critical, let's not uplift it.

Honza

Comment 31

[Oriol Brufau [:Oriol]]

Maybe the fix could be uplifted without the test?

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See comment 31.

Comment 33

[Gerry Chang [:gchang]]

Per comment #30, mark 51 won't fix.

Comment 34

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

(In reply to Oriol from comment #31)
> Maybe the fix could be uplifted without the test?
I think that the fix is rather minor and can wait one cycle.

Honza