Closed Bug 1309605 Opened 3 years ago Closed 3 years ago

in devtools network panel, json post missing in params tab

Categories

(DevTools :: Netmonitor, defect)

51 Branch
defect
Not set

Tracking

(firefox49 unaffected, firefox50 unaffected, firefox51+ wontfix, firefox52+ fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + wontfix
firefox52 + fixed

People

(Reporter: antoine, Assigned: Honza)

References

Details

(Keywords: regression, testcase)

Attachments

(6 files)

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)
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Untriaged → Developer Tools: Netmonitor
Do you have a simple testcase to reproduce the issue?
Flags: needinfo?(antoine)
Attached file 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
Flags: needinfo?(antoine)
Ok, I see the bug when loading the testcase locally in FF52, thanks.
Keywords: testcase
Attached image 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
Oriol, could you check this regression, please.
Blocks: 996691
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(oriol-bugzilla)
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
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 });
Flags: needinfo?(oriol-bugzilla)
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.
Attached patch scope-init.patchSplinter Review
This fixes the problem.

But probably some test should be added. I won't have time for that.
Hi :jwalker,
Since this is related to devtools, I was wondering if you can help find someone to follow up with this issue?
Flags: needinfo?(jwalker)
Flags: needinfo?(jwalker) → needinfo?(odvarko)
Attached patch bug1309605.patchSplinter Review
@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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko) → needinfo?(mratcliffe)
Attachment #8802935 - Flags: review?(jsnajdr)
Tracking 52+ for this regression which has a testcase.
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.
Attachment #8802935 - Flags: review?(jsnajdr) → review+
(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
(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.
Duplicate of this bug: 1307167
(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 :)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eade199cd98
Fix JSON Post body preview in the Net panel; r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7eade199cd98
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi :Honza,
Since this also affects 51, do you think the patch is worth uplifting to 51?
Flags: needinfo?(odvarko)
Duplicate of this bug: 1313082
(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
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(mratcliffe)
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
Attachment #8802935 - Flags: approval-mozilla-aurora?
(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
See Also: → 1313350
Comment on attachment 8802935 [details] [diff] [review]
bug1309605.patch

Fix a regression related to devtools network panel. Take it in 51 aurora.
Attachment #8802935 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Flags: needinfo?(odvarko)
Maybe the fix could be uplifted without the test?
Per comment #30, mark 51 won't fix.
(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
Flags: needinfo?(odvarko)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.