Closed Bug 757042 Opened 12 years ago Closed 11 years ago

XMLHttpRequest: POST always inserts cache-related headers (even when duplicate, shouldn't anyway)

Categories

(Core :: Networking: HTTP, defect)

x86
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED DUPLICATE of bug 801826

People

(Reporter: helder.magalhaes, Unassigned)

References

Details

(Keywords: perf)

I've recently bumped into an unexpected issue where XHR-based requests were being sent with duplicate cache-related headers:
  Pragma: no-cache, no-cache
  Cache-Control: no-cache, no-cache

Testing with both Internet Explorer 9 and Chrome 19 confirmed that this wasn't an issue in the framework currently being used, and I was able to reproduce it with a simple code snippet:
  var xhr = new XMLHttpRequest();
  xhr.open('POST', '/serverRelativeAddress/serverHandler');
  xhr.setRequestHeader('Pragma', 'no-cache');
  xhr.setRequestHeader('Cache-Control', 'no-cache');
  xhr.send('dummyContent);

I've later narrowed the issue to POST requests only (GET and HEAD don't trigger the issue, but probably only due to not inserting them by default).

From experiments, what seems to be happening is that Gecko always appends the "no-cache" headers independently from author request headers. (The "appends" part was inferred by setting a different header value and observing headers sent to server: the "no-cache" keeps being appended last.)

This leads to several issues:
  1. Useless overhead (20 bytes) in every client-->server interaction
     (performance hit on AJAX applications, therefore the "perf" keyword)
  2. Gecko not conforming to the XHR v2 specification regarding headers:
     «It must not send Cache-Control or Pragma request headers
     automatically unless the end user explicitly requests such behavior
     (e.g. by reloading the page).» [1]
  3. The unexpected duplicate header values may confuse some servers
     (especially "Pragma", which typically only expects "no-cache" [2])

[1] http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-send
[2] http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.32
If the no cache headers/header values are being used for historical and/or compatibility reasons, then I'd proposed that, at least, the author request headers were checked in order to avoid duplicates.
Forgot to mention a few interesting nits found during the experiments that may help saving some time and/or help towards a solution:
  * I've noticed that Firebug Console filters the duplicate values in headers
    (the effect can only be observed in Net panel and/or in the native Web Console)
  * Checked with both release (currently 12.0) and nightly build (currently 15.0a1)
  * I've disabled Firebug but the issue kept appearing
    (so apparently there's no relationship between the add-on and this issue)
  * A possible workaround can be not to insert the author request headers for Gecko
    (but a regression range is first needed in order to implement this)
For a POST, the DOM XHR code sets the LOAD_BYPASS_CACHE flag on the channel.

When that flag is set, nsHttpChannel::SetupTransaction does:

572         // We need to send 'Pragma:no-cache' to inhibit proxy caching even if
573         // no proxy is configured since we might be talking with a transparent
574         // proxy, i.e. one that operates at the network level.  See bug #14772.
575         mRequestHead.SetHeader(nsHttp::Pragma, NS_LITERAL_CSTRING("no-cache"), true);
576         // If we're configured to speak HTTP/1.1 then also send 'Cache-control:
577         // no-cache'
578         if (mRequestHead.Version() >= NS_HTTP_VERSION_1_1)
579             mRequestHead.SetHeader(nsHttp::Cache_Control, NS_LITERAL_CSTRING("no-cache"), true);

The "true" there is for "append, instead of replacing".

What XHR really wants here is not to affect the _request_, but the handling of the _response_: we don't want Necko to even bother caching the response, since such a cache entry will never be hit.  But I'm not sure Necko exposes an API for that other than LOAD_BYPASS_CACHE...
Component: Networking → Networking: HTTP
QA Contact: networking → networking.http
(In reply to Boris Zbarsky (:bz) from comment #3)
> For a POST, the DOM XHR code sets the LOAD_BYPASS_CACHE flag on the channel.

Thanks for heads up, bz! :-)

I crawled through the code a bit and was about to propose very specific changes in 'nsHttpChannel::SetupTransaction', which basically checked for the header value (using 'HasHeaderValue') before setting it, but then I realized that it could be made more generically (and potentially more useful by avoiding other similar issues such as this one) if this was made directly in 'nsHttpHeaderArray::SetHeader'.

WDYT? Are there any legitimate use-cases where one may want to set duplicate HTTP header values?
"Content-Encoding: gzip, gzip" can be set legitimately, though it's a bit silly to do that.

There might be others...

The right fix here is really to not mess with the headers at all in the XHR case.
(In reply to Helder "Lthere" Magalhães from comment #2)
> * A possible workaround can be not to insert the author request headers for Gecko
>     (but a regression range is first needed in order to implement this)

A quick research showed that this issue is there since the initial Hg import:
http://hg.mozilla.org/mozilla-central/annotate/2552b0541f87/content/base/src/nsXMLHttpRequest.cpp#l3051

A bit of crawling through the CVS interface hinted towards this commit:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&file=nsXMLHttpRequest.cpp&rev2=1.141&rev1=1.140

Apparently, a fix made for bug 309424 apparently introduced a partial regression: as far as I can see, previously we only set the no-cache flags if not there already (suggestion I was giving in comment #4), whereas now we force adding those flags (even when that means duplicating them).

This means that the regression range is something like "since Firefox 1.5", according to the commit message:
  1.141 <darin@meer.net> 2006-04-19 20:37
  fixes bug 309424 "mozilla 1.5beta1 freezes & goes into 95+% cpu usage browsing blackisha.com" r=biesi sr=bzbarsky

Setting a "dependency" on that bug, although I'm not sure if formally that makes sense...


(In reply to Boris Zbarsky (:bz) from comment #5)
> The right fix here is really to not mess with the headers at all in the XHR
> case.

Agreed, this is the right thing to do (as per the XHR v2 spec. stated in comment #0).
Depends on: 309424
I was trying to develop a JavaScript-based workaround to this issue, in order to avoid the useless overhead (20 bytes, referred in comment #0) for AJAX-intensive applications as, even when fixed, this issue will be around for a while.

The problem is that there is currently no target milestone set, so one can't write something like "if gecko above 1.8 or below 16 apply workaround". Given that nightly builds just flipped version to 16, trying to aim to anything less than that seems unreasonable, since the proper fix for this will likely involve API changes. So, can a target milestone be set, please?

In the meantime, given the potential impact, might I still insist towards the two step fix:
1. Avoiding duplicating cache headers (target milestone 16, for example)
   (solves points 1 - when cache headers are set - and 3 of the original report)
   (causing unexpected behavior is highly unlikely)
2. Avoid messing with the request at all (target milestone 17, for example)
   (this fixes the issue - point 2 in particular)
   (might cause undesired regressions/side-effects in broken proxies and/or code, either client or server-size)

Naturally, if breaking this down, a follow-up bug would be filled to deal with the remaining part.
Sorry, target milestone is only set when a fix is committed. It actually reflects the version in which the fix will be released.
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #8)
> Sorry, target milestone is only set when a fix is committed. It actually
> reflects the version in which the fix will be released.

Oops, sorry for not RTM before wasting everyone's time. Thanks for the quick response! :-)

Adding related Firebug issue, related in comment #0 and comment #2, in "See also".
Adding related issue in qooxdoo (JavaScript framework), where a script-based workaround is being proposed to dodge the issue for outdated and current Firefox versions.
Symptoms seem to have been fixed by bug 801826. Target milestone is set to version 19 as of this writing.

Thanks for the feedback, everyone! :-)
Status: NEW → RESOLVED
Closed: 11 years ago
No longer depends on: 309424
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.