Closed Bug 341062 Opened 18 years ago Closed 18 years ago

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

Categories

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

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: rambousek, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(3 files, 2 obsolete files)

Stack to some interesting layout asserts that fire
23.82 KB, text/plain
Details
better reduced testcase
295 bytes, text/html
Details
Actual patch
1.69 KB, patch
mrbkap
: review+
sicking
: superreview+
Details | Diff | Splinter Review 
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
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
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.
Attached file reduced testcase (obsolete) — 
This took a while to reduce :).
This still shows the problem on the initial load, but doesn't happen again if you [shift+]reload.
Attachment #225211 - Attachment is obsolete: true
Attached patch Proposed fix (obsolete) — — Splinter Review 
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?
Attached patch Actual patch — — Splinter Review 
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
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: