Edit and Resend "Request headers" not working as expected, adding "no-cache" (Cache-Control / Pragma).

NEW
Unassigned

Status

defect
P3
normal
2 years ago
4 months ago

People

(Reporter: bruinsmaarten, Unassigned, Mentored)

Tracking

(Blocks 1 bug)

57 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

My Firefox version is: 57.0 (64-bit) up to date

In Firefox Developer Tools > Network > click row > Headers, you have "Edit and Resend".

Now for example, I'm gonna edit and resend the headers of:

https://www.google.nl/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png

To be sure press "CONTROL F5". Now I remove:

Cache-Control: no-cache
Pragma: no-cache

Now I press "Send". After this request I check the "Request headers" of it. Now the removed header fields and values (see above) are back.

Only in some cases the Host field is mandatory, so it's not that Firefox has to send "Cache-Control" and "Pragma".



Actual results:

The result is that the removed header fields / values are back.


Expected results:

I would expect that if I remove something, it's not coming back.
Component: Untriaged → Developer Tools: Netmonitor
I could reproduce the problem, needs investigation.

Thanks for the report!
Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3

Comment 2

2 years ago
I took a look at this. If you take a request, delete all the headers, I found firefox will add:

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.5
Cache-Control: no-cache
Connection: keep-alive
DNT: 1
Host: www.google.com
Pragma: no-cache
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

I looked through the React netmonitor code, and I don't think the problem is coming from that level. In the `request` actions, in the `sendCustomRequest` method, the correct headers are being passed to `connector.sendHTTPRequest`. So maybe the request is being modified by the connector further down the line? I've tried to trace this further in devtools-core to see where that request is actually being sent, but haven't found anything yet.
I could reproduce the issue and would like to work on this. I'm new to the community, so a little help of where to start would be appreciated.

Updated

Last year
Product: Firefox → DevTools

Comment 4

