User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0 Build ID: 20160502172042 Steps to reproduce: - Execute some crossdomain and GET XHR (it will execute an OPTION request followed by a GET request) - Go to the debugger and try to replay the same XHR Actual results: The OPTION XHR actually returns 200, so that's pretty good, i'm allowed to do some great stuffs.. but, the GET request NEVER launch... Expected results: The options returns a 200, then the GET will be launched and executed as the first GET xhr was....

So, i've inspected further more, and this wild bug appears from version 42.

Could you provide a live testcase showing easily the issue?

Go to https://app.expernova.com, then open the network debbuger and try to replay the GET XHR https://api.expernova.com/presets then only the OPTIONS is re-triggered..

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3de175bd4e83775e35077d71167967fa457d0e0a&tochange=10343dcbe03d370fd57a261c6833c9928956bb33 Probably: Michael Layzell — Bug 1200337 - Part 1: Don't expose standard HTTP headers during interception in non-e10s mode, r=mcmanus or Michael Layzell — Bug 1188932 - Allow the User-Agent header to be explicitly set by requests, r=bkelly, r=jgraham

I believe that this patch should fix the problem, and from my testing it appears to. Unfortunately, I don't know how to test interactions with the devtools UI, so I'm going to need some help to write the tests for this patch. vporof, AFAICT you reviewed the original patch for adding replay support, so I figure you might know how to write these tests :).

Michael, why exactly is the GET request not send and why does setting only the custom headers help? When I try to test this without the patch, the only difference between the first and second OPTIONS request is that the Access-Control-Request-Headers header has an additional "user-agent" field in the second one. I'll help you writing the test. It will be based on the existing browser_net_resend.js one. There's another CORS-related bug in Network Monitor, bug 1214752. It should get fixed and landed before this one, otherwise we'll have to distinguish between e10s and non-e10s cases in the test.

(In reply to Jarda Snajdr [:jsnajdr] from comment #6) > Michael, why exactly is the GET request not send and why does setting only > the custom headers help? > > When I try to test this without the patch, the only difference between the > first and second OPTIONS request is that the Access-Control-Request-Headers > header has an additional "user-agent" field in the second one. Yup, that's the difference. The user-agent field is not a "standard" one, so if it is set by content explicitly, then a cors preflight is required to confirm that the request is approved. This preflight will have the extra "user-agent" header included, meaning that the server may reject it due to not listing user-agent in the list of approved headers. This patch avoids setting the user-agent field explicitly when replaying the request if it was not set explicitly in the first request, meaning that the preflight won't request the user-agent field. > I'll help you writing the test. It will be based on the existing > browser_net_resend.js one. > > There's another CORS-related bug in Network Monitor, bug 1214752. It should > get fixed and landed before this one, otherwise we'll have to distinguish > between e10s and non-e10s cases in the test. Sounds good.

This is a test for the "Replay CORS" feature. It adds two things: 1. A "browser_net_resend_cors.js" testcase that sends a CORS POST request and then resends it without any change. Checks that an OPTIONS+POST combo is displayed in netmonitor two times. 2. In existing "browser_net_resend.js" case, I edit the "User-Agent" header of the replayed request and check if the changed header was really sent. Note that you need to run this test with --disable-e10s, because it doesn't work in e10s because of bug 1091612. This patch is intended to be applied on top of the test from bug 1214752. Running the tests reveals several problems with your patch: 1. When I resend the POST request in "browser_net_resend_cors.js", the headers "Cache-Control" and "Pragma", although added by the browser, are visited by visitNonDefaultRequestHeaders, detected as custom, and added to the "Access-Control-Request-Headers". That makes the replayed request different from the original one. 2. After I edit the headers in the netmonitor UI, they are parsed and set at [1]. At this moment, all the "custom" attributes are lost. 3. When I edit the "User-Agent" header in the netmonitor UI, I expect the new value to be sent in the replayed request. However, this header would be detected as non-custom and ignored when setting the replayed request headers. Maybe the right solution is to detect which headers were edited by the users and which were left intact, and call setRequestHeader only on those that were explicitly edited. Setting them all blindly, even if the value is not changed, seems to add them to "Access-Control-Request-Headers" as an unwanted side-effect. [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-view.js#2644

This is not a recent regression and given that there is no fix in the works, I think it is too late for Fx47.

Tracking for 48/49, seems likely we can fix this regression at least in 49.

This seems like it will require some changes to the way that devools works (namely, we need to expose some sort of option in the text view stating that a particular row should be ignored when the request is replayed). I don't know very much about how devtools works, and don't know what this should look like. Is there any chance you could find someone more qualified than me to take over the rest of this bug?

Yes, I can take it. It will most likely require tweaking the Netmonitor UI - to detect which headers were really changed, which should be modified on the new request and which should be left as defaults.

Since this is not a recent regression, I wonder if it's necessary for FF48 or if we want to accept a patch for FF48, in consideration of the release schedule. May I know your opinion, Michael? Thanks!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13) > Since this is not a recent regression, I wonder if it's necessary for FF48 > or if we want to accept a patch for FF48, in consideration of the release > schedule. May I know your opinion, Michael? Thanks! I don't think it would make sense to block FF48 on this patch, as the regression isn't recent or seriously affecting most of our users.

(In reply to Michael Layzell [:mystor] from comment #14) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #13) > > Since this is not a recent regression, I wonder if it's necessary for FF48 > > or if we want to accept a patch for FF48, in consideration of the release > > schedule. May I know your opinion, Michael? Thanks! > > I don't think it would make sense to block FF48 on this patch, as the > regression isn't recent or seriously affecting most of our users. Thanks for the comment, based on which I change "status-firefox48" to wontfix. Please feel free to change the status if you have different opinions.

I spent some time analyzing this bug and the conclusion is: if we want to fix it, we must define first what exactly it means to "resend a request". If it's a CORS request that triggered a preflight, there are two choices: 1. Replay both the preflight and flight requests. In case I didn't do any edit (headers or body) in the Netmonitor UI, it should behave exactly the same as the original requests. The preflight should have the same Access-Control-Request-* headers etc. 2. Replay only the flight request, don't send the preflight. It turns out that option 1 is impossible to implement correctly. It's because the list of headers in Access-Control-Request-Headers is determined not by their presence and content, but by HOW they were set. If they were set by the browser, they are safe and don't trigger a preflight. However, if they were set by the user (using the XHR or Fetch API), they are considered unsafe and are included in the A-C-R-H header of the preflight. That means that even knowing the exact headers of the original request, I'm unable to figure out which were set by the user and which were set by the browser. Therefore, I cannot replicate exactly the preflight request. So, only option 2 is possible to implement. Alex, what do you think? Is there any other person who could contribute to this problem?

I'm not dogfooding this panel much, and I'm all but an expert on CORS requests, so I'm not sure my advice is really valuable... But at first sight, given that we list both requests in the panel, it seems sensible to replay them individually. May be someone from necko would have more useful input about this? Also, looking at chrome, I don't see any OPTIONS requests. But while looking at https://app.expernova.com/, I do see duplicated requests made to /presets?? Thanks again for polishing the network monitor!!

Jason, wondering if you know what the next move is here?

Sadly I don't know CORS or the XHR code very well :(

The code that replays the selected HTTP request needs to satisfy the following requirements: - don't trigger a preflight request, i.e., disable the CORS security checks. The CORS preflight mechanism distinguishes between headers that were added or changed by the page author and these that were added by the browser. This makes sense for XHR requests made by the web pages, but not so much for the devtools replay feature. - allow to set headers that are otherwise considered unsafe for web pages. Like Host, Referer. Complete list is at [1] - the request should have loadingNode and loadGroup set to the window/document being inspected. Otherwise, it won't appear in the Netmonitor - the request shouldn't interact with cache - don't use cached responses, don't store received response to cache - the request should be anonymous - don't add cookies or Authorization header unless explicitly specified in the replay UI It turns out that XMLHttpRequest is not up to the task (not even with the mozSystem flag), so I rewrote the sending code to use a nsIHttpChannel directly, setting all the needed flags and properties on the channel. Added tests that check that no CORS preflight is sent and that the headers are OK. Should fix almost all the dependent bug, except bug 1124685 - when privacy.donottrackheader.enabled is true, the DNT header is added no matter what, without any way to opt-out.

Comment on attachment 8787638 [details] Bug 1270096 - Netmonitor request replay: disable CORS checks, set request headers accurately https://reviewboard.mozilla.org/r/76340/#review75890 Nice work! I made some minor comments, but all look good to me, R+ assuming Try is green. - tests pass for me localy - also works for me on simple web app. One additional thing I noticed in the UI is the 'cause' column value set to 'other'. It would be nice to display 'resent' or something similar. Honza ::: devtools/client/netmonitor/test/browser_net_resend_cors.js:43 (Diff revision 2) > + > + info("Waiting for tab update"); > + yield monitor.panelWin.once(EVENTS.TAB_UPDATED); > + > + info("Cloning the selected request into a custom clone"); > + RequestsMenu.cloneSelectedRequest(); Why cloneSelectedRequest() call is needed? As as user, I do just one action (edit & resend) from the context menu. Shouldn't the test mimic that? ::: devtools/client/netmonitor/test/browser_net_resend_cors.js:62 (Diff revision 2) > + is(item.status, 200, "The response has the right status"); > + // eslint-disable-next-line mozilla/no-cpows-in-tests > + is(item.responseContent.content.text, "Access-Control-Allow-Origin: *", > + "The response has the right content"); > + > + info("Finishing the test"); It's also possible to resend the(OPTIONS) Preflight request. Shouldn't the test also cover that? ::: devtools/client/netmonitor/test/browser_net_resend_headers.js:24 (Diff revision 2) > + let requestUrl = SIMPLE_SJS; > + let requestHeaders = [ > + { name: "Host", value: "fakehost.example.com" }, > + { name: "User-Agent", value: "Testzilla" }, > + { name: "Referer", value: "http://example.com/referrer" }, > + { name: "Accept", value: "application/jarda"}, Nice content type :)

(In reply to Jan Honza Odvarko [:Honza] from comment #23) > One additional thing I noticed in the UI is the 'cause' column value set to > 'other'. > It would be nice to display 'resent' or something similar. Yeah, I'm thinking about adding a TYPE_DEVTOOLS value to nsIContentPolicyBase.idl. Bug 886233 (display source map requests in Netmonitor) also involves sending network requests on behalf of devtools, so it could be added while fixing that bug. > > + RequestsMenu.cloneSelectedRequest(); > > Why cloneSelectedRequest() call is needed? As as user, I do just one action > (edit & resend) from the context menu. Shouldn't the test mimic that? Even as user, you do two actions: 1) Click on "Edit and Resend" - that shows the request editor UI 2) Click on "Send" - send the request after you're finished editing RequestsMenu.cloneSelectedRequest() does the first step. > It's also possible to resend the(OPTIONS) Preflight request. Shouldn't the > test also cover that? Good point, test amended. > > + { name: "Accept", value: "application/jarda"}, > > Nice content type :) Thanks :) I'm testing that the header was really set, and not overriden by some default. So I need header values that are unique enough :)

Pushed by jsnajdr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f9c16c7e72ef Netmonitor request replay: disable CORS checks, set request headers accurately r=Honza

Hello Jarda, do you think it's worthwhile uplifting this patch to Beta50? I noticed it since it was marked as a P1 regression.

Hi Jeremie, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

(In reply to Ritu Kothari (:ritu) from comment #28) > Hello Jarda, do you think it's worthwhile uplifting this patch to Beta50? I > noticed it since it was marked as a P1 regression. I think it's not worth it for two reasons: - it's not really a regression. Yes, the STR started happening only after bug 1188932 (support setting custom User-Agent in XHR) landed, but that just transformed one buggy behavior (User-Agent wasn't set at all) into a different buggy behavior (setting User-Agent triggers unwanted preflight request). The actual bug is a few years old - see the "Blocks" list to see all the duplicate reports dating back to 2013. - backporting the tests would be nontrivial, as this bug got landed after a heavy refactoring of Netmonitor mochitests in bug 1297362.

Given that this is not a new regression and comment 30, we will let this one ride the 51 train.

Thank's mates! It appears solved - FF 51.0.1, i don't see anymore "Options" and XHR resending is fully functional!