Network monitor resent XHR requests don't show up anymore

VERIFIED FIXED in Firefox 65

Status

P3
normal
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: office, Assigned: tanhengyeow, Mentored)

Tracking

(Blocks: 2 bugs, {good-first-bug, regression})

51 Branch
Firefox 65
good-first-bug, regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox-esr60 wontfix, firefox53 wontfix, firefox54 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 verified)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Open a webpage with xhr calls. Open the network monitor and go to the xhr tab. Then right click on an xhr call and choose "Edit and Resend" and the "Send"


Actual results:

After submitting the new(resend) xhr call doesn't show show up in the xhr list. (worked in former firefox versions)


Expected results:

After submitting the new(resend) xhr call should show show up in the xhr list with the updated request/response values. this worked in former firefox version but itis not working anymore. this is very annoying for somebody who is programming xhr apis cause now its not possible to resend/manipulate/check xhr calls with firefox..
(Reporter)

Updated

2 years ago
Component: Untriaged → Developer Tools: Netmonitor

Comment 1

2 years ago
regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a18a3e221752e57641ed74c438fa20dc7f50cd1e&tochange=f9c16c7e72efaed1129bb949732ca0ec1936f387

Jarda Snajdr — Bug 1270096 - Netmonitor request replay: disable CORS checks, set request headers accurately r=Honza

Jarda, is it expected?
Blocks: 1270096
Status: UNCONFIRMED → NEW
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
Ever confirmed: true
Flags: needinfo?(jsnajdr)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
@Loic: I can't reproduce the problem on my machine.

