[FIX]inspecting getElementsByTagName().length while a page is loading has strange consequences

RESOLVED FIXED in mozilla1.6alpha

Status

()

Core
DOM: Core & HTML
P1
major
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: jsp, Assigned: bz)

Tracking

Trunk
mozilla1.6alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925

Inspecting the length of a list returned by getElementsByTagName() early in page
loading causes incorrect lengths to be returned by later calls.

Reproducible: Always

Steps to Reproduce:
1. Load the test case.
2. Follow the directions in the page.
Actual Results:  
The number of items in the element list varies depending on whether or not the
length of a list of elements in the document is inspected while the document is
loading.

Expected Results:  
The length of the element list should always be the same.
(Reporter)

Comment 1

14 years ago
Created attachment 132465 [details]
test case
Created attachment 132466 [details]
Same testcase showing a bit more information
Attachment #132465 - Attachment is obsolete: true
Created attachment 132467 [details]
Same as XML (with a few minor changes to make it valid XML)
In HTML, it looks like the <body> is getting lost.  In XML, the <body> and
everything in it is getting lost.

This may be due to what the content model looks like when we run the script and
how that interacts with the document-observer behavior of content lists... I'll
try to look into this for 1.6b, but if someone has time before that and can work
on this, that would be much appreciated.
Assignee: dom_bugs → bzbarsky
Severity: normal → major
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.6beta
ccing some people who should know what's going on here...
Created attachment 132857 [details] [diff] [review]
This fixes things, but needs lots of testing

The problem with HTML seems to be that when the <body> tag is opened the sink
does NOT send out notifications for the content that's been appended into the
document up to that point.  In particular, in the testcase given it never sends
a notification for the <body> node.  If the <script> is moved to be before the
<meta> and <title> tags, then those don't get notified about either. The upshot
of all this is that the content list has no way to find out that the <body>
node exists (since it relies on observer notifications after its length has
been gotten).

Not notifying document observers at all just because presshells happen to not
care yet (because they have not done an initial reflow) is just wrong.	The
patch moves that part of the logic down to the one observer that cares -- the
presshell.

The XML case is simpler, since the XML sink is not incremental (bug 18333). 
The problem there, as far as I can tell, is that the XML sink never sends out
any ContentAppended or ContentInserted notifications whatsoever.  I'm actually
somewhat surprised we render XML documents.  ;)  So I added a ContentInserted
on the root element just before the StartLayout() call (the idea being that
since we've not called StartLayout() the presshell will ignore the
ContentInserted per change above and won't have bogus frame construction).

The changes look pretty reasonable to me; the question is what people think
they may break... what should I test?
This is very much alpha material... ;)
Summary: inspecting getElementsByTagName().length while a page is loading has strange consequences → [FIX]inspecting getElementsByTagName().length while a page is loading has strange consequences
Target Milestone: mozilla1.6beta → mozilla1.6alpha
Comment on attachment 132857 [details] [diff] [review]
This fixes things, but needs lots of testing

heikki, Peter, do you have time to review this?
Attachment #132857 - Flags: superreview?(hjtoi-bugzilla)
Attachment #132857 - Flags: review?(peterv)
Comment on attachment 132857 [details] [diff] [review]
This fixes things, but needs lots of testing

heikki won't get to this for a while, but he said that in general it looks like
the right thing to do...

I've tested this on the various content-duplication bugs I've found in
bugzilla, and it's not breaking anything that's not already broken.
Attachment #132857 - Flags: superreview?(hjtoi-bugzilla) → superreview?(jst)
Comment on attachment 132857 [details] [diff] [review]
This fixes things, but needs lots of testing

Seems like the right thing to do to me. sr=jst
Attachment #132857 - Flags: superreview?(jst) → superreview+
Comment on attachment 132857 [details] [diff] [review]
This fixes things, but needs lots of testing

Looks good to me, though I think we want the same notification to happen in
nsXMLContentSink::OnTransformDone, just before the call to StartLayout.
Attachment #132857 - Flags: review?(peterv) → review+
Created attachment 133038 [details] [diff] [review]
Patch with those comments addressed
Attachment #132857 - Attachment is obsolete: true
Checked in.  Amazingly enough, Tp is not affected in any way.  :)
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 14

14 years ago
this appears to have broken xml pretty printing.  see bug 222539.

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.