1340100 - Network monitor resent XHR requests don't show up anymore

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[office]

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..

Comment 1

[Loic]

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?

Comment 2

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

@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

Comment 3

[office]

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...

Comment 4

[office]

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

[Loic]

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.

Comment 6

[Jarda Snajdr [:jsnajdr]]

(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.

Comment 7

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

(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

Comment 8

[office]

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...

Comment 9

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix.

Comment 10

[[e-mail]]

> > 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

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

(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

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

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

Comment 13

[Deepjyoti Mondal]

@Honza I would like to work on this :)

Comment 14

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

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

Excellent, assigned to you.

Honza

Comment 15

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

[Deepjyoti Mondal]

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

Comment 18

[Deepjyoti Mondal]

Attachment: 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

Comment 19

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

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.

Comment 20

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

(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

[Deepjyoti Mondal]

Attachment: 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)?

Comment 22

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

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

Comment 23

[Deepjyoti Mondal]

Attachment: 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

Comment 24

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

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

Comment 25

[Deepjyoti Mondal]

Attachment: crash.png

@Honza I am also performing the same steps as you mentioned above but I am getting this page (screen shot).

Comment 26

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

Can you attach the crash report (see about:crashes)

Honza

Comment 27

[office]

Hi guys, any updates on this bug?

Comment 28

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

@Deepjyoti: can you still reproduce the crash?

Honza

Comment 29

[Mellina Y.]

Hi!
Can I work on this issue?
Thanks!

Comment 30

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

Assigned to you!
Honza

Comment 31

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

@Mellina: any progress on this bug? Can I help somehow?

Honza

Comment 32

[Mellina Y.]

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.

Comment 33

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

Unassigning Mellina (based on offline request)

Honza

Comment 34

[Ioannis Valasakis [:wizofe]]

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

Comment 35

[Ioannis Valasakis [:wizofe]]

@Honza, I will drop out my request as I started working on another one :) Thank you!

Comment 36

[Hengyeow (:tanhengyeow)]

Attachment: Bug 1340100 - Set 'request cause' according to the original request. r=Honza

Set 'request cause' according to the original request.

Comment 37

[Hengyeow (:tanhengyeow)]

:Honza

Patch is up and ready for review :D

Comment 38

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

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

Comment 39

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

I am seeing some test failures, can you please investigate?

Honza

Comment 40

[Hengyeow (:tanhengyeow)]

Resolved the merge conflict.

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

Everything's positive :)

Comment 41

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

(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

Comment 42

[Hengyeow (:tanhengyeow)]

Got it :)

Latest try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=677e006b99779553f994938070dab7b9ef79abf8

Comment 43

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

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

Comment 44

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

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

Comment 45

[Hengyeow (:tanhengyeow)]

: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?

Comment 46

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

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

Comment 47

[Julian Descottes [: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.

Comment 48

[Julian Descottes [:jdescottes]]

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.

Comment 49

[Julian Descottes [:jdescottes]]

Opened Bug 1500461 to investigate the root issue.

Comment 50

[Hengyeow (:tanhengyeow)]

Updated the patch to use `lazyRequireGetter`. 

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ae3f348845c478eddcc960a3486175ed2e65cba

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/527ac367e301
Set 'request cause' according to the original request. r=Honza

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/527ac367e301

Comment 55

[Adrian Florinescu [:aflorinescu]]

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.