Closed
Bug 1340100
Opened 8 years ago
Closed 6 years ago
Network monitor resent XHR requests don't show up anymore
Categories
(DevTools :: Netmonitor, defect, P3)
Tracking
(firefox51 wontfix, firefox52 wontfix, firefox-esr60 wontfix, firefox53 wontfix, firefox54 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 verified)
People
(Reporter: office, Assigned: tanhengyeow, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, regression, Whiteboard: [netmonitor-reserve])
Attachments
(2 files, 3 obsolete files)
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..
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
Comment 2•8 years ago
|
||
@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)
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...
here is a screencapture video of the bug. (right click dialog is somehow missing in video) https://www.youtube.com/watch?v=G8MqZjNK1PI
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)
Comment 6•8 years ago
|
||
(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)
Comment 7•8 years ago
|
||
(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]
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...
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Comment 10•8 years ago
|
||
> > 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
Comment 11•8 years ago
|
||
(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
Comment 12•8 years ago
|
||
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 14•8 years ago
|
||
(In reply to Deepjyoti Mondal from comment #13) > @Honza I would like to work on this :) Excellent, assigned to you. Honza
Assignee: nobody → djmdeveloper060796
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
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-firefox55:
--- → fix-optional
Flags: needinfo?(djmdeveloper060796)
Comment 17•8 years ago
|
||
@Lis I will be uploading a patch soon. I was busy with some stuff. I will submit a patch ASAP
Updated•8 years ago
|
Flags: needinfo?(djmdeveloper060796)
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
(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•8 years ago
|
||
@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 22•8 years ago
|
||
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•8 years ago
|
||
@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)
Comment 24•8 years ago
|
||
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•8 years ago
|
||
@Honza I am also performing the same steps as you mentioned above but I am getting this page (screen shot).
Flags: needinfo?(odvarko)
Comment 26•8 years ago
|
||
Can you attach the crash report (see about:crashes) Honza
Flags: needinfo?(odvarko)
Comment 28•7 years ago
|
||
@Deepjyoti: can you still reproduce the crash? Honza
Flags: needinfo?(djmdeveloper060796)
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Blocks: netmonitor-edit-and-resend
Updated•6 years ago
|
Updated•6 years ago
|
Summary: Networkmonitor resended XHR requests dont show up anymore → Network monitor resent XHR requests don't show up anymore
Comment 31•6 years ago
|
||
@Mellina: any progress on this bug? Can I help somehow? Honza
Flags: needinfo?(pong7219)
Comment 32•6 years 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)
Comment 33•6 years ago
|
||
Unassigning Mellina (based on offline request) Honza
Assignee: pong7219 → nobody
Status: ASSIGNED → NEW
Comment 34•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(odvarko)
Comment 35•6 years ago
|
||
@Honza, I will drop out my request as I started working on another one :) Thank you!
Flags: needinfo?(odvarko)
Updated•6 years ago
|
Assignee: nobody → E0032242
Status: NEW → ASSIGNED
Assignee | ||
Comment 37•6 years ago
|
||
:Honza Patch is up and ready for review :D
Flags: needinfo?(odvarko)
Comment 38•6 years ago
|
||
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)
Comment 39•6 years ago
|
||
I am seeing some test failures, can you please investigate? Honza
Flags: needinfo?(djmdeveloper060796) → needinfo?(E0032242)
Assignee | ||
Comment 40•6 years 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)
Comment 41•6 years ago
|
||
(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)
Assignee | ||
Comment 42•6 years ago
|
||
Got it :) Latest try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=677e006b99779553f994938070dab7b9ef79abf8
Flags: needinfo?(odvarko)
Comment 43•6 years ago
|
||
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)
Comment 44•6 years ago
|
||
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•6 years 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)
Comment 46•6 years ago
|
||
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)
Comment 47•6 years ago
|
||
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.
Comment 48•6 years ago
|
||
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)
Assignee | ||
Comment 50•6 years ago
|
||
Updated the patch to use `lazyRequireGetter`. Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ae3f348845c478eddcc960a3486175ed2e65cba
Flags: needinfo?(odvarko)
Updated•6 years ago
|
Attachment #8861553 -
Attachment is obsolete: true
Flags: needinfo?(odvarko)
Comment 52•6 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/527ac367e301 Set 'request cause' according to the original request. r=Honza
![]() |
||
Comment 53•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/527ac367e301
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Updated•6 years ago
|
status-firefox63:
--- → wontfix
status-firefox64:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Flags: in-testsuite+
Comment 55•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•