1248613 - Fix testing/web-platform/meta/XMLHttpRequest/setrequestheader-header-allowed.htm Pragma header missing regression

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

The regression has been there since ever.  It was fixed with bug 1176988.  Caused more problems (bug 1215970) and had to be reverted in bug 1248049.  That introduces this web-compat regression again.  That needs to be fixed.

Josh, can you please take care of this bug?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Wait, why is it Josh's responsibility to fix a bug you're introducing?

But since I'm already wasting time on this, I actually ... ran the test.  The basic problem is that per spec a web page should be able to set the "Pragma" header, but this patch breaks that: setting LOAD_BYPASS_CACHE will clobber the page-set "Pragma" header; see nsHttpChannel::SetupTransaction.

The only way to fix this on the DOM side is to not pass LOAD_BYPASS_CACHE.  If that's what we should do, then just do so in bug 1215970.  If we really do want LOAD_BYPASS_CACHE here, then the fix for this will need to be on the necko side.

Comment 2

[Boris Zbarsky [:bzbarsky]]

By "this patch" in comment 1 I mean the patch for bug 1215970.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And note that it's possible we need a spec change here to allow overriding the Pragma header like this.  Again, that would be something to decide on the necko end first (e.g. whether bypassing intermediate caches for this case is really useful and if so whether setting the Pragma header is the right way to do it).

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Wait, why is it Josh's responsibility to fix a bug you're introducing?

Thanks for helping here Boris.

First, I'm not introducing anything.  It's been there since ever.  It has been "fixed" because Josh has landed a networking patch, not reviewed by a necko peer, that caused a major regression.  I'm just asking if he can take care.  If not, someone else will have to, probably me.  A price I win for fixing a completely different bug.  But that happens..

> 
> But since I'm already wasting time on this, I actually ... ran the test. 
> The basic problem is that per spec a web page should be able to set the
> "Pragma" header, but this patch breaks that: setting LOAD_BYPASS_CACHE will
> clobber the page-set "Pragma" header; see nsHttpChannel::SetupTransaction.

http://hg.mozilla.org/mozilla-central/annotate/d719ac4bcbec/netwerk/protocol/http/nsHttpChannel.cpp#l791

To be exact we don't clobber, we add no-cache to the end.

> 
> The only way to fix this on the DOM side is to not pass LOAD_BYPASS_CACHE. 
> If that's what we should do, then just do so in bug 1215970.  If we really
> do want LOAD_BYPASS_CACHE here, then the fix for this will need to be on the
> necko side.

The reason I want to pass LOAD_BYPASS_CACHE down is to prevent delays of opening a cache entry that definitely doesn't exist (creation of a file) that will never be used - we do so even we only open-reaonly the entry.  Could potentially be fixed in the cache, tho would bring some complexity.


(In reply to Boris Zbarsky [:bz] from comment #3)
> And note that it's possible we need a spec change here to allow overriding
> the Pragma header like this.  Again, that would be something to decide on
> the necko end first (e.g. whether bypassing intermediate caches for this
> case is really useful and if so whether setting the Pragma header is the
> right way to do it).

We also have LOAD_BYPASS_LOCAL_CACHE that might be more appropriate for this (only skip our cache, but don't tell anything about any intermediate caches ; I think this flag has to be there since ever).  It will not reintroduce the web-compat regression and will also preserve optimal behavior (no caching).

I'll check on it and close this bug if confirmed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> To be exact we don't clobber, we add no-cache to the end.

From the point of view of the test, it's the same: the upshot is that the server does not get the value it's expecting.

Thanks for looking into this!

Comment 6

[Honza Bambas (:mayhemer)]

Using BYPASS_LOCAL_CACHE in bug 1248049 doesn't introduce this regression.