1481732 - Query string is not parsed correctly into parameters

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Simon Speich]

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)

Comment 1

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

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

Comment 2

[tossj]

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?

Comment 3

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

(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

Comment 4

[tossj]

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?

Comment 5

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

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

Comment 6

[tossj]

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

Comment 7

[tossj]

Attachment: 1481732.patch

Comment 8

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

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)"]

Comment 9

[tossj]

Attachment: 1481732-edit-1.patch

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

Comment 10

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

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

Comment 11

[tossj]

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

Comment 12

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

Done

Comment 13

[tossj]

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?

Comment 14

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

(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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Andreea Pavel [:apavel]]

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