9 months ago
I would like to try my hand at this bug if that is alright.
Flags: needinfo?(odvarko)
(In reply to vihavira from comment #4)
> I would like to try my hand at this bug if that is alright.
Sounds good, but looking at this bug again. I am not sure if it's good-first-bug. According to the comment #2 it sounds like more complicated issue...

Some more analysis would be good to have though.
Perhaps you can do some analysis first and let me know if you still interested?

Honza
Flags: needinfo?(odvarko) → needinfo?(vihavira)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

9 months ago
Sorry for the delay, I was out of commission all last week. I am looking into this right now, and when I try to reproduce the bug am finding some minor differences.

I used the example webpage above:https://www.google.nl/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png


My headers are:
Host: www.google.nl
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://bugzilla.mozilla.org/
DNT: 1
Connection: keep-alive
Upgrade-Insecure-Requests: 1
If-Modified-Since: Thu, 08 Dec 2016 01:00:57 GMT
Cache-Control: max-age=0
TE: Trailers


As you can see Cache-Control is a little different. When I delete it I get both Pragma: no-cache and Cache-Control: no-cache(so I guess the issue is the same?)

I am wondering why they would both show up, isn't Pragma used for HTTP/1.0 caches, would this make it mutually exclusive?

Also should we continue discussing this here or is there another space or medium y'all would prefer(say, IRC or email?). Let me know.
Flags: needinfo?(vihavira) → needinfo?(odvarko)
@Michal: it looks like the platform is adding some default headers? Any chance for DevTools to disable such logic?

Honza
Flags: needinfo?(odvarko) → needinfo?(michal.novotny)
So the devtools code creates the HTTP channel, sets request method and request headers and then it calls asyncOpen(). There is no reason to expect that nsHttpChannel won't add or change any header. One possibility is add some method to nsHttpChannel that would freeze all headers. Other possibility is to try to alter the headers when "http-on-modify-request" notification is sent.
Flags: needinfo?(michal.novotny)

I was able to reproduce the error and would like to contribute to this issue, if i understood well,must have a default header that is placed whenever none value is put. I may be wrong, but I looked at some files, for example,the CustomHeaderPanel.js and found this snippet;

button({
        className: "devtools-button",
        id: "custom-request-send-button",
        onClick: sendCustomRequest,
    },
    CUSTOM_SEND,
),
...
 textarea({
       className: "tabpanel-summary-input",
       id: "custom-headers-value",
       onChange: (evt) =>
           this.updateCustomRequestFields(evt, request, updateRequest),
           rows: 8,
           value: headers,
           wrap: "off",
}),

After passing some debugging time, I noticed that the order ID we want to modify is obtained and the information in that request is "merge with the header we want to customize" before we click the Send button, the header is as it should, as it captures the changes on the keyboard, if we deleted the headers it changes state, but after clicking on the send button, it is modified, so I understood that the problem is precisely in the sendCustomRequest method. But as I'm still trying to understand the code and I'm new here, I need help

Hi,

To my understanding, the bug is caused by this line where we set LOAD_BYPASS_CACHE. Here if LOAD_BYPASS_CACHE is true, we set the headers related to caching. Not sure about what's the best way to fix this though.

Hi, As I am new here, and I do not have much knowledge about the code, I thought of adding a parameter with a default value in the sendHTTPRequest method, which would be a flag to set whether or not to use the cache

In this case, the default value would be True, and when sendCustomRequest invokes it, would pass false. But i'm not certain because a bitwise OR is made here:

    channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE |
                         Ci.nsIRequest.INHIBIT_CACHING |
                         Ci.nsIRequest.LOAD_ANONYMOUS;

so I understand that in case my default variable is none, the load flags would not receive any of these values

or change the flag to something that does not use a cache directly, I looked at this link: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICachingChannel

it seems for me that this flag LOAD_BYPASS_LOCAL_CACHE is the more indicated

In nsHttpChannel.cpp, I find where the cache header is added:

  // if doing a reload, force end-to-end
  if (mLoadFlags & LOAD_BYPASS_CACHE) {
    // We need to send 'Pragma:no-cache' to inhibit proxy caching even if
    // no proxy is configured since we might be talking with a transparent
    // proxy, i.e. one that operates at the network level.  See bug #14772.
    rv = mRequestHead.SetHeaderOnce(nsHttp::Pragma, "no-cache", true);
    MOZ_ASSERT(NS_SUCCEEDED(rv));
    // If we're configured to speak HTTP/1.1 then also send 'Cache-control:
    // no-cache'
    if (mRequestHead.Version() >= HttpVersion::v1_1) {
      rv = mRequestHead.SetHeaderOnce(nsHttp::Cache_Control, "no-cache", true);
      MOZ_ASSERT(NS_SUCCEEDED(rv));
    }
  } else if ((mLoadFlags & VALIDATE_ALWAYS) && !mCacheEntryIsWriteOnly) {
    // We need to send 'Cache-Control: max-age=0' to force each cache along
    // the path to the origin server to revalidate its own entry, if any,
    // with the next cache or server.  See bug #84847.
    //
    // If we're configured to speak HTTP/1.0 then just send 'Pragma: no-cache'
    if (mRequestHead.Version() >= HttpVersion::v1_1)
      rv = mRequestHead.SetHeaderOnce(nsHttp::Cache_Control, "max-age=0", true);
    else
      rv = mRequestHead.SetHeaderOnce(nsHttp::Pragma, "no-cache", true);
    MOZ_ASSERT(NS_SUCCEEDED(rv));
  }

Therefore, what Lequertier said is right, the cache is add since the LOAD_BYPASS_CACHE tag is enabled.

So, we can check whether the request is "custom", which means user uses devtools to resend, and remove the LOAD_BYPASS_CACHE tag in sendHTTPRequest method.

Before this change, resending custom request will automatically add "no-cache" headers. In this batch, I check for whether the request is custom or not, and disable "LOAD_BYPASS_CACHE" tag if the request is edited and resent by user.

Before this change, resending custom request will automatically add "no-cache" headers. In this batch, I check for whether the request is custom or not, and disable "LOAD_BYPASS_CACHE" tag if the request is edited and resent by user.

Attachment #9047631 - Attachment description: Summary: → Bug 1421342 - Fix a bug for adding no-chache to resent request headers. r=honza!

Hi @Laphets, i had tried a approach like this, as I commented here, but don't fix this problem, because the header is still changed in other parts of the code, for example, note that there's a else:

  else if ((mLoadFlags & VALIDATE_ALWAYS) && !mCacheEntryIsWriteOnly) {
    // We need to send 'Cache-Control: max-age=0' to force each cache along
    // the path to the origin server to revalidate its own entry, if any,
    // with the next cache or server.  See bug #84847.
    //
    // If we're configured to speak HTTP/1.0 then just send 'Pragma: no-cache'
    if (mRequestHead.Version() >= HttpVersion::v1_1)
      rv = mRequestHead.SetHeaderOnce(nsHttp::Cache_Control, "max-age=0", true);
    else
      rv = mRequestHead.SetHeaderOnce(nsHttp::Pragma, "no-cache", true);
    MOZ_ASSERT(NS_SUCCEEDED(rv));
  }

When i test just removing LOAD_BYPASS_CACHE, i still saw some headers.

I was waiting some feedback of mentors of my approach, to decide how to proceed

Hi @Fanny, thanks for your reply!
I do find a case that may fail: when the "Disable Cache" is enabled!

Yes, you're right, the tag VALIDATE_ALWAYS may also affect the "no-cache" header, but in definition of the tag, you can see:

    /**
     * The following flags control the frequency of cached content validation
     * when neither LOAD_BYPASS_CACHE or LOAD_FROM_CACHE are set.  By default,
     * cached content is automatically validated if necessary before reuse.
     * *************
     */
    const unsigned long VALIDATE_ALWAYS           = 1 << 11;
    const unsigned long VALIDATE_NEVER            = 1 << 12;
    const unsigned long VALIDATE_ONCE_PER_SESSION = 1 << 13;

So I'm also confused about whether should we also disable VALIDATE_ALWAYS, but in my opinion, only setting LOAD_BYPASS_CACHE will make it work, since VALIDATE_ALWAYS seems only control the frequency of cached content validation.
What do you think about it?

I'm now working on to solve the issue that when "Disable Cache" is enabled, the "no-cahche" header still added.

I find the reason why the "Disable Cache" option is enabled, the "no-cache" will be added regardless disable LOAD_BYPASS_CACHE.

First, this line: https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#1095 , will set LOAD_BYPASS_CACHE for defaultLoadFlags. And here: https://searchfox.org/mozilla-central/source/docshell/base/nsIDocShell.idl#362 , it said that defaultLoadFlags will be added for each request automatically, which means even we manually disable LOAD_BYPASS_CACHE, the tag will still be added to the request by some c++ code when we enable choice "Disable Cache".

So I think it's due to some design issue -- the user's custom sent request do not have enough permission to set all the things as it wants. @honza What do you think about this?

But our solution do fix the issue when the "Disable Cache" option is not enabled @Fanny.

In my opinion, the workflow works like this:

  1. if user click "Disable Cache" checkbox => set LOAD_BYPASS_CACHE to defaultLoadFlags in docshell

  2. user click resend request

  3. (by default)(and we can make change here) Set LOAD_BYPASS_CACHE Tag

  4. (From here, it's hard for us to change the behavior) Add defaultLoadFlags

  5. if LOAD_BYPASS_CACHE is set, then add "no-cache" header

  6. network i/o

And my batch fixes 2。

(In reply to Laphets [:Laphets] from comment #19)

I find the reason why the "Disable Cache" option is enabled, the "no-cache" will be added regardless disable LOAD_BYPASS_CACHE.

First, this line: https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#1095 , will set LOAD_BYPASS_CACHE for defaultLoadFlags. And here: https://searchfox.org/mozilla-central/source/docshell/base/nsIDocShell.idl#362 , it said that defaultLoadFlags will be added for each request automatically, which means even we manually disable LOAD_BYPASS_CACHE, the tag will still be added to the request by some c++ code when we enable choice "Disable Cache".

So I think it's due to some design issue -- the user's custom sent request do not have enough permission to set all the things as it wants. @honza What do you think about this?

But our solution do fix the issue when the "Disable Cache" option is not enabled @Fanny.

Hi @Laphets,

First of all, i think that we should search more informations with the core team, because i believe that as header is put for default in all requests, there’s a good reason, and if we change this, we can break some code. @honza must be more information.

That said, we can have two scenarios:

1 - this is not designed to suffer changes. In this case, i think in add a disclaimer on the frame, explaining that the cache headers are add for default

2 - this is passible of alteration. Here, we can adopt the same approach of the flags that you implement.

(In reply to Fanny Batista Vieira [:fanny] [:fanny] from comment #21)

First of all, i think that we should search more informations with the core team, because i believe that as header is put for default in all requests, there’s a good reason, and if we change this, we can break some code. @honza must be more information.

Also, the patch may break some tests. Mochitests are used to test the netmonitor behaviors. Please see here for more information about the mochitests and how to run them locally. For example, this one and this one might need to be updated.

Vincent

(In reply to Fanny Batista Vieira [:fanny] [:fanny] from comment #21)

First of all, i think that we should search more informations with the core team, because i believe that as header is put for default in all requests, there’s a good reason, and if we change this, we can break some code.

Absolutely agree with this. According to the analysis, this should be synced with the Networking team since headers are appeded automatically by the (Firefox Necko) platform. I am also removing the good-first-bug, since this doesn't seem to be that easy.
But anyone interested can continue analysis on this.

Thanks!

Honza

Keywords: good-first-bug

I think the right flag may be LOAD_BYPASS_LOCAL_CACHE instead. LOAD_BYPASS_CACHE is meant to bypass also any transparent and intermediate caches on the path to the end server. But the use case for devtools is probably to only behave as the browser has never seen the response before.

Having two options (bypass local cache, bypass all caches) in dev tools seems like an overkill.

It would be good to dig into the source history for why LOAD_BYPASS_CACHE was used in the first place, maybe there is a reason.

Comment 25

4 months ago

Hey! I am an Outreachy applicant. It would be great if I get to contribute here. Please assign me this issue.
Thank you,
Dhruvi

(In reply to Dhruvi Butti from comment #25)

Hey! I am an Outreachy applicant. It would be great if I get to contribute here. Please assign me this issue.

Before I assign this to you, do you have any specific plans how to fix this bug?
(I am asking since this bug doesn't feel like good-first-bug)

Honza

Flags: needinfo?(dhruvibutti9477)

Comment 27

4 months ago

Hey! I am new to this but after digging into this I found that the filtering of headers in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_headers_filter.js might help here as the caches filtering after resending the header creates issues. And as mentioned above the sorting of the headers here: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_headers_sorted.js is also creating a problem as we are statically defining the expected request and response headers. As of now, I think altering expected headers might solve the problem. I might not be correct but by discussing and taking your inputs I might be able to solve this.
Thank you,
Dhruvi

Flags: needinfo?(dhruvibutti9477)
You need to log in before you can comment on or make changes to this bug.