3248 - HTTP headers are not passed on to main NGLayout code [IMPORT]

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Hixie (not reading bugmail)]

HTTP headers are not passed on to the main NGLayout code. For example, any
LINK HTTP headers should be parsed and passed on to the document to emulate
<LINK> elements.

This page does not refresh:
http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/refresh1.http.html
(See also bug #563, redirects)

The first two tests here are not green:
http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/cascade/acidlinkcascade.html

Test 65 does not go green:
http://www.bath.ac.uk/%7Epy8ieh/internet/importtest/extra/index.html

Comment 1

[Gagan]

Pushed past necko landing...

Comment 2

[leger]

Changing all Networking Library/Browser bugs to Networking-Core component for
Browser.

Occasionally, Bugzilla will burp and cause Verified bugs to reopen when I do
this in a bulk change.  If this happens, I will fix. ;-)

Comment 3

[Gagan]

This is now fixed with Necko. main NGLayout code can ask for any HTTP header it
wants using the channel.

Comment 4

[Paul MacQuiddy]

These tests do not pass on my windows system. The first two tests at
acidlinkcascade.html are not green, and test 65 does not pass. I do not know
enough about these tests to provide further info right now.

Comment 5

[Gagan]

Jud: who (docloader/webshell) should get this bug now?

Comment 6

[Judson Valeski]

The first url doesn't contain any refresh headers (therefore it's not
refreshing).

The other two urls deal w/ LINK meta tags.

Gagan, what does "LINK" do in http? I'm wondering if this is a case in which we
need to have our back channel into necko from layout (ugh).

Comment 7

[Judson Valeski]

re-assigning to vidur (he's my only layout contact ;) and moving to M10 as I
don't see this bumping other M9 dev work. I'm also removing 7232's dependency on
this bug, as this is now a layout issue.

Comment 8

[vidur (gone)]

This falls into Peter Linss's domain. The HTTP response header is now available
off the nsIChannel passed into the HTMLContentSink and should be queried for
link headers (section 19.6.2.4 of
http://www.cis.ohio-state.edu/htbin/rfc/rfc2068.html).

Comment 9

[Peter Linss]

Moving non-beta 1 items to M15

Comment 10

[Hixie (not reading bugmail)]

Hmm, http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/refresh1.http.html does
have a refresh header! This is what the server sends me:

 HTTP/1.1 200 OK
 Date: Tue, 12 Oct 1999 20:03:53 GMT
 Server: Apache/1.3.9 (Unix)
 Refresh: 3;url=refresh2.http.html                <<<< NOTICE THIS LINE! <<<
 Last-Modified: Wed, 16 Jun 1999 00:37:41 GMT
 ETag: "8061-877-3766f1d5"
 Accept-Ranges: bytes
 Content-Length: 2167
 Connection: close
 Content-Type: text/html

It is not refreshing. (It works in IE5.)

Comment 11

[Peter Linss]

I believe refresh headers were/are supposed to be handled down in network or
parser code...

Comment 12

[Pierre Saslawsky]

Reassigning peterl's bugs to myself.

Comment 13

[Pierre Saslawsky]

Accepting peterl's bugs that have a Target Milestone

Comment 14

[Warren Harris]

I believe this is a layout bug. You should be able to access the Refresh header
by QI'ing the nsIChannel object to an nsIHTTPChannel and calling GetHeader (or
something like that). Gagan can give you the details.

After you get the Refresh header, you have to add the magic that forces the
refresh after the specified timeout. Necko doesn't do that because it needs to
involve the docloader/webshell/current document, etc. (I believe.)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The magic that causes the refresh is already there.  <meta http-equiv="Refresh:
..." or whatever it is already works fine (see
http://www.psych.upenn.edu/~baron/david/satellite/ ).  The only thing that needs
to be done is using information from the HTTP header in the same place as the
info from the META.  (The same thing applies to Link elements in HTTP headers
and probably a few other things.)

It sounds like this should be pretty easy to fix, and it's blocking a few
features of Bugzilla, so I hope you can do it soon :-)

There is one question though... for headers that can only exist once (like
Refresh, and unlike Link (I think)), does the META take precedence or does the
HTTP header win?  HTML 4.0 is annoyingly silent on this issue (or at least I
couldn't find anything it says).

This should be tested with the Content-Type header too (charset parameter).

Comment 16

[Judson Valeski]

although the meta refresh code does a content refresh and re-load, a different
path should be taken to Refresh the page for the HTTP header. The meta stuff is
designed to handle *only* HTTP-EQUIV headers.

Comment 17

[Hixie (not reading bugmail)]

Well, that is probably not a good idea. The code that handles HTTP headers and
the code that handles HTTP equivalent headers found within the markup should be
one and the same. After all, almost every HTTP header can be also included in
HTTP-equiv tags. Why duplicate the code?

Comment 18

[Judson Valeski]

I share your concern. HTTP-EQUIV is a very, very, very *evil* hack. It's only
relevent in client's that can handle changing HTTP headers (the definition of
that is hard to narrow down too) *after* the actual HTTP transaction has taken
place. All the browser clients I can think of handle this as a hack which bites.
The problem is that you have content dictating a network level change. It flat
out doesn't make sense. Thus, the code is seperate.

BTW: per some specification (don't recall off the top of my head which) *any*
HTTP header can be represented as an HTTP-EQUIV header. This complicates the
situation in a large way.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ian - one of the other things about refresh headers is that they can be used
with images and things that are part of a document, rather than the main
document.  Probably the existing code only handles the case of the object being
the document.

However, it would probably be good if the HTTP-EQUIV code could let the network
code do the work, when possible.  Or something...

Comment 20

[Sean Richardson]

The LINK HTTP header, defined in section 19.6.1.2 of RFC 2068 (the original
HTTP 1.1 spec), appears in RFC 2616 (June 1999 HTTP 1.1 spec, obsoletes 2068)
only in the following context in section 19.6.3 "Changes from RFC 2068":
   >The Alternates, Content-Version, Derived-From, Link, URI, Public and
   >Content-Base header fields were defined in previous versions of this
   >specification, but not commonly implemented. See RFC 2068 [33].

This looks like abandonment, which makes no good sense: a consistent
set of links for every document at a server could be very useful.
The HTML 4 spec has this to say (in the subsection cited below):
  >Preferred style sheets specified with META or HTTP headers have precedence
  >over those specified with the LINK element.
This is unchanged in the HTML 4.01 spec, presumably edited after drafts
of the new HTTP 1.1 spec were available. Unless or until that statement
is removed from the HTML spec, continuing to use the content of any
"Link" HTTP header makes sense to me.

On the topic of precedence of HTTP-EQUIV versus natural HTTP headers:
Quoting from http://www.w3.org/TR/REC-html40/present/styles.html#h-14.3.2 :
  >If two or more META declarations or HTTP headers specify the preferred style
  >sheet, the last one takes precedence. HTTP headers are considered to occur
  >earlier than the document HEAD for this purpose.

Generalized, this would mean that any <META HTTP-EQUIV="" CONTENT=""> would
over-ride the CONTENT of the header (if any) for which it an EQUIValent.
A corollary: the final state of the HTTP header environment for an HTML
document is not knowable until the <HEAD> is closed explicitly or can
no longer be open.

The example of <META http-equiv="Default-Style" content="compact"> taking
precedence over preferred stylesheets defined with LINK in the same
subsection of the HTML 4 spec works correctly: it applies the stylesheet
titled "compact" even if another preferred stylesheet is LINKed first
(tested with 1999-12-05-16-M12 on WinNT).

The cleanest implementation would probably be to use the same parser for
META HTTP-EQUIV equivalent-to-headers as for actual HTTP headers, and
change any and all headers once only, when the <HEAD> is closed,
propagating any resultant changes at that time as if the headers with
substituted content had always been that way, even if that has implications
that invalidate everything done for the current document so far.

Comment 21

[Hixie (not reading bugmail)]

Dan Connolly and I are working together to write a new ID for the "Link" HTTP
header, although due to other work this project has been delayed. It was removed
from the HTTP ID for the exact reasons given: they were not commonly
implemented. As I understand it, for a feature to make it into an ID, it
generally needs two client implementations, two proxy implementations, and two
server implementations (if relevant each time).

BTW, IIRC Mozilla also supports the "Content-Base" HTTP header (when used in a
META tag, anyway).

I agree that the ideal implementation is one where we use one parser for both
real and meta HTTP headers, but I disagree that the </head> tag should have any
relevance. If there is ever any sort of equivalent mechanism for XML docs, it
is likely to be allowed anywhere (as a PI), so we should simple act on the
headers when we get them, not delay.

This is, IIRC, what we do now for both "link" and "content-base".

Comment 22

[Sean Richardson]

The relevance of </HEAD> *for HTML documents only* is that the last
preferred stylesheet specified with META HTTP-EQUIV takes precedence,
and until the HEAD is closed, there is no way to say that there isn't another.
Given that in HTML the META element may not occur in the BODY, it does not
look like there is anything to be gained by speculatively loading each
preferred stylesheet specified by META HTTP-EQUIV in turn when normally
the HEAD will be explicity or implicitly closed in less elapsed time than
it would take to receive the first packet of the first stylesheet, even
if this were done in parallel (non-pipelined). Whether done in parallel or not,
if there is more than one preferred stylesheet linked in by META HTTP-EQUIV
at least one would not be applied; beginning to load it would cause a
perceptible and wasteful delay before content appears on a slow link.

Point noted for XML - does this apply to XHTML also? I would agree that
any stylesheet linked in *after* the close of the HEAD should be applied
immediately, but this is not incompatible with waiting for the </HEAD>
for any linked in *before* that.

Comment 23

[leger]

Bulk move of all Networking-Core (to be deleted component) bugs to new
Networking component.

Comment 24

[Pierre Saslawsky]

Pushing my M15 bugs to M16

Comment 25

[Paul MacQuiddy]

spam, changing qa contact from paulmac to [e-mail] on networking/RDF 
bugs

Comment 26

[Pierre Saslawsky]

Despite what Vidur wrote on 1999-08-16, I'm really enclined to side with Peter 
when we wrote on 1999-10-12 "I believe refresh headers were/are supposed to be 
handled down in network or parser code...".

Pushing to M18. It sounds like the Necko folks have done their part. Reassigned 
to Parser.

Comment 27

[rickg]

Harish: I don't know whether this is a legit issue, or just folks passing the 
buck. Please take a look (I'm passing the buck, too!)  : )

Comment 28

[harishd]

putting on nsbeta3 radar.

Comment 29

[Hixie (not reading bugmail)]

jan: Since this is (was) mainly an issue with the CSS Import Test, I'm gonna
grab QA. Feel free to take it back if you want it...

Comment 30

[harishd]

spoke with Ian and he's okay with FUTURING this bug.  

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 31

[Hixie (not reading bugmail)]

Removing the nsbeta3 nomination (which was added by Harish) because he was
agreed to Futuring this bug.

Marking bug 'arch'. *As I understand it* this is an archicture problem. We need
to make sure we get to it very early on after RTM.

Comment 32

[Hixie (not reading bugmail)]

Marking ns6.01 so that we fix this ASAP after the 6.0 release.

Comment 33

[Hixie (not reading bugmail)]

Marking "highrisk arch mozilla0.9" to indicate that this is a fundamental change
to the architecture, but we've been putting it off for a while now, but that we
really want this in for the next release.

Comment 34

[harishd]

Attachment: proposed patch v1.1

Comment 35

[Judson Valeski]

couple questions.
1. is your tabbing->space setting in line w/ the rest of the file? There seems
to be some allignment missmatch. I can't tell if your fixing whitespace, or
injecting inconsistencies.
2. you're getting the "default load group" to acquire a channel. are you sure
that gives you the channel corresponding w/ the current document? Feels like you
might get the wrong channel.

Comment 36

[Heikki Toivonen (remove -bugzilla when emailing directly)]

A few comments. The indentation in the files seems to be weird - are there tabs?
Could you fix that?

I think you could avoid one variable & QI here, when you get the request just QI
directly to nsIHTTPChannel:
+      nsCOMPtr<nsIRequest> request;
+      loadgroup->GetDefaultLoadRequest(getter_AddRefs(request));
+      if(request) {
-        nsCOMPtr<nsIChannel> channel(do_QueryInterface(request));
-        if(channel) {
!          nsCOMPtr<nsIHTTPChannel> httpchannel(do_QueryInterface(request));
+          if (httpchannel) {


You could simply say "while(*name) {":
+            while(*name!=0) {

I would like to see here a check if we got key, and if not, return
NS_ERROR_OUT_OF_MEMORY:
+              key=NS_NewAtom(*name++);

nsXPIDLCString has a member function get() that returns const char* so you don't
need the cast. Also, did you try this without the temporary variable "value",
just passing tmp to ProcessHeaderData()? We have lots of automation happening
with strings:
+                nsAutoString value;
+                value.AssignWithConversion(NS_STATIC_CAST(const char*, tmp));
+                ProcessHeaderData(key,value);

Comment 37

[harishd]

Jud: 
1) The alignment gets wacky, sometimes, when I use -uw flags.
2) rpotts suggested using default load group to get the appropriate document 
channel. However, passing down the channel to the sink, when the sink gets 
constructed in the document, would probably clear the ambiguity. I can make the 
change, if you insist.

Comment 38

[Judson Valeski]

your call on the ambiguity. if you did make it explicit, you'd be shielded from
any semantic changes, or bugs, in the default load group implementation. if rick
suggested it, I'm sure it's gold though. your call.

Comment 39

[Gagan]

Isn't DefaultLoadRequest the first request on a loadgroup? Are you sure that is 
what you want? To me it seems that if you have an image with one of these 
headers that you care about, you might miss that header if that image sits on a 
document. Just double-check that with rick... otherwise looks ok to me (with 
heikki and jud's suggestions)

Comment 40

[harishd]

Attachment: patch v1.2 [ passing channel to the sink; made changes suggested by heikki; ]

Comment 41

[Heikki Toivonen (remove -bugzilla when emailing directly)]

r=heikki, although I suspect if we get out of memory situation we should
probably bail out immediately and not try to do anything else as that is also
likely to fail. Your call.

Comment 42

[vidur (gone)]

sr=vidur with a few recommendations:
1) I'd like to see nsCOMPtrs used for referencing the nsIAtoms that are created.
2) You can avoid an extra copy of the header value in ProcessHTTPHeaders by
wrapping the returned value in a nsLiteralString and making ProcessHeaderData
accept a nsAReadableString& for the value.
3) I didn't see the diff for the prototype for NS_NewHTMLContentSink (in
nsHTMLParts.h, I believe).

Similar code probably needs to sit in nsXMLContentSink, but that's another
problem and one that could be dealt with when you consolidate the content sinks.

Comment 43

[harishd]

Fix is in. Marking FIXED.

Comment 44

[Hixie (not reading bugmail)]

excellent :-D