Closed Bug 1583397 Opened 1 year ago Closed 9 months ago

Resend request forgets headers

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox75 verified)

VERIFIED FIXED
Firefox 75
Tracking Status
firefox75 --- verified

People

(Reporter: Harald, Assigned: thiago.arrais)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(5 files, 1 obsolete file)

What were you doing?

  1. Run in console fetch('https://api.github.com/search/topics?q=firefox', {headers: {Accept: 'application/vnd.github.mercy-preview+json'}}) with XHR filter enabled
  2. Resend request on XHR entry

What happened?

Request fails because Accept header is missing.

What should have happened?

Accept header is carried over.

Would this be a good first bug, Honza?

Flags: needinfo?(odvarko)

Thanks Harald for the report!

Some instructions:

  1. The client side calls this method to ask the backend to resend specific request
    https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/devtools/client/netmonitor/src/connector/firefox-connector.js#287

  2. This is the backend piece that does the actual resending
    https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/devtools/server/actors/webconsole.js#1834

We need to check both places for the missing headers and see whether this is a problem with DevTools code or the platform

Honza

Flags: needinfo?(odvarko)
Keywords: good-first-bug

Hi Jan/Herald! I'd like to take a look at this and see if I can fix this. This would be my first contribution here.

(In reply to Shobhit Chittora from comment #3)

Hi Jan/Herald! I'd like to take a look at this and see if I can fix this. This would be my first contribution here.

OK, sounds great!

There are some instructions in comment #2 so, that should help you to know where to start.
Also you might want to read:
https://docs.firefox-dev.tools/getting-started/

Please post any of your findings here in the report.
Also use Request information from field (at the bottom of this page) when you have questions.

Honza

Hi, i couldn't able to reproduce this issue any more. Is this still exist?

Attached image image.png

Yes, I can still reproduce the issue. See my screenshot

  • the first request is 200
  • the second one (resent) is 415 - Unsupported Media Type

Honza

hmm. Please find the attached screenshots. Am i missing something. I am using firefox 69 version with MacOS Sierra.


@chittorashobhit@gmail.com: can you reproduce the issue?

Honza

Flags: needinfo?(chittorashobhit)

@chittorashobhit@gmail.com: can you reproduce the issue?

Couldn't reproduce this on the first try. Steps I tried

  1. In new tab made an XHR in the console.
  2. Went to network tab to resend the same.

System detail -
OS - MacOS mojave 10.14
Firefox - 69.0.1

Flags: needinfo?(chittorashobhit)

Adding my result of trying to reproduce this.

@chittorashobhit@gmail.com: can you reproduce the issue?

Do you have any other browser specific setting enabled which might lead to this?

(In reply to Shobhit Chittora from comment #9)

Firefox - 69.0.1

Can you please try Firefox Nightly? Fx71
https://www.mozilla.org/en-US/firefox/nightly/all/

Honza

Flags: needinfo?(chittorashobhit)

Diff of the headers in requests.

Flags: needinfo?(chittorashobhit)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #12)

(In reply to Shobhit Chittora from comment #9)

Firefox - 69.0.1

Can you please try Firefox Nightly? Fx71
https://www.mozilla.org/en-US/firefox/nightly/all/

Honza

Hi Honza! My bad! I was trying something else. I can totally reproduce this in Fx71.

Hi Honza Odvarko [:Honza],

I've tried looking at the root cause of this. I peeked in the file below ->
request .js -> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/actions/requests.js#93

I tried to resend a request, which ( as per my investigation ) is done using the the action below ->
api.js -> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/api.js#231

This looks for a requestID, which is our first request. It then looks for the requestID in the redux store to using the selector below ->
requests.js -> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/selectors/requests.js#164

On inspecting this retrieved request from the store, I don't see the property we're looking for i.e. request.requestHeaders.headers

Also attaching below a sample request retrieved from the store.

{
  "id": "server0.conn0.netEvent4",
  "startedMs": 1569543578970,
  "method": "GET",
  "url": "https://api.github.com/search/topics?q=firefox",
  "isXHR": true,
  "cause": {
    "type": "fetch",
    "loadingDocumentUri": "about:home",
    "stacktraceAvailable": true
  },
  "stacktrace": "(void 0)",
  "fromCache": "(void 0)",
  "fromServiceWorker": "(void 0)",
  "isThirdPartyTrackingResource": false,
  "referrerPolicy": "no-referrer-when-downgrade",
  "blockedReason": "(void 0)",
  "channelId": 342442037477378,
  "urlDetails": {
    "baseNameWithQuery": "topics?q=firefox",
    "host": "api.github.com",
    "scheme": "https",
    "unicodeUrl": "https://api.github.com/search/topics?q=firefox",
    "isLocal": null,
    "url": "https://api.github.com/search/topics?q=firefox"
  },
  "requestHeadersAvailable": true,
  "requestCookiesAvailable": true,
  "httpVersion": "HTTP/1.1",
  "remoteAddress": "13.234.168.60",
  "remotePort": 443,
  "status": "200",
  "statusText": "OK",
  "headersSize": 1109,
  "securityState": "secure",
  "isRacing": "(void 0)",
  "securityInfoAvailable": true,
  "responseHeadersAvailable": true,
  "responseCookiesAvailable": true,
  "totalTime": 429,
  "eventTimingsAvailable": true,
  "contentSize": 12690,
  "transferredSize": 3501,
  "mimeType": "application/json; charset=utf-8",
  "responseContentAvailable": true
}

Hope this is the right direction to keep looking for this? Thanks for any and all help.

Also I see a lot of (void 0) in the request object. Why is that so? Are these values coming from the c++ objects or something like that?

Attached patch temp.diffSplinter Review

(In reply to Shobhit Chittora from comment #15)

On inspecting this retrieved request from the store, I don't see the property we're looking for i.e. request.requestHeaders.headers

Ah, that's the problem. Thanks for analysis!

The issue is that we are fetching headers on demand i.e. when the user actually want to see them in the Headers side panel. It's fetched here:
https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/devtools/client/netmonitor/src/components/HeadersPanel.js#115

When cloning the request we need to do the same (even if the Headers panel wasn't opened yet)
Also, when cloning the request we need to also clone the requestHeadersAvailable field that indicates that headers are ready to be fetched from the backed.

Please see my patch, understand, add comments if you find them useful and upload to Phab

(In reply to Shobhit Chittora from comment #16)

Also I see a lot of (void 0) in the request object. Why is that so? Are these values coming from the c++ objects or something like that?

Some field can be undefined, that's ok.

This also needs a test.

The test can be very similar to what we have
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_resend_xhr.js#7

But, the test should not click on the original request (the one being resent)
https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/devtools/client/netmonitor/test/browser_net_resend_xhr.js#22

Clicking causes the Headers panel to open and fetch the Headers automatically

You can extend the request and do another resend without clicking on the original request and checking that expected header is there.

Honza

Assignee: nobody → chittorashobhit
Status: NEW → ASSIGNED
Has STR: --- → yes

Thanks Honza!
Understood the changes in the patch you've posted. Let me check the changes once more and write test around it. Thanks for this.

Hey Honza! Why is a reload / re-select of the request required in your patch.

// Reload the request from the store to get the headers.
    request = getRequestById(getState(), request.id);

(In reply to Shobhit Chittora from comment #19)

Hey Honza! Why is a reload / re-select of the request required in your patch.

// Reload the request from the store to get the headers.
    request = getRequestById(getState(), request.id);

This is because we use immutable Redux store. That means the the request variable (referencing the entire request object) is not modified and still points to (immutable) data/request. So, we need to get the request from the store again (using getRequestById) to get new reference pointing to the new data (which contains the headers)

You can read about Redux immutability here:
https://redux.js.org/faq/immutable-data

Honza

Also Honza! The test you pointed at is failing after our patch due to the below code awaits on fetching the headers.

   // Fetch request headers and post data from the backend.
   await fetchNetworkUpdatePacket(connector.requestData, request, [
     "requestHeaders",
     "requestPostData",
   ]);

devtools/client/netmonitor/test/browser_net_resend_xhr.js
  FAIL A promise chain failed to handle a rejection: Connection closed, pending request to server0.conn0.child1/consoleActor2, type sendHTTPRequest failed

Thanks for the update. Can you please look at the failure and fix that test?
Honza

Flags: needinfo?(chittorashobhit)

Unassigning the bug for now as it looks like Shobhit isn't working on it anymore. Please feel free to claim it again should you resume work on this.

Assignee: chittorashobhit → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(chittorashobhit)
Assignee: nobody → thiago.arrais
Status: NEW → ASSIGNED

Just Note about the STR from comment #0. It needs to be done in the Console panel.

If you open the Details sidebar for the original request (the Network panel) or if you expand the request in the Console before resending - it'll work.

Honza

Depends on D61381

Attachment #9123870 - Attachment description: Bug 1583397 - Part 1/: Ensures headers are fetched before trying to resend request. r=Honza → Bug 1583397 - Ensures headers are fetched before trying to resend request. r=Honza
Attachment #9125573 - Attachment is obsolete: true
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44c99468c2fc
Ensures headers are fetched before trying to resend request. r=Honza

Thiago, this test seems to be failing
devtools/client/netmonitor/test/browser_net_resend_cors.js

Honza

Duplicate of this bug: 1606548

Yup. I am aware of that. But I unfortunately can't find a way to fix it. Any ideas?

By the way, I had pointed that out in https://phabricator.services.mozilla.com/D61381#1905363 . But that didn't block the review. What should I have done differently?

Flags: needinfo?(thiago.arrais) → needinfo?(odvarko)

I've managed to fix browser_net_resend_cors.js. It is no longer failing, but now I'm not really sure it tests what it should. Please review accordingly.

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ed20f401a70
Ensures headers are fetched before trying to resend request. r=Honza
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
Flags: needinfo?(odvarko)

Confirmed the issue and note on comment 25 using Firefox 72.0 on Windows 10.
Verified with 75.0a1 (2020-02-24) Windows 10, macOS 10.13, Kubuntu 19.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.