Here is my STR:
1) Load http://janodvarko.cz/firebug/tests/601/Issue601.htm
2) Open DevTools, select the Network panel
3) Click the POST button on the page
4) Right click on the new entry (process.php) in the Network panel
5) Execute 'Edit and Resend' action from the context menu
6) New request entry appears in the list and 'New Request' form appears at the right side of the panel
7) Pressing 'Send' sends the request to the HTTP server (and the new entry created in step #6 is updated)

What am I doing wrong?

Honza
Flags: needinfo?(epinal99-bugzilla2)
(Reporter)

Comment 3

2 years ago
I restarted Firefox with a clean Profile and in safe Mode.
When i repoduce your steps:

1) Load http://janodvarko.cz/firebug/tests/601/Issue601.htm
2) Open DevTools, select the Network panel
3) Click the POST button on the page
4) Right click on the new entry (process.php) in the Network panel
5) Execute 'Edit and Resend' action from the context menu
6) New request entry appears in the list and 'New Request' form appears at the right side of the panel
7) Pressing 'Send' sends the request to the HTTP server (and the new entry created in step #6 is updated)

everything works the same as you described except the resend request vanishes from the left box after i do the resend...
(Reporter)

Comment 4

2 years ago
here is a screencapture video of the bug. (right click dialog is somehow missing in video)
https://www.youtube.com/watch?v=G8MqZjNK1PI

Comment 5

2 years ago
The request resent appears in the 'cause' column with the label "other" instead of "xhr". You can see that by selecting only "XHR" in Netmonitor tabs.
Flags: needinfo?(epinal99-bugzilla2)
(In reply to Loic from comment #5)
> The request resent appears in the 'cause' column with the label "other"
> instead of "xhr". You can see that by selecting only "XHR" in Netmonitor
> tabs.

Oh, this seems to be the real cause! If you watch the video closely enough, the "XHR" filter in the request list is enabled. However, the resent request cause is not "XHR", because it wasn't really triggered by the XmlHttpRequest API. It was triggered by privileged DevTools code, using the nsHttpChannel internal API. That's why it's marked as "other". And it's filtered out of the list for that reason.

Bug 1270096 changed the internal implementation how the request is being sent - before that, XmlHttpRequest API was used. But that had some serious shortcomings, like not allowing some headers to be changed. See bug 1270096 and its duplicates, blockers and see-also's for a lot more information.

If the resent request really didn't appear in the list, at least three mochitests (browser_net_resend*.js) would have to be failing. That's why I've been very confused by this unlikely bug report.

It's up for discussion whether we should "fake" the nsIContentPolicy field of the resent request (TYPE_OTHER vs TYPE_XMLHTTPREQUEST and other TYPE_* options) - it's possible to set it if we want to. Then the "cause" field won't tell the real "truth", but it may match the user expectations better.
Flags: needinfo?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #6)
> (In reply to Loic from comment #5)
> Oh, this seems to be the real cause! If you watch the video closely enough,
> the "XHR" filter in the request list is enabled. 
I see, thanks Jarda.

> It's up for discussion whether we should "fake" the nsIContentPolicy field
> of the resent request (TYPE_OTHER vs TYPE_XMLHTTPREQUEST and other TYPE_*
> options) - it's possible to set it if we want to. Then the "cause" field
> won't tell the real "truth", but it may match the user expectations better.
I put this on a list of things to discuss.

Honza
Priority: -- → P4
Whiteboard: [netmonitor][triage]
(Reporter)

Comment 8

2 years ago
Thx guys for the quick support!! You are right. The call is now in the the "All" tab. Since it always was in the XHR tab that didnt come to my mind...
Blocks: 1307743
Flags: qe-verify?
Priority: P4 → --
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
> > It's up for discussion whether we should "fake" the nsIContentPolicy field
> > of the resent request (TYPE_OTHER vs TYPE_XMLHTTPREQUEST and other TYPE_*
> > options) - it's possible to set it if we want to. Then the "cause" field
> > won't tell the real "truth", but it may match the user expectations better.
> I put this on a list of things to discuss.
> 

That make sense since User should not care about how we internally deal with the request, but they will expect the resent package is still XHR
(In reply to Fred Lin [:gasolin] from comment #10)
> > > It's up for discussion whether we should "fake" the nsIContentPolicy field
> > > of the resent request (TYPE_OTHER vs TYPE_XMLHTTPREQUEST and other TYPE_*
> > > options) - it's possible to set it if we want to. Then the "cause" field
> > > won't tell the real "truth", but it may match the user expectations better.
> > I put this on a list of things to discuss.
> > 
> 
> That make sense since User should not care about how we internally deal with
> the request, but they will expect the resent package is still XHR

Yes, I agree. The 'cause' field (in the new request) should be the same as in the original request.

Honza
Detailed instructions for anyone interested in fixing this bug:

1) The point is to properly set 'request cause' according to the original request. Currently 'other' is always used.

2) When an existing request is cloned and ready for custom modification, it needs to also copy the 'cause' field here:
https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/devtools/client/netmonitor/reducers/requests.js#207

let newRequest = new Request({
  ...
  cause: clonedRequest.cause,
});


3) Here is the place where the Net monitor sends the custom request data to the backend:
https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/devtools/client/netmonitor/actions/requests.js#73

The `data` object should also have 'cause' field set from the 'selected' (original) request.

let data = {
  ...
  cause: selected.cause,
};


4) Here is the backend place where the 'cause' field needs to be set.
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#1657

The `contentPolicyType` should be set according to 'cause' field from 'message.request'.
If the 'message.request' is not set 'Ci.nsIContentPolicy.TYPE_OTHER' should be used
by default (to keep backward compatibility with the client)

5) We need a test for this.


Honza
Mentor: odvarko
Keywords: good-first-bug

Comment 13

2 years ago
@Honza I would like to work on this :)
(In reply to Deepjyoti Mondal from comment #13)
> @Honza I would like to work on this :)

Excellent, assigned to you.

Honza
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Blocks: 1348737
No longer blocks: 1307743
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Deepjyoti, any luck? Do you need help, or would you like to pass this bug on for someone else to work on?

Looks like we have a workaround in comment 5.
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
status-firefox55: --- → fix-optional
Flags: needinfo?(djmdeveloper060796)

Comment 17

2 years ago
@Lis I will be uploading a patch soon. I was busy with some stuff. I will submit a patch ASAP

Updated

2 years ago
Flags: needinfo?(djmdeveloper060796)

Comment 18

2 years ago
Created attachment 8858555 [details] [diff] [review]
bug1340100.patch

I have set contentPolicyType to XMLHTTPREQUEST if message.request is set otherwise TYPE_OTHER.

I tried logging the value of message.request.cause but it is showing unavailable in the console
Attachment #8858555 - Flags: review?(odvarko)
Comment on attachment 8858555 [details] [diff] [review]
bug1340100.patch

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

Thanks for the patch!

Please see my comments inline.

Honza

::: devtools/server/actors/webconsole.js
@@ +1656,5 @@
>        uri: NetUtil.newURI(url),
>        loadingNode: doc,
>        securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> +      contentPolicyType: message.request ? Ci.nsIContentPolicy.TYPE_XMLHTTPREQUEST :
> +                                           Ci.nsIContentPolicy.TYPE_OTHER

You need to use `message.request.cause`. The `cause` field is the one you are passing in `sendCustomRequest` on the client side.

But, you are passing a text (e.g. 'xhr', 'stylesheet', etc.) so, you can't compare with Ci.nsIContentPolicy.TYPE_XMLHTTPREQUEST, which is a number. You need to utilize LOAD_CAUSE_STRINGS map in shared/webconsole/network-monitor.js file and convert string to number.
Attachment #8858555 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> You need to use `message.request.cause`. The `cause` field is the one you
> are passing in `sendCustomRequest` on the client side.
Sorry, I made a typo..

You need to use `message.cause`

Honza

Comment 21

2 years ago
Created attachment 8860800 [details] [diff] [review]
bug1340100.patch

@Honza thanks for the pointers. LOAD_CAUSE_STRINGS in network-monitor.js provides a mapping between nsIContentPolicyTypes and the corresponding strings. So to get the reverse mapping I have included a new function stringToCauseType in the same file. Now how should I use it in onSendHTTPRequest method in webconsole.js. Should I create a new property in NetworkMonitor object (used in webconsole.js)?
Attachment #8858555 - Attachment is obsolete: true
Attachment #8860800 - Flags: review?(odvarko)
Comment on attachment 8860800 [details] [diff] [review]
bug1340100.patch

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

(In reply to Deepjyoti Mondal from comment #21)
> Created attachment 8860800 [details] [diff] [review]
> bug1340100.patch
> 
> @Honza thanks for the pointers. LOAD_CAUSE_STRINGS in network-monitor.js
> provides a mapping between nsIContentPolicyTypes and the corresponding
> strings. So to get the reverse mapping I have included a new function
> stringToCauseType in the same file. Now how should I use it in
> onSendHTTPRequest method in webconsole.js. Should I create a new property in
> NetworkMonitor object (used in webconsole.js)?

1) Import in webconsole.js 
const { stringToCauseType } = require("devtools/shared/webconsole/network-monitor");

2) Export from network-monitor.js 
exports.stringToCauseType = stringToCauseType;

3) Use:
contentPolicyType: stringToCauseType(message.request.cause.type) || Ci.nsIContentPolicy.TYPE_OTHER

Honza
Attachment #8860800 - Flags: review?(odvarko)

Comment 23

2 years ago
Created attachment 8861553 [details] [diff] [review]
bug1340100.patch

@Honza I have exported stringToCauseType function in network-monitor and required it in webconsole.js. Now I am able to call the function.I have logged the value being passed to stringToCauseType function and the value the filter function is returning. Its returning an Integer (as expected). However when I am using this function in onSendHTTPRequest , tab is crashing.I have attached a patch showing the changes I have made
Attachment #8860800 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
What exactly should I do to reproduce the crash?

Here is what I am doing:

1) load google.com
2) right click on a request, e.g. the first 'document'
3) pick edit & resend, click send button in the form

I am seeing new request with correct 'cause' type. No crash.

Honza
Flags: needinfo?(odvarko)

Comment 25

2 years ago
Created attachment 8864285 [details]
crash.png

@Honza I am also performing the same steps as you mentioned above but I am getting this page (screen shot).
Flags: needinfo?(odvarko)
Can you attach the crash report (see about:crashes)

Honza
Flags: needinfo?(odvarko)
(Reporter)

Comment 27

a year ago
Hi guys, any updates on this bug?
@Deepjyoti: can you still reproduce the crash?

Honza
Flags: needinfo?(djmdeveloper060796)

Updated

6 months ago
Product: Firefox → DevTools
Assignee: djmdeveloper060796 → nobody
Status: ASSIGNED → NEW
status-firefox55: fix-optional → ---

Comment 29

3 months ago
Hi!
Can I work on this issue?
Thanks!
Assigned to you!
Honza
Assignee: nobody → pong7219
Status: NEW → ASSIGNED
Summary: Networkmonitor resended XHR requests dont show up anymore → Network monitor resent XHR requests don't show up anymore
@Mellina: any progress on this bug? Can I help somehow?

Honza
Flags: needinfo?(pong7219)

Comment 32

2 months ago
Hi @Honza,

Sorry for the delay. I got sick this weekend and was not able to look at it :/ I will look at it today though and will give you updates on this.

Thanks.
Mellina.
Flags: needinfo?(pong7219)
Unassigning Mellina (based on offline request)

Honza
Assignee: pong7219 → nobody
Status: ASSIGNED → NEW
Hi @Honza, is this still a bug? I tried to reproduce following your guide:

>>>
Here is my STR:
1) Load http://janodvarko.cz/firebug/tests/601/Issue601.htm
2) Open DevTools, select the Network panel
3) Click the POST button on the page
4) Right click on the new entry (process.php) in the Network panel
5) Execute 'Edit and Resend' action from the context menu

At this step, the tab is shrunk and I get no Edit and Resent menu. Is that what we are looking for?

6) New request entry appears in the list and 'New Request' form appears at the right side of the panel
7) Pressing 'Send' sends the request to the HTTP server (and the new entry created in step #6 is updated)
>>>

In that case, with some extra help for where to look, I want to be assigned this one for #outreachy
Thanks!

Ioannis
Flags: needinfo?(odvarko)
@Honza, I will drop out my request as I started working on another one :) Thank you!
Flags: needinfo?(odvarko)
Assignee: nobody → E0032242
Status: NEW → ASSIGNED
(Assignee)

Comment 36

2 months ago
Created attachment 9015630 [details]
Bug 1340100 - Set 'request cause' according to the original request. r=Honza

Set 'request cause' according to the original request.
(Assignee)

Comment 37

2 months ago
:Honza

Patch is up and ready for review :D
Flags: needinfo?(odvarko)
Excellent!

The patch looks good, but let's see try results before R+

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

Btw. there is one conflict (in browser.ini) when applying on the latest m-c, so you need to rebase.

Honza
Flags: needinfo?(odvarko)
I am seeing some test failures, can you please investigate?

Honza
Flags: needinfo?(djmdeveloper060796) → needinfo?(E0032242)
(Assignee)

Comment 40

2 months ago
Resolved the merge conflict.

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed,busted,exception&classifiedState=unclassified&revision=ce9f536a06290693c1305a12d8f05b64bc2e1edb

Everything's positive :)
Flags: needinfo?(E0032242) → needinfo?(odvarko)
(In reply to Heng Yeow (:tanhengyeow) from comment #40)
> Resolved the merge conflict.
> 
> Try results:
> https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed,
> busted,
> exception&classifiedState=unclassified&revision=ce9f536a06290693c1305a12d8f05
> b64bc2e1edb
No try selector specified, no test running

I am usually using this selector:

./mach try -b do -p linux64 -u mochitest-dt -t none

ie
-b both debug and optimized build
-t testing on Linux 64 bit
-u run all DevTools tests
-t no Talos (performance tests)

Please push to try again.

Honza
Flags: needinfo?(odvarko)
I am seeing:
try: -b o -p linux64 -u mochitest-1 -t none

You need to execute "mochitest-dt" (DevTools) test suite.

It should be:
try: -b o -p linux64 -u mochitest-dt -t none

Honza
Flags: needinfo?(odvarko) → needinfo?(E0032242)
I pushed to try today and I am still seeing some test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec1bb00ab107af9773d09d19798ba3f6da7597c&selectedJob=205739716

---

1) The tab loaded by these tests crashes.
devtools/client/debugger/test/mochitest/browser_dbg_worker-source-map.js
devtools/client/application/test/browser_application_panel_debug-service-worker.js

2) There is an error in Browser console for the following test:
devtools/client/netmonitor/test/browser_net_resend_headers.js

error occurred while processing 'sendHTTPRequest: TypeError: cause is undefined; can't access its "type" property
Stack: sendHTTPRequest@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1594:7
Async*onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1313:15
receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5
MessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5
ready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5
_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:892:5
connectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:313:12
onConnect</<@resource://devtools/server/startup/frame.js:50:22
onConnect<@ main.js:1168
_unknownError
resource://devtools/server/main.js:1168:5
_queueResponse/responsePromise<
resource://devtools/server/main.js:1197:27


---

Can you please localize, which exact change in the patch causes the crash? Consequently we can ask the platform team for a fix.
Also please take a look a the error, perhaps it's simple to fix it?

Honza
(Assignee)

Comment 45

2 months ago
:Honza

Fixed a test that requires the new cause field to be set when sending a new request, see Phabricator for more details.

Tried to run the try results again:
https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a7125df5f9b3ffb1eaafe9d0427e07756d456684&selectedJob=206151108
I can reproduce the failing tests in my local build. However, I wasn't able to get much insights through the error logs. Also, most of them are linked to existing intermittent bugs. Any advice on proceeding on with this?
Flags: needinfo?(E0032242) → needinfo?(odvarko)
Thanks for the fix!

So, the problem with the crash is the following line:

require("devtools/server/actors/network-monitor/network-observer");

... in `server/actors/webconsole.js` file

When running the 'devtools/client/application/test/browser_application_panel_debug-service-worker.js' test locally and analyzing the output, I am seeing:

0:12.27 GECKO(9288) JavaScript error: resource://devtools/shared/worker/loader.js, line 423: Error: Can't import XPCOM service from worker thread!
0:12.27 GECKO(9288) JavaScript error: resource://devtools/shared/worker/loader.js, line 273: NetworkError: A network error occurred.


@Julian: do you know if there is any chance to reuse the `stringToCauseType()` helper in `server/actors/webconsole.js`?
It crashes because the file is loaded by a worker?

Honza
Flags: needinfo?(odvarko) → needinfo?(jdescottes)
I am not sure yet why this crashes. The patch here breaks the toolbox as soon as you target is a worker or a service worker. The file itself is not loaded via a worker. It seems to work if you switch the require to a lazyRequireGetter

  loader.lazyRequireGetter(this, "stringToCauseType", 
    "devtools/server/actors/network-monitor/network-observer", true);

But we still need to investigate more.
So we are initializing a DebuggerServer in the same process as the worker we target. And it looks we don't have access to Cc, Ci etc... in this process. It is really scary we haven't run into this issue before. I am not sure we have any guideline or anything to prevent hitting codepaths requiring Cc/Ci/... on the server.

I would suggest you unblock the situation here by using a workaround (eg the lazyRequireGetter from my comment above) and we move the overall issue to a follow up bug.
Flags: needinfo?(jdescottes)
Opened Bug 1500461 to investigate the root issue.
(Assignee)

Comment 50

2 months ago
Updated the patch to use `lazyRequireGetter`. 

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ae3f348845c478eddcc960a3486175ed2e65cba
Flags: needinfo?(odvarko)
Attachment #8861553 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Duplicate of this bug: 1432724

Comment 52

2 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/527ac367e301
Set 'request cause' according to the original request. r=Honza
https://hg.mozilla.org/mozilla-central/rev/527ac367e301
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
status-firefox54: fix-optional → wontfix
status-firefox63: --- → wontfix
status-firefox64: --- → wontfix
status-firefox-esr60: --- → wontfix
Flags: in-testsuite+
Duplicate of this bug: 1377741
From the above comments, to sumarize my understanding of this issue: the resent XHR requests have as catalogued cause in the network panel: "other", when the request cause should be XHR for both initial send and the resend request as well. 

Reproduced this behavior on affected build: Nightly 64.0a1 20181010100123 / Windows 8.1 and verified the fix on the latest Nightly 65.0a1 20181031223503, thus marking this issue as verified.
Status: RESOLVED → VERIFIED
status-firefox65: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.