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

VERIFIED FIXED in mozilla0.9

Status

()

Core
HTML: Parser
P2
major
VERIFIED FIXED
19 years ago
16 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: harishd)

Tracking

({arch, highrisk, testcase})

Trunk
mozilla0.9
arch, highrisk, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fix in hand ] hit during nsbeta2 standards compliance testing (py8ieh:need small testcases for lots of headers), URL)

Attachments

(2 attachments)

(Reporter)

Description

19 years ago
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

Updated

19 years ago
Status: NEW → ASSIGNED

Updated

19 years ago
Target Milestone: M7

Updated

18 years ago
Target Milestone: M7 → M8

Updated

18 years ago
Blocks: 7232

Comment 1

18 years ago
Pushed past necko landing...

Comment 2

18 years ago
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. ;-)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 3

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

Updated

18 years ago
Whiteboard: will attempt to verify when release builds available

Updated

18 years ago
Status: RESOLVED → REOPENED

Updated

18 years ago
Resolution: FIXED → ---

Comment 4

18 years ago
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.

Updated

18 years ago
Assignee: gagan → valeski
Status: REOPENED → NEW

Comment 5

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

Comment 6

18 years ago
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).

Updated

18 years ago
Assignee: valeski → vidur
No longer blocks: 7232
Target Milestone: M9 → M10

Comment 7

18 years ago
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.

Updated

18 years ago
Assignee: vidur → peterl

Comment 8

18 years ago
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).

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 9

18 years ago
Moving non-beta 1 items to M15
(Reporter)

Comment 10

18 years ago
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

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

Comment 12

18 years ago
Reassigning peterl's bugs to myself.

Comment 13

18 years ago
Accepting peterl's bugs that have a Target Milestone

Comment 14

18 years ago
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.)
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).

Updated

18 years ago
Whiteboard: will attempt to verify when release builds available

Comment 16

18 years ago
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.
(Reporter)

Comment 17

18 years ago
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

18 years ago
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.
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

18 years ago
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.
(Reporter)

Comment 21

18 years ago
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

18 years ago
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

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

Comment 24

18 years ago
Pushing my M15 bugs to M16

Comment 25

17 years ago
spam, changing qa contact from paulmac to tever@netscape.com on networking/RDF 
bugs
QA Contact: paulmac → tever

Comment 26

17 years ago
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.
Assignee: pierre → rickg
Status: ASSIGNED → NEW
Component: Networking → Parser
QA Contact: tever → janc
Target Milestone: M16 → M18

Comment 27

17 years ago
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!)  : )
Assignee: rickg → harishd
(Assignee)

Comment 28

17 years ago
putting on nsbeta3 radar.
Status: NEW → ASSIGNED
Keywords: nsbeta3
(Reporter)

Comment 29

17 years ago
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...
Blocks: 7954
Keywords: correctness, testcase
QA Contact: janc → py8ieh=bugzilla
Whiteboard: hit during nsbeta2 standards compliance testing
(Assignee)

Comment 30

17 years ago
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. 
Target Milestone: M18 → Future
(Reporter)

Comment 31

17 years ago
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.
Keywords: nsbeta3 → arch
Priority: P3 → P2
Whiteboard: hit during nsbeta2 standards compliance testing → hit during nsbeta2 standards compliance testing (py8ieh:need small testcases for lots of headers)
(Reporter)

Comment 32

17 years ago
Marking ns6.01 so that we fix this ASAP after the 6.0 release.
Keywords: ns6.01
(Reporter)

Updated

17 years ago
Summary: HTTP headers are not passed on to main NGLayout code → HTTP headers are not passed on to main NGLayout code [IMPORT]

Updated

17 years ago
OS: Windows 98 → All
Hardware: PC → All
Blocks: 34135
(Reporter)

Comment 33

17 years ago
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.
Keywords: ns601 → highrisk, mozilla0.9
(Reporter)

Updated

17 years ago
Keywords: nsbeta1
(Assignee)

Updated

17 years ago
Target Milestone: Future → mozilla0.9
(Assignee)

Comment 34

17 years ago
Created attachment 26366 [details] [diff] [review]
proposed patch v1.1
(Assignee)

Updated

17 years ago
Whiteboard: hit during nsbeta2 standards compliance testing (py8ieh:need small testcases for lots of headers) → [fix in hand ] hit during nsbeta2 standards compliance testing (py8ieh:need small testcases for lots of headers)

Comment 35

17 years ago
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.
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);
(Assignee)

Comment 37

17 years ago
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

17 years ago
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

17 years ago
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)
(Assignee)

Comment 40

17 years ago
Created attachment 26413 [details] [diff] [review]
patch v1.2 [ passing channel to the sink; made changes suggested by heikki; ]
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

17 years ago
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.
(Assignee)

Comment 43

17 years ago
Fix is in. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 44

16 years ago
excellent :-D
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.