[FIX]regression from seamonkey 1.1a, page rendered incorrectly

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
HTML: Parser
P1
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Adam Rambousek, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.9alpha1
x86
Linux
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a1) Gecko/20060610 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a1) Gecko/20060610 SeaMonkey/1.5a

In Gecko/20060605 SeaMonkey/1.1a, this page is rendered alright, but in any build of SeaMonkey/1.5a the content of large frame is somehow doubled and it's not possible to fill in the form fields. The page uses lot of Javascript, so it's possible the bug is somehow related to it.

Reproducible: Always

Comment 1

12 years ago
The change on the trunk was due to 332644 (verified via backout), but the page doesn't load properly if I load it locally, so I don't know what's actually going on.
Assignee: general → mrbkap
Component: General → HTML: Parser
Product: Mozilla Application Suite → Core
QA Contact: general → parser
Version: unspecified → Trunk
Hmm...  I'd have thought this would be a duplicate of bug 339249, but I seem to see this in builds that postdate that fix.

I'll look into this, but if someone can come up with a minimal testcase that would be wonderful....
Blocks: 332644
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 225194 [details]
Stack to some interesting layout asserts that fire

The problem seems to be that we have two HTMLContentSinks around that have the same mDocument:

(gdb) p this
$23 = (HTMLContentSink * const) 0x8f917d0
(gdb) p ((nsParser*)((nsHTMLDocument*)mDocument.mRawPtr)->mParser.mRawPtr)->mSink.mRawPtr
$24 = (nsIContentSink *) 0x8f9181c
(gdb) p ((class HTMLContentSink*)((nsParser*)((nsHTMLDocument*)mDocument.mRawPtr)->mParser.mRawPtr)->mSink.mRawPtr)->mDocument
$26 = {mRawPtr = 0x91b8630}
(gdb) p mDocument
$27 = {mRawPtr = 0x91b8630}

We end up doing an append notification on |this|, which triggers editor munging the content model, which flushes tags on the other sink.  Since the mInNotification member is per-sink, the other sink doesn't know we're in the middle of a flush ourselves...

Blake, any idea what's going on here?  document.write is _definitely_ involved and the CloseBody() call comes from document.close() being called in JS.
Created attachment 225211 [details]
reduced testcase

This took a while to reduce :).
Keywords: testcase
Created attachment 225212 [details]
better reduced testcase

This still shows the problem on the initial load, but doesn't happen again if you [shift+]reload.
Attachment #225211 - Attachment is obsolete: true
Created attachment 225218 [details] [diff] [review]
Proposed fix

In brief, the terminated parser's sink didn't die right away, so kept observing the document.  Since the <html> node is not changed when we do document.open(), both sinks had contexts pointing to it; the old sink suddenly saw new content on that context when the new head and body got created, and decided it had to notify.

The patch just stops observing the document at what looks like the right spot to me in the sink.
Attachment #225218 - Flags: superreview?(mrbkap)
Attachment #225218 - Flags: review?(mrbkap)
Attachment #225218 - Flags: superreview?(mrbkap) → superreview?(bugmail)
Assignee: mrbkap → bzbarsky
Priority: -- → P1
Summary: regression from seamonkey 1.1a, page rendered incorrectly → [FIX]regression from seamonkey 1.1a, page rendered incorrectly
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 225218 [details] [diff] [review]
Proposed fix

This patch seems fine on its own, but I think you're missing nsHTMLContentSink.cpp (i.e., the actual bug fix)...
Did you attach the wrong patch here?
Created attachment 225349 [details] [diff] [review]
Actual patch

Er... yes.  This is the real fix.  It's just one line of code and some comments.
Attachment #225218 - Attachment is obsolete: true
Attachment #225349 - Flags: superreview?(bugmail)
Attachment #225349 - Flags: review?(mrbkap)
Attachment #225218 - Flags: superreview?(bugmail)
Attachment #225218 - Flags: review?(mrbkap)
Comment on attachment 225349 [details] [diff] [review]
Actual patch

Did you figure out what was keeping the content sink alive? That might be worth fixing as well (if possible).
Attachment #225349 - Flags: review?(mrbkap) → review+
> Did you figure out what was keeping the content sink alive?

Probably the fact that the parser is alive.  This is kept alive by the termination function set by the first document.close() call.  I'm not sure that's something we want to "fix".
Attachment #225349 - Flags: superreview?(bugmail) → superreview+
Fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.