1492730 - DevTools Edit and resend broken when filter by XHR

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Andrei Petcu]

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

Steps to reproduce:

Go to any web page that has XHR requests for example https://www.theguardian.com/politics/2018/sep/19/theresa-may-tells-eu27-she-wont-delay-brexit-despite-lack-of-a-deal

Open dev tools, network monitor and filter requests by XHR

Select an entry and hit edit and resend.

Video https://vimeo.com/290841061


Actual results:

The side panel gets closed instead of switching to edit and resend mode.


Expected results:

If instead of filtering by XHR, I select all requests then the edit and resend works fine. I expect edit and resend to work both with the filter and without.

Comment 1

[Andrei Petcu]

This is a long standing bug. I reported it just now since it is the first time I got an actual reproducible list of steps. I used a new profile, not my main one.

Comment 2

[Andrei Petcu]

This affects Firefox stable v62 and ESR 60.2.0esr so it's not something recently introduced.

Comment 3

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

Thanks for the report!
I can reproduce the problem on my machine Win10, Firefox Nightly 64

Honza

Comment 4

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

Some information for anyone interested in fixing this bug:

0) It all starts with filtered request list (XHR filter applied in this case)

1) When Edit & Resend action is executed a clone of the selected request is created.
Here is the context menu handler:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js#183

CLONE_SELECTED_REQUEST action is fire handled here:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/reducers/requests.js#120

2) You can see that not every prop is cloned here:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/requests.js#133

The new clone is also used as the new current selection:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/reducers/requests.js#147

3) When a new request appears in the list (a clone in this case) the list is automatically filtered
These functions are used for filtering:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/utils/filter-predicates.js#10

But, these functions use proper that are not available in the clone (e.g. mimeType). So, the clone is filtered out and thus the current selected request (clone) is not visible. 

That also means that the Details panel is auto-closed:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/components/MonitorPanel.js#84

... and that's the bug

4) One solution is to always treat a clone as something that should *not* be filtered out.
This approach seems to be fine since already used for sorting:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/selectors/requests.js#11-15


The filtering selector function is here:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/selectors/requests.js#38-46

Perhaps we could do something like as follows (or better introduce filterWithClone function):
const getFilterFn = createSelector(
  state => state.filters,
  filters => r => {
    const matchesType = Object.keys(filters.requestFilterTypes).some(filter => {
      if (r.id.endsWith("-clone")) {
        return true;
      }
      return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r);
    });
    return matchesType && isFreetextMatch(r, filters.requestFilterText);
  }
);


This also needs a test case:
This one might be used as an inspiration:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/test/browser_net_edit_resend_caret.js#13

Honza

Comment 5

[meag.harty]

Hi! I'm interested in working on this bug.

Meag

Comment 6

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

@Meag: Hi, any progress on this bug? Are you still working on it?

Honza

Comment 7

[meag.harty]

Almost ready for a patch submission. I will look to submit one this evening. thanks!

Comment 8

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

(In reply to meag.harty from comment #7)
> Almost ready for a patch submission. I will look to submit one this evening.
> thanks!

Awesome!
Honza

Comment 9

[meag.harty]

Attachment: bug1492730-patch

Your bug analysis was spot on, so this was a simple fix. Had some fun with the mochitests, let me know what you think. Thanks!

Comment 10

[meag.harty]

Comment on attachment 9013883 [details] [diff] [review]
bug1492730-patch

># HG changeset patch
># User meagonqz <[e-mail]>
># Date 1537849869 14400
>#      Tue Sep 25 00:31:09 2018 -0400
># Node ID 893521e25268dd3e5bd78c4d09df57c6c55c4824
># Parent  221c18ebe962f68358b4cba927df9099ea935b40
>Bug 1492730 - network: show resent XHR when filtering r=honza
>
>diff --git a/devtools/client/netmonitor/src/selectors/requests.js b/devtools/client/netmonitor/src/selectors/requests.js
>--- a/devtools/client/netmonitor/src/selectors/requests.js
>+++ b/devtools/client/netmonitor/src/selectors/requests.js
>@@ -35,10 +35,13 @@ function sortWithClones(requests, sorter
>   return sorter(a, b);
> }
> 
>-const getFilterFn = createSelector(
>+const getFilterWithCloneFn = createSelector(
>   state => state.filters,
>   filters => r => {
>     const matchesType = Object.keys(filters.requestFilterTypes).some(filter => {
>+      if (r.id.endsWith("-clone")) {
>+        return true;
>+      }
>       return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r);
>     });
>     return matchesType && isFreetextMatch(r, filters.requestFilterText);
>@@ -78,7 +81,7 @@ const getSortedRequests = createSelector
> 
> const getDisplayedRequests = createSelector(
>   state => state.requests,
>-  getFilterFn,
>+  getFilterWithCloneFn,
>   getSortFn,
>   ({ requests }, filterFn, sortFn) => {
>     const arr = [...requests.values()].filter(filterFn).sort(sortFn);
>diff --git a/devtools/client/netmonitor/test/browser.ini b/devtools/client/netmonitor/test/browser.ini
>--- a/devtools/client/netmonitor/test/browser.ini
>+++ b/devtools/client/netmonitor/test/browser.ini
>@@ -120,6 +120,7 @@ subsuite = clipboard
> skip-if = (os == 'mac') # Bug 1479782
> [browser_net_header-docs.js]
> [browser_net_edit_resend_caret.js]
>+[browser_net_edit_resend_with_filtering.js]
> [browser_net_filter-01.js]
> [browser_net_filter-02.js]
> [browser_net_filter-03.js]
>diff --git a/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js
>new file mode 100644
>--- /dev/null
>+++ b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js
>@@ -0,0 +1,67 @@
>+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+"use strict";
>+
>+/**
>+* Tests if resending a XHR request while filtering X displays
>+* the correct requests
>+*/
>+
>+add_task(async function() {
>+  const { tab, monitor } = await initNetMonitor(POST_RAW_URL);
>+
>+  const { document, store, windowRequire, parent } = monitor.panelWin;
>+  const parentDocument = parent.document;
>+  const Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>+  store.dispatch(Actions.batchEnable(false));
>+
>+  // Execute 1 XHR requests
>+  await performRequests(monitor, tab, 1);
>+
>+  // Filter by XHR
>+  info("Filtering by XHR... ");
>+  EventUtils.sendMouseEvent({ type: "click" },
>+  document.querySelector(".requests-list-filter-xhr-button"));
>+
>+  // Confirm XHR request and click it
>+  info("Confirming XHR request... ");
>+  const xhrRequestItem = document.querySelectorAll(".request-list-item")[0];
>+  const waitForItem = waitUntil(() => xhrRequestItem);
>+  await waitForItem;
>+  info("Clicking XHR request... ");
>+  EventUtils.sendMouseEvent({ type: "mousedown" }, xhrRequestItem);
>+
>+  const {
>+    getSelectedRequest
>+  } = windowRequire("devtools/client/netmonitor/src/selectors/index");
>+  const firstRequest = getSelectedRequest(store.getState());
>+
>+  // Open context menu and execute "Edit & Resend".
>+  EventUtils.sendMouseEvent({ type: "contextmenu" }, xhrRequestItem);
>+  const contextMenuResend =  parentDocument.querySelector("#request-list-context-resend");
>+  info("Clicking resend menu... ");
>+  contextMenuResend.click();
>+
>+  // Click Resend
>+  const resendButton = document.querySelector("#custom-request-send-button");
>+  await waitUntil(() => resendButton);
>+  resendButton.click();
>+
>+  // Filtering by "other" so the resent request is visible after completion
>+  EventUtils.sendMouseEvent({ type: "click" },
>+  document.querySelector(".requests-list-filter-other-button"));
>+
>+  // Select the cloned request
>+  const cloneRequestItem = document.querySelectorAll(".request-list-item")[0];
>+  cloneRequestItem.click();
>+  const resendRequest = getSelectedRequest(store.getState());
>+
>+  ok(resendRequest.id !== firstRequest.id, "The second XHR request was made and is unique");
>+
>+  ok(resendRequest.id.replace(/-clone$/, "") == firstRequest.id, "The second XHR request is a clone of the first");
>+
>+  return teardown(monitor);
>+});

Comment 11

[meag.harty]

Attachment: bug1492730-patch-2

oops! This is the correct diff.

Comment 12

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

Comment on attachment 9013885 [details] [diff] [review]
bug1492730-patch-2

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

Thanks for the patch, it fixes the problem for me!

Please see my inline comments related to the test.

Also, can you please look at bug 1432724, it looks like duplicate.

Honza

::: devtools/client/netmonitor/src/selectors/requests.js
@@ +35,4 @@
>    return sorter(a, b);
>  }
>  
> +const getFilterWithCloneFn = createSelector(

Please create a comment in front of the method.

Something like:

/**
 * Take clones into account when filtering. If a request is
 * a clone it's not filtered out.
 */

::: devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js
@@ +9,5 @@
> +* Tests if resending a XHR request while filtering X displays
> +* the correct requests
> +*/
> +
> +add_task(async function() {

Please format the comment as:

/**
 *
 */

And remove the empty line between the comment and the function name

@@ +23,5 @@
> +
> +  // Filter by XHR
> +  info("Filtering by XHR... ");
> +  EventUtils.sendMouseEvent({ type: "click" },
> +  document.querySelector(".requests-list-filter-xhr-button"));

What this querySelector is supposed to do?

@@ +26,5 @@
> +  EventUtils.sendMouseEvent({ type: "click" },
> +  document.querySelector(".requests-list-filter-xhr-button"));
> +
> +  // Confirm XHR request and click it
> +  info("Confirming XHR request... ");

Please remove the trailing space in the log (after three dots).
This repeats in several other places.

@@ +29,5 @@
> +  // Confirm XHR request and click it
> +  info("Confirming XHR request... ");
> +  const xhrRequestItem = document.querySelectorAll(".request-list-item")[0];
> +  const waitForItem = waitUntil(() => xhrRequestItem);
> +  await waitForItem;

Why you awaiting the return value from querySelector?
It's either there or not, but not changing over time.

@@ +46,5 @@
> +  contextMenuResend.click();
> +
> +  // Click Resend
> +  const resendButton = document.querySelector("#custom-request-send-button");
> +  await waitUntil(() => resendButton);

Again why the await here?
Can we just remove that?

@@ +51,5 @@
> +  resendButton.click();
> +
> +  // Filtering by "other" so the resent request is visible after completion
> +  EventUtils.sendMouseEvent({ type: "click" },
> +  document.querySelector(".requests-list-filter-other-button"));

Similarly as before what this querySelector is supposed to do?
You are not using its return value...

Comment 13

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

Just to make sure you know..
The following bug is related and could be your next:
Bug 1377741 - Edit and resend doesn't work with filtered content

Honza

Comment 14

[meag.harty]

Attachment: bug1492730-patch3

Fixed up the comment style, fixed up a few missing indentations that made the use of querySelectors unclear (they were part of a sending a mouseEvent), and removed the info logging  in favor of comments.

I removed what I intended to be the waiting for the querySelector to be truthy since locally the tests passed fine without it; I should have made the value passed to waitUntil( () => querySelector("#item") ) without the predefined variable.

Comment 15

[meag.harty]

Attachment: bug1492730-patch4

Small comment fixup and don't use EventUtils.sendMouseEvent for click events

Comment 16

[meag.harty]

Comment on attachment 9015147 [details] [diff] [review]
bug1492730-patch3

># HG changeset patch
># User meagonqz <[e-mail]>
># Date 1537849869 14400
>#      Tue Sep 25 00:31:09 2018 -0400
># Node ID c71f97397718b8cd392980c5b47045a0930db980
># Parent  221c18ebe962f68358b4cba927df9099ea935b40
>Bug 1492730 - network: show resent XHR when filtering r=honza
>
>diff --git a/devtools/client/netmonitor/src/selectors/requests.js b/devtools/client/netmonitor/src/selectors/requests.js
>--- a/devtools/client/netmonitor/src/selectors/requests.js
>+++ b/devtools/client/netmonitor/src/selectors/requests.js
>@@ -35,10 +35,17 @@ function sortWithClones(requests, sorter
>   return sorter(a, b);
> }
> 
>-const getFilterFn = createSelector(
>+/**
>+ * Take clones into account when filtering. If a request is
>+ * a clone, it's not filtered out.
>+ */
>+const getFilterWithCloneFn = createSelector(
>   state => state.filters,
>   filters => r => {
>     const matchesType = Object.keys(filters.requestFilterTypes).some(filter => {
>+      if (r.id.endsWith("-clone")) {
>+        return true;
>+      }
>       return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r);
>     });
>     return matchesType && isFreetextMatch(r, filters.requestFilterText);
>@@ -78,7 +85,7 @@ const getSortedRequests = createSelector
> 
> const getDisplayedRequests = createSelector(
>   state => state.requests,
>-  getFilterFn,
>+  getFilterWithCloneFn,
>   getSortFn,
>   ({ requests }, filterFn, sortFn) => {
>     const arr = [...requests.values()].filter(filterFn).sort(sortFn);
>diff --git a/devtools/client/netmonitor/test/browser.ini b/devtools/client/netmonitor/test/browser.ini
>--- a/devtools/client/netmonitor/test/browser.ini
>+++ b/devtools/client/netmonitor/test/browser.ini
>@@ -120,6 +120,7 @@ subsuite = clipboard
> skip-if = (os == 'mac') # Bug 1479782
> [browser_net_header-docs.js]
> [browser_net_edit_resend_caret.js]
>+[browser_net_edit_resend_with_filtering.js]
> [browser_net_filter-01.js]
> [browser_net_filter-02.js]
> [browser_net_filter-03.js]
>diff --git a/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js
>new file mode 100644
>--- /dev/null
>+++ b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js
>@@ -0,0 +1,59 @@
>+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+"use strict";
>+
>+/**
>+ * Tests if resending a XHR request while filtering XHR displays
>+ * the correct requests
>+ */
>+add_task(async function() {
>+  const { tab, monitor } = await initNetMonitor(POST_RAW_URL);
>+
>+  const { document, store, windowRequire, parent } = monitor.panelWin;
>+  const parentDocument = parent.document;
>+  const Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>+  store.dispatch(Actions.batchEnable(false));
>+
>+  // Execute 1 XHR requests
>+  await performRequests(monitor, tab, 1);
>+  EventUtils.sendMouseEvent({ type: "click" },
>+    document.querySelector(".requests-list-filter-xhr-button"));
>+
>+  // Confirm XHR request and click it
>+  const xhrRequestItem = document.querySelectorAll(".request-list-item")[0];
>+  EventUtils.sendMouseEvent({ type: "mousedown" }, xhrRequestItem);
>+
>+  const {
>+    getSelectedRequest
>+  } = windowRequire("devtools/client/netmonitor/src/selectors/index");
>+  const firstRequest = getSelectedRequest(store.getState());
>+
>+  // Open context menu and execute "Edit & Resend".
>+  EventUtils.sendMouseEvent({ type: "contextmenu" }, xhrRequestItem);
>+  EventUtils.sendMouseEvent({ type: "click" },
>+    parentDocument.querySelector("#request-list-context-resend"));
>+
>+  // Click Resend
>+  EventUtils.sendMouseEvent({ type: "click" },
>+    document.querySelector("#custom-request-send-button"));
>+
>+  // Filtering by "other" so the resent request is visible after completion
>+  EventUtils.sendMouseEvent({ type: "click" },
>+    document.querySelector(".requests-list-filter-other-button"));
>+
>+  // Select the cloned request
>+  EventUtils.sendMouseEvent({ type: "click" },
>+    document.querySelectorAll(".request-list-item")[0]);
>+  const resendRequest = getSelectedRequest(store.getState());
>+
>+  ok(resendRequest.id !== firstRequest.id,
>+    "The second XHR request was made and is unique");
>+
>+  ok(resendRequest.id.replace(/-clone$/, "") == firstRequest.id,
>+    "The second XHR request is a clone of the first");
>+
>+  return teardown(monitor);
>+});

Comment 17

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

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

Comment 18

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

Comment on attachment 9015156 [details] [diff] [review]
bug1492730-patch4

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

Looks good to me!

R+ assuming try is green

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

Thanks for the help.
Honza

Comment 19

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

@Meag: try push is green and the next step is marking the bug as `checkin-needed` so somebody can land it in the repo. I am doing it now for you, so you can see how it's done. You can do it yourselves next time.

Thanks for the help!

Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e6514203c9
network: show resent XHR when filtering r=honza

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c6e6514203c9