on moviefone.com, only the ad and tab bar display--page body is blank

VERIFIED FIXED

Status

()

P1
major
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: ekrock, Assigned: harishd)

Tracking

({regression})

Trunk
x86
Windows NT
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-] [Fix in hand], URL)

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
Using Commercial 2000092808 on WinNT 4.0 SP4.

To repro, compare display of http://moviefone.com/ in the current commercial
build and earlier ones, Nav4, or IE. The page body is blank in today's
commercial build.

This is a recent regression. A definite RTM stopper--moviefone.com is a popular
web site and a major AOL brand.
(Reporter)

Comment 1

18 years ago
Nominating rtm, marking regression.
Keywords: regression, rtm
Priority: P3 → P1
(Reporter)

Comment 2

18 years ago
4xp, P1. This is a high profile AOL brand and high profile backwards 
compatibility.
Keywords: 4xp
(Reporter)

Comment 3

18 years ago
Reassigning to karnaze for initial triage and analysis so this doesn't languish 
on Clayton's plate.
Assignee: clayton → karnaze
(Reporter)

Comment 4

18 years ago
Nominating nsbeta3 as this might even qualify for stop-ship nsbeta3.
Keywords: nsbeta3

Comment 5

18 years ago
jst has agreed to take a look. When javascript is turned off, much of the 
content shows up (although not 100% correct).
Assignee: karnaze → jst

Comment 6

18 years ago
I'm reassigning this to harishd and ccing rickg, since ekrock thinks this is a 
serious bug (I'm also marking it nsbeta3+, rtm+).

from jst:

I had a look at the page and I know what's causing the bad layout, it's 
basically due to the fact that the page contains things like:

<noscript>
<table>
...
</noscript>
</table>

I.e. ill-formed HTML, could you reassign the bug to me so that I can 
comment on it and forward it to Harish?
Assignee: jst → harishd
Whiteboard: [nsbeta3+] [rtm+]

Comment 7

18 years ago
Eric, I just spoke to Rick and he promised to look at it tomorrow (so I'm 
reassigning it to him). I'm also removing the nsbeta3+ that I added.
Assignee: harishd → rickg
Whiteboard: [nsbeta3+] [rtm+] → [rtm+]

Comment 8

18 years ago
In fact I'm trying to recreate the problem now; then I'll determine what IE and 
nav do in such cases. Hold please...
Status: NEW → ASSIGNED
(Assignee)

Comment 9

18 years ago
Rickg, feel free to assign this bug to me. The problem is that when SCRIPT is 
enabled the NOSCRTIP contents are not autoclosed.

Comment 10

18 years ago
Using an enlarged testcase (attached), neither 4x nor IE render this (error) 
case as a table. The behavior is that containers opened in the <noscript> don't 
propagate out (as you'd expect) when the </noscript> appears. 

The solution is to autoclose containers upon the </noscript> and handle the 
broken markup that follows as usual. I should have a patch sometime early 
tomorrow.  

Comment 11

18 years ago
Ok -- I have a short (6 line) patch which seems to fix the problem. I'll run the 
top 100, and should be ready with this for tomorrow, if there's any hope of 
landing for pr3.

Comment 12

18 years ago
Even simpler: I have it down to a 1 line change.

Updated

18 years ago
Whiteboard: [rtm+] → [rtm+] fix in hand

Comment 13

18 years ago
Marking "needinfo". This seems like a bad thing, but we need a patch, review and
super review to clear for checkin.
Whiteboard: [rtm+] fix in hand → [rtm+ needinfo] fix in hand
(Reporter)

Comment 14

18 years ago
We should accept this for RTM. It is backward compatibility with poorly-formed
HTML 3.2 on the web--there's a heck of a lot of that on the web the last time I
checked! moviefone is a high profile AOL site. Also, if movefone.com screwed
this tag order up, others likely did too, and I'd rather not find out how many
the hard way.

Updated

18 years ago
Whiteboard: [rtm+ needinfo] fix in hand → [rtm+ need info] fix in hand

Comment 15

18 years ago
Here's the patch...

Index: CNavDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v
retrieving revision 3.317
diff -r3.317 CNavDTD.cpp
1635c1635,1636
<       if(thePrevTag==aContext[theChildIndex]){
---
>       if((thePrevTag==aContext[theChildIndex]) ||
>          (eHTMLTag_noscript==aCurrentTag)){  //bug 54571
Whiteboard: [rtm+ need info] fix in hand → [rtm+] fix in hand
(Assignee)

Comment 16

18 years ago
Looking at the patch I assume that we're autclosing noscript contents regardless
of whether script is ON or OFF. If my assumption is correct then I'd argue that
noscript contents *should not* be autoclosed when script is turned OFF.

Comment 17

18 years ago
Harish is right. I left out the code that caches the javascript (ON|OFF) state. 
Harish: I'm giving this back to you, since you wrote the code returns state 
(from the sink) indicating scripting status. Your comments show that we get 
NS_HTMLPARSER_ALTERNATECONTENT if js is enabled, but it's not clear how we know 
JS disabled. If you know, then cache the state with the DTD (I'll assume 
mScriptDisabled), and change my patch thusly:

Index: CNavDTD.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/CNavDTD.cpp,v
retrieving revision 3.317
diff -r3.317 CNavDTD.cpp
1635c1635,1636
<       if(thePrevTag==aContext[theChildIndex]){
---
>       if((thePrevTag==aContext[theChildIndex]) ||
>          (mScriptDisabled && (eHTMLTag_noscript==aCurrentTag))){  //bug 54571

Assignee: rickg → harishd
Status: ASSIGNED → NEW
(Assignee)

Comment 18

18 years ago
When JS is enabled we get a message from the sink NS_HTMLPARSER_ALTERNATECONTENT
( meaning that NOSCRIPT content should not be treated as a regular content ).
When JS is disabled sink would send an NS_OK message ( meaning that the NOSCRIPT
content should be treated as a regular content, i.e., as though NOSCRIPT did not
exist ). The message from the sink is stored in mDTDState.  I'll modify rickg's
patch accordingly.

Btw, I think the white board should be changed from [rtm+] to [rtm need info].
Status: NEW → ASSIGNED

Comment 19

18 years ago
PDT wants this fix, but can't tell r=who? or sr=who? Can you provide that info
and then we'll get it in.
(Assignee)

Comment 20

18 years ago
Created attachment 16341 [details] [diff] [review]
Proposed patch

Comment 21

18 years ago
PDT marking [rtm need info]. Need reviewers.
Whiteboard: [rtm+] fix in hand → [rtm need info] fix in hand

Comment 22

18 years ago
a=hyatt.  i use moviefone regularly, so i have a vested interest in getting this 
in. :)
(Assignee)

Comment 23

18 years ago
Thanx for the approval hyatt!
(Assignee)

Comment 24

18 years ago
Created attachment 16882 [details] [diff] [review]
Proposed patch...
(Assignee)

Updated

18 years ago
Whiteboard: [rtm need info] fix in hand → [rtm need info] [Fix in hand]
(Assignee)

Updated

18 years ago
Depends on: 56501

Comment 25

18 years ago
This bug will be fixed when Harish checks in his fix for bug 56501 into the
branch.  That fix is in the trunk right now and will get checked into the branch
once the PDT blesses bug 56501 rtm++.

Marking rtm-.
Whiteboard: [rtm need info] [Fix in hand] → [rtm-] [Fix in hand]
(Assignee)

Comment 26

18 years ago
56501 changes are in. This should be fixed now.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 27

18 years ago
Verified fixed in the 2001010209 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.