Mozilla briefly displays "<html><body></body><html>" during redirects

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: marc.loiselle, Assigned: rpotts)

Tracking

({regression, topembed})

Trunk
x86
All
regression, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

17 years ago
Redirects using Location http header result in Mozilla briefly displaying
"<html><body></body><html>"

Buildid 2001102403 on Windows
I saw it on buildid 2001102203 on linux as well

Related to bug 102737

Recent regression

Comment 1

17 years ago
been seeing that on linux too for the past days.
Fine sample page is the CNN "Quickvote" window.

Updated

17 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
looks like docshell or the uriloader code may not be handling
NS_BINDING_REDIRECTED properly.

cc'ing rpotts for help.

Comment 3

17 years ago
It's not just the <html><body></body></html> bits.

Point a browser to:

http://www.slashdot.org

You get to see a lovely message about permanent redirection to

http://slashdot.org

which is well and good, but I haven't seen that message except for in the last
few days, and I have a pretty good feeling that redirection's been around for a
lot longer.

I've seen this on both my Linux and Windows builds as well.

Comment 4

17 years ago
-> docshell
Assignee: darin → adamlock
Component: Networking: HTTP → Embedding: Docshell
QA Contact: tever → adamlock

Comment 5

17 years ago
-> rpotts
Assignee: adamlock → rpotts

Comment 6

17 years ago
Seeing this as well in a lot of different sites.

Comment 7

17 years ago
yea, this is all over the place. click on a link at http://my.netscape.com and
you'll see it there too. upping severity, and nominating for mozilla0.9.6

I'm seeing this on today's mozilla win2k build.
Severity: normal → critical
Keywords: mozilla0.9.6, regression

Comment 8

17 years ago
I think bug 98203, bug 102737 and bug 106558 are all about the same thing. Do we
want to concentrate on just one bug and mark the others DUPEs?

I have been seeing this for a while on some pages if the connection times out or
something, but recently (late october builds) it has appeared a lot more, most
noticable when trying to go to http://www.hotmail.com/, since it redirects you
somewhere else every time.
(Assignee)

Comment 9

17 years ago
It appears that this problem was introduced on 10/18/01 in rev 1.59 of
nsHTtpChannel.cpp.  

nsHTttpCHannel.cpp line 1236: mListener->OnStartRequest(...) was added.

This exposed a problem upstream in the URILoader - which up until this point had
only dealt with NS_BINDING_ABORTED / NS_ERROR_NO_CONTENT messages from
OnStartRequest.

Since NS_BINDING_REDIRECTED was *not* one of these two error messages, the
notification was passed through.  This caused the UnknownDecoder to be hooked up
as the listener - because the content type of the redirected channel was *not* set.

Because the stream was empty, the UnknownDecoder called the content type
'text/plain' and passed it to the parser.

The parser received an OnStopRequest immediately following the OnStartRequest
(with no calls to OnDataAvailable) os it dropped into some code which added
'<html><body></body></html>' to the stream...

Unfortunately, the content-type of the "empty" stream is text/plain *so* the
'empty html document is emited as plain text :-(

what a mess !!!
(Assignee)

Comment 10

17 years ago
I forgot to mention that the fix for this bug also addresses the issues of bug
#72679
(Assignee)

Comment 11

17 years ago
Created attachment 55644 [details] [diff] [review]
proposed patch to stop the document loading process if any error occurs in OnSTartRequest...
(Assignee)

Comment 12

17 years ago
I would *really* like to remove ProcessCanceledCase(...) altogether.  but i'm
afraid that bug #40116 might reappear :-(

-- rick

Comment 13

17 years ago
will this patch cause trouble for "host-not-found" errors that need to be
propagated downstream?
(Assignee)

Comment 14

17 years ago
no, because the OnStopRequest will *still* be propagated through...  it is only
the OnStartRequest that will be supressed in error conditions...
(Assignee)

Comment 15

17 years ago
Created attachment 56030 [details] [diff] [review]
better patch that removes ProcessCanceledCase(... ) too...
(Assignee)

Comment 16

17 years ago
I've just attached a 'better' patch which removes ProcessCanceledCase(...) too...

The main difference in behavior between this patch and the last one, is that if
an error occurs on a channel, any pending data will be flushed through
OnDataAvailable(...).  Before, ProcessCanceledCase(...) would short-circuit the
call.

I believe that this is safe...  To minimize possible circular references, i'm
clearing out 'm_contextListener' and 'm_originalContext' in OnStopRequest 
(which is *always* called).

still looking for r= and sr=  :-)

(Assignee)

Comment 17

17 years ago
*** Bug 72679 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Blocks: 19073

Comment 18

17 years ago
Comment on attachment 56030 [details] [diff] [review]
better patch that removes ProcessCanceledCase(... ) too...

r=darin (this looks correct to me)
Attachment #56030 - Flags: review+

Comment 19

17 years ago
Comment on attachment 56030 [details] [diff] [review]
better patch that removes ProcessCanceledCase(... ) too...

sr=mscott although I'm nervous...our track record with changing this piece of good isn't very good =). Also, are you worried that by taking out the ProcessCanceledCase we are going to regress the original crash I was fixing with that? I forget the bug # but we can look it up in lxr if we have to.
Attachment #56030 - Flags: superreview+
(Assignee)

Comment 20

17 years ago
hey scott,

the original bug that ProcessCanceledCase(...) was added to fix was bug #40116.  

Since we are now short-circuiting OnStartRequest(...) for *any* failure - not 
just NS_BINDING_ABORTED - i believe that we should be safe.  after all, we are 
just being *more* restrictive, not less :-)

-- rick
(Assignee)

Comment 21

17 years ago
I've checked the patch in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 22

17 years ago
Is this bug related to "Object Moved" message, displayed briefly before 
redirecting ? This message should be removed, too, because many web developers
rely heavily on redirecting mechanism, and this message ruins "continious
browsing" fealing.
Reopening.  This caused bug 108869.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

17 years ago
Created attachment 56942 [details] [diff] [review]
an even better patch (that doesn't break multipart/x-mixed-replace)...
(Assignee)

Updated

17 years ago
Attachment #55644 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #56030 - Attachment is obsolete: true

Comment 25

17 years ago
Comment on attachment 56942 [details] [diff] [review]
an even better patch (that doesn't break multipart/x-mixed-replace)...

of course!!  (r/sr=darin)
Attachment #56942 - Flags: review+
*** Bug 109329 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 27

17 years ago
I've just checked the 'better patch' in.  i believe that everything should go
smoothly with this one :-)
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Is this good enough for 0.9.6?
Blocks: 104864

Comment 29

17 years ago
nominating for EDT.
Keywords: edt0.9.4
Keywords: mozilla0.9.6 → mozilla0.9.6+

Comment 31

17 years ago
this should be fine for 0.9.6... however, the fix for bug 108267 also went in
before branching for 0.9.6, and it should have also eliminated the temporary
<html><body></body><html> page shown during redirects (since bug 108267 covered
the cause of the regression).  so, unless this patch helps with other bugs, i
don't see it being quite so critical anymore.
Well, using the test case in the bug I don't see anything during the redirect. 
Was the bug really obvious using the URL above?  If it's not a big deal then
I'll leave it be.
No longer blocks: 104864
Keywords: mozilla0.9.6+

Comment 33

17 years ago
we're not experiencing this empty/tag doc stuff in 0.9.4, minusing.
Keywords: edt0.9.4 → edt0.9.4-
EDT - marking this with a plus. 
Keywords: edt0.9.4- → edt0.9.4+

Comment 35

17 years ago
Tested on the following:
11_14_22_0.9.4ec build on Win98
11_28_22_0.9.4ec build on Win2000
12_04_10_trunk build on Win2000
12_04_08_trunk build on Linux

Tested using these sites:
http://www.myforums.com
http://my.netscape.com
http://www.slashdot.org

Also tested using a bogus ftp address where I previously saw this problem.
Results: No evidence of '<html><body></body><html>' display in any testing.

*** Bug 113573 has been marked as a duplicate of this bug. ***

Comment 37

17 years ago
that's what I thought as well. minusing.
Keywords: edt0.9.4+ → edt0.9.4-

Updated

17 years ago
Keywords: topembed
You need to log in before you can comment on or make changes to this bug.