1481732 - Query string is not parsed correctly into parameters

Reporter

Description

•

Last year

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

Send GET request where query string contains RQL syntax, e.g. ...&species=in=(52,60)&=...


Actual results:

Parameters are not parsed correctly and displayed in the Params tab:
species: in


Expected results:

should be displayed as:
species: in=(52,60)

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

Comment 1

•

Last year

Thanks for the report I can reproduce this on my Win10 machine too.

Some instructions for anyone interested in fixing this bug:

1) Here is the place where URL params are rendered in the Params side panel
See the `parseQueryString` method
https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/src/components/ParamsPanel.js#119

2) Here is `parseQueryString` method implementation:
https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/src/utils/request-utils.js#301-313

It's likely that it's this method that needs to be fixed.

Honza

Mentor: odvarko

Status: UNCONFIRMED → NEW

Ever confirmed: true

Keywords: good-first-bug

Priority: -- → P3

Whiteboard: good-first-bug

tossj

Assignee

Comment 2

•

Last year

Hi,

I have a fix for this bug, but I need help writing a test case. I've been looking at

https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/test/browser_net_complex-params.js

and

https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/test/html_params-test-page.html

and it looks like there is not an existing way to test GET requests with a parameter. Could you confirm this? It also appears as if the PUT and PATCH test requests are not being checked. Is this correct?

Flags: needinfo?(odvarko)

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

Comment 3

•

Last year

(In reply to tossj from comment #2)
> I have a fix for this bug, but I need help writing a test case. I've been
> looking at
> 
> https://searchfox.org/mozilla-central/rev/
> f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/test/
> browser_net_complex-params.js
> 
> and
> 
> https://searchfox.org/mozilla-central/rev/
> f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/test/
> html_params-test-page.html
> 
> and it looks like there is not an existing way to test GET requests with a
> parameter. Could you confirm this?
Yes, the html_params-test-page.html test page is currently not testing get with params.
It would be great if we could execute the `get` method on the page directly and pass
custom `query` argument to it.

We currently use `performRequest` by default, see
https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/devtools/client/netmonitor/test/head.js#768-774

We could perhaps append a new arguments to that method called `testName` and `testArgs`.
The new impl would be something like as follows:

async function performRequests(monitor, tab, count, testName = "performRequests", testArgs = null) {
  const wait = waitForNetworkEvents(monitor, count);
  await ContentTask.spawn(tab.linkedBrowser, count, requestCount => {
    let args = testArgs ? testArgs : [count];
    content.wrappedJSObject[testName](args);
  });
  await wait;
}

And in your test, you would call
await performRequests(monitor, tab, 1, "get", [address, query]);

What do you think?


> It also appears as if the PUT and PATCH
> test requests are not being checked. Is this correct?
Ah, that seems correct!

// Execute requests.
await performRequests(monitor, tab, 7); <---- SHOULD BE 9

Honza

Flags: needinfo?(odvarko)

tossj

Assignee

Comment 4

•

Last year

There is a get function at https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/devtools/client/netmonitor/test/html_params-test-page.html#21-32 that takes queries. Couldn't that be called in the performRequests functions that is at the end of the html file at line 60, (something like get('baz', 'species=in=(52,60)'); )?

What I meant when I said there is no way to test GET requests with parameters was that browser_net_complex-params.js only contains functions that assert (the ok() and is() functions) for POST requests, and for GET requests without parameters, in functions testParamsTab1(), testParamsTab2(), and testParamsTab3(). Due to this, another function would have to be written in browser_net_complex-params.js to assert that GET requests with parameters are displayed correctly. Is this correct?

Also, if I understand the code correctly, in addition to making this change:
    await performRequests(monitor, tab, 7); <---- SHOULD BE 9

the request still needs to be clicked in the devtools panel
(presumably using something similar to EventUtils.sendMouseEvent({ type: "mousedown" }, document.querySelectorAll(".request-list-item")[7]);,
as is done in line 64 of browser_net_complex-params.js, and then further ok()'s and is()'s have to be done to make sure the parameters are displaying correctly.

Should new tests be added in the way you suggested, or would the existing functions work just as well?

Flags: needinfo?(odvarko)

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

Comment 5

•

Last year

Sorry for the delay!

(In reply to tossj from comment #4)
> There is a get function at
> https://searchfox.org/mozilla-central/rev/
> aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/devtools/client/netmonitor/test/
> html_params-test-page.html#21-32 that takes queries. Couldn't that be called
> in the performRequests functions that is at the end of the html file at line
> 60, (something like get('baz', 'species=in=(52,60)'); )?
Yes, this is an option too.

> Also, if I understand the code correctly, in addition to making this change:
>     await performRequests(monitor, tab, 7); <---- SHOULD BE 9
> 
> the request still needs to be clicked in the devtools panel
> (presumably using something similar to EventUtils.sendMouseEvent({ type:
> "mousedown" }, document.querySelectorAll(".request-list-item")[7]);,
> as is done in line 64 of browser_net_complex-params.js, and then further
> ok()'s and is()'s have to be done to make sure the parameters are displaying
> correctly.
Sounds right to me.

> Should new tests be added in the way you suggested, or would the existing
> functions work just as well?
What are you suggesting sounds right, so feel free to go ahead!

(generalization of the `performRequests` might be done as part of another bug)

Should I assign this bug to you?

Honza

Flags: needinfo?(odvarko) → needinfo?(tossj)

tossj

Assignee

Comment 6

•

Last year

Yes, you can assign it to me. I will add the tests in the existing functions.

Flags: needinfo?(tossj)

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

Updated

•

Last year

Assignee: nobody → tossj

Status: NEW → ASSIGNED

tossj

Assignee

Comment 7

•

Last year

Attached patch 1481732.patch (obsolete) — Details — Splinter Review

Attachment #9003576 - Flags: review?(odvarko)

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

Comment 8

•

Last year

Comment on attachment 9003576 [details] [diff] [review]
1481732.patch

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

Thanks for the patch!

Looks good and works for me.

R+ assuming my inline comment is resolved.

Honza

::: devtools/client/netmonitor/test/browser_net_complex-params.js
@@ +81,5 @@
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> +    document.querySelectorAll(".request-list-item")[9]);
> +  await wait;
> +  testParamsTabGetWithArgs(new Map([
> +    ['species', 'in=(52,60)'],

Please use quotes: 
	
["species", "in=(52,60)"]

Attachment #9003576 - Flags: review?(odvarko) → review+

tossj

Assignee

Comment 9

•

Last year

Attached patch 1481732-edit-1.patch — Details — Splinter Review

I've made the quote change. After this patch is reviewed, is the next step pushing to a try server, or is bug finished?

Attachment #9003576 - Attachment is obsolete: true

Flags: needinfo?(odvarko)

Attachment #9004620 - Flags: review?(odvarko)

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

Comment 10

•

Last year

Comment on attachment 9004620 [details] [diff] [review]
1481732-edit-1.patch

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

(In reply to tossj from comment #9)
> Created attachment 9004620 [details] [diff] [review]
> 1481732-edit-1.patch
Thanks!

> I've made the quote change. After this patch is reviewed, is the next step
> pushing to a try server, or is bug finished?

Yes, please push to Try and if it's green mark this bug as `checkin-needed`
(Click the Edit bug blue button at the top and put `checkin-needed` into the Keywords field)

Honza

Attachment #9004620 - Flags: review?(odvarko) → review+

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

Updated

•

Last year

Flags: needinfo?(odvarko)

tossj

Assignee

Comment 11

•

Last year

:Honza, would you be my voucher to gain Level 1 Commit Access?

Flags: needinfo?(odvarko)

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

Comment 12

•

Last year

Done

Flags: needinfo?(odvarko)

tossj

Assignee

Comment 13

•

Last year

I ran tests on the try server, and here are my results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcfda3e7501447987725c7a7cffd0045e2243177

I am failing the W-e10s(wpt1) test for Windows 10 x64 opt. The error says "exit status 1073807364 when cloning repos fails. (task timeout during cloning)", and there is a bugzilla issue about the error (https://bugzilla.mozilla.org/show_bug.cgi?id=1485628).

Is there a way to fix this, or is this something out of the scope of the bug and is is okay to get the `checkin-needed` keyword?

Flags: needinfo?(odvarko)

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

Comment 14

•

Last year

(In reply to tossj from comment #13)
> I ran tests on the try server, and here are my results:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dcfda3e7501447987725c7a7cffd0045e2243177
> 
> I am failing the W-e10s(wpt1) test for Windows 10 x64 opt. The error says
> "exit status 1073807364 when cloning repos fails. (task timeout during
> cloning)", and there is a bugzilla issue about the error
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1485628).

I think you can ignore the failure, doesn't seem to be related.
Thanks!

Honza

Flags: needinfo?(odvarko)

tossj

Assignee

Updated

•

Last year

Keywords: checkin-needed

Pulsebot

Comment 15

•

Last year

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ce1febd1fc
Fixed netmonitor request parameter parsing so equals signs are not removed from value field. r=Honza

Keywords: checkin-needed

Andreea Pavel [:apavel]

Comment 16

•

Last year

bugherder

https://hg.mozilla.org/mozilla-central/rev/06ce1febd1fc

Status: ASSIGNED → RESOLVED

Closed: Last year

status-firefox64: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 64

1481732.patch Last year tossj 6.46 KB, patch	Honza : review+	Details \| Diff \| Splinter Review
1481732-edit-1.patch Last year tossj 6.47 KB, patch	Honza : review+	Details \| Diff \| Splinter Review

Bugzilla

Query string is not parsed correctly into parameters

Categories

(DevTools :: Netmonitor, defect, P3)

Tracking

(firefox64 fixed)

People

(Reporter: simon, Assigned: tossj, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: good-first-bug)

Crash Data

Security

(public)

User Story

Attachments

(1 file, 1 obsolete file)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Updated

Comment 7

Comment 8

Comment 9

Comment 10

Updated

Comment 11

Comment 12

Comment 13

Comment 14

Updated

Comment 15

Comment 16