DevTools Edit and resend broken when filter by XHR

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: andreip, Assigned: meag.harty, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

64 Branch
Firefox 64
good-first-bug

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: good-first-bug,)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 months ago
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.
(Reporter)

Comment 1

2 months ago
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.
(Reporter)

Updated

2 months ago
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
(Reporter)

Comment 2

2 months ago
This affects Firefox stable v62 and ESR 60.2.0esr so it's not something recently introduced.
Thanks for the report!
I can reproduce the problem on my machine Win10, Firefox Nightly 64

Honza
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
Blocks: 1475161
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug,
(Assignee)

Comment 5

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

Meag
Assignee: nobody → meag.harty
Status: NEW → ASSIGNED
@Meag: Hi, any progress on this bug? Are you still working on it?

Honza
Flags: needinfo?(meag.harty)
(Assignee)

Comment 7

2 months ago
Almost ready for a patch submission. I will look to submit one this evening. thanks!
Flags: needinfo?(meag.harty)
(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
(Assignee)

Comment 9

2 months ago
Created attachment 9013883 [details] [diff] [review]
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!
Attachment #9013883 - Flags: review?(odvarko)
(Assignee)

Comment 10

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

># HG changeset patch
># User meagonqz <meag.harty@gmail.com>
># 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);
>+});
(Assignee)

Comment 11

2 months ago
Created attachment 9013885 [details] [diff] [review]
bug1492730-patch-2

oops! This is the correct diff.
Attachment #9013883 - Attachment is obsolete: true
Attachment #9013883 - Flags: review?(odvarko)
Attachment #9013885 - Flags: review?(odvarko)
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...
Attachment #9013885 - Flags: review?(odvarko)
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
QA Contact: odvarko
Assignee: meag.harty → nobody
Status: ASSIGNED → NEW
Assignee: nobody → meag.harty
Status: NEW → ASSIGNED
QA Contact: odvarko
(Assignee)

Comment 14

a month ago
Created attachment 9015147 [details] [diff] [review]
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.
Attachment #9013885 - Attachment is obsolete: true
Attachment #9015147 - Flags: review?(odvarko)
(Assignee)

Comment 15

a month ago
Created attachment 9015156 [details] [diff] [review]
bug1492730-patch4

Small comment fixup and don't use EventUtils.sendMouseEvent for click events
Attachment #9015156 - Flags: review?(odvarko)
(Assignee)

Comment 16

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

># HG changeset patch
># User meagonqz <meag.harty@gmail.com>
># 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);
>+});
Attachment #9015147 - Attachment is obsolete: true
Attachment #9015147 - Flags: review?(odvarko)
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
Attachment #9015156 - Flags: review?(odvarko) → review+
@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
Keywords: checkin-needed

Comment 20

a month ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e6514203c9
network: show resent XHR when filtering r=honza
Keywords: checkin-needed

Comment 21

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c6e6514203c9
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.