4891 - document.forms[x] not defined if form is document.written out

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Akkana Peck]

If you create a form text input field via javascript, and set its contents via
javascript, the form is laid out twice: once in the correct location, and once
at the top of the page.

See the attached URL for a small example (internally, it's at
http://warp/employees/akkana/bugs/jsformbug.html or
/u/akkana/public_html/bugs/jsformbug.html to make it easier to see the JS code).

Comment 1

[troy]

Eric, forms related

Comment 2

[leger]

Linux specific, or all platforms?

Comment 3

[Akkana Peck]

All platforms.

Comment 4

[Eric Pollmann]

Redistributing to M8...

Comment 5

[Chris Petersen]

With the June 30th build (Mac, Win 95, Linux), only one text field appears in the
test case. This appears to be fixed.

Comment 6

[Eric Pollmann]

Didn't make M8

Comment 7

[Eric Pollmann]

This bug has been fixed (it was caused by a malformed content model, so I
believe thanks go to Harish or Rickg).

Comment 8

[Eric Pollmann]

Opps, I meant to say: This bug is now hidden by another bug - form[0] is not
defined at the time that the value setting operation is executed.  I don't think
this is feasible to get done for M10.

Comment 9

[Eric Pollmann]

After careful consideration, I've decided that I probably won't get this bug in
for M12.  Currently I have nearly 50 bugs scheduled for M13, so there is a
possibility that this bug may need to be moved out farther still.

Comment 10

[ekrock's old account (dead)]

Moving to M16. Ugly but doesn't prevent use of page.

Comment 11

[Eric Pollmann]

Hey Vidur, here's another backwards compatability issue - wondered if you had
any thoughts on it?

Comment 12

[Eric Pollmann]

Rescheduling

Comment 13

[Akkana Peck]

I keep seeing this bug moved out, milestone after milestone.  But I'm not seeing
it any more, on the test page, and haven't for quite a while now.  I thought it
was fixed; is there reason to believe it's still happening? 

Comment 14

[ekrock's old account (dead)]

Nominating nsbeta2. We have to start drawing a line on DOM0 backward 
compatibility; these bugs are supposed to be a high priority for nsbeta2 per the 
beta2 criteria.

Comment 15

[Eric Pollmann]

Text field no longer shows up twice.

Comment 16

[Eric Pollmann]

This bug is not for the general document.forms[x] case, but only if the form is 
written out by a script.  We should get this working, but it is less severe than 
if the general case had been broken.

Comment 17

[Eric Pollmann]

I have a fix for this in my tree - it is a small change to stop caching the list 
of forms in the document.  We not traverse the document looking for forms 
whenever someone calls document.mforms.

I'll attach a preliminary diff, though I'll also have to go and nuke all other 
references to mForms before I can check this in.

Comment 18

[Eric Pollmann]

Attachment: Preliminary fix

Comment 19

[Johnny Stenback (:jst)]

Caching the forms in a document should work just fine even if the document is
dynamically changed, the nsContentList that holds the form elements in the
document is a and observer of the document and it should be notified when the
document changes and it should update itself to contain new form elements even
if the elements are dynamically added to the document. I think we should fix the
content list to work properly with dynmically added content in stead of removing
the caching, since that could really hurt performance on large documents that
contain form elements that are accessed through document.forms[x].

Eric, please don't remove the caching, either have a look at the document
observer methods in layout/base/src/nsContentList.cpp and see if you find the
problem, or reassign this to me and I'll have a look.

Comment 20

[Eric Pollmann]

Hm, the problem seems harder to reproduce now - did fixes recently go in that 
would have helped this?

I noticed that if printStuff is called in the onload method on the body, this is 
still reproducible.  The problem appears to be that we write out <form>, then 
try to access the forms array before writing out </form>.  We don't get a 
content appended call in the content list until after the forms array is 
accessed.  The stack for the content appended is:

nsContentList::ContentAppended(nsContentList * const 0x0378dd0c, nsIDocument * 
0x0377a860, nsIContent * 0x03752648, int 0) line 249
nsDocument::ContentAppended(nsDocument * const 0x0377a860, nsIContent * 
0x03752648, int 0) line 1641
nsHTMLDocument::ContentAppended(nsHTMLDocument * const 0x0377a860, nsIContent * 
0x03752648, int 0) line 1147
HTMLContentSink::NotifyAppend(nsIContent * 0x03752648, int 0) line 4151
SinkContext::FlushTags(int 1) line 1901
HTMLContentSink::Notify(HTMLContentSink * const 0x03787978, nsITimer * 
0x0378dfc0) line 2326
nsTimer::Fire() line 200
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x01562120, unsigned 
int 0) line 117
nsAppShell::Run(nsAppShell * const 0x015164e0) line 116
nsAppShellService::Run(nsAppShellService * const 0x01515c30) line 372

Should we flush the sink after every document.write(ln)?

Attaching a test case that still shows the bug.

Comment 21

[Eric Pollmann]

Attachment: test case

Comment 22

[Johnny Stenback (:jst)]

Hmm, flushing the sink after every document.write() would probably also be quite
bad for performance, perhaps a fair compromise would bee to drop the cached
mForms in the HTML document every time document.write() (and writeln) is called?
Something similar probably needs to be done for mImages, mApplets, mEmbeds,
mLinks, mAnchors, mForms and mLayers (whow, why do we still have a mLayers?).

This should be fairly straight forward to do in nsHTMLDocument::WriteCommon()
and nsHTMLDocument::ScriptWriteCommon() (those two functions should probably be
one, but that's a later problem).

Eric, I haveb't tried this but do you think this sounds reasonable?

Comment 23

[Eric Pollmann]

> Hmm, flushing the sink after every document.write() would probably also be
> quite bad for performance

Ah, true, it could be - haven't tested.

It might also lead to some nasty things like adding selects with half of their 
options and other badness.  :(

> perhaps a fair compromise would bee to drop the cached mForms in the HTML
> document every time document.write() (and writeln) is called?

Will this have the desired effect?  What I noticed was that the form wasn't 
added to the content model until the "</form>" was written out (causing the sink 
to flush).  So even if we drop mForms, and regenerate it, won't it still be out 
of date?  Or does creating a content list flush the sink?

Also, what about a case like this:

...
frm = document.forms
document.write("<form><input type=text name=foo>")
frm[0].foo.value = "Set the value!"
document.write("</form>");
...

This is very close to what the original test case tries, but won't even hit the 
documents GetForms method to regenerate the cached data in time to set the 
input's value (So neither of our fixes will have any effect, I think).

I originally didn't know that the content list was a document observer - it's 
great that it is!  Shouldn't being a document observer be sufficient to solve 
all these cases, provided that the document is updated (sink flushed) soon 
enough after document.write's?  What kinds of bad side effects could this cause 
beyond performance problems?

Comment 24

[Eric Pollmann]

See also bug 32022, which is related.

Comment 25

[leger]

Sent over to rickg for PDT call.

Comment 26

[rickg]

I think it's time to nail this one down; approving for nsbeta2. 
Eric: flushing the sink after every document.write() would be unwise.

Comment 27

[leger]

Removing [NEED INFO] from Status Whiteboard.

Comment 28

[Eric Pollmann]

As an experiment, I changed this so we do flush the sink after every 
document.write.  As you might have expected, this actually improves perceived 
performance for some document.write cases (content is created synchronously).  I 
am now in the process of cooking up some long devious test that might be slower 
or incorrect due to the flushing to compare it with not flushing after every 
document.write.  Can any of you (particularly those opposed to flushing) pitch 
in test cases here, or descriptions thereof, in case I'm overlooking something 
where performance would suffer because of this?  I'll report back with data 
after this is done.

One other possibility that might also work for the case mentioned above on 
2000-05-22 17:47: throwing away mForms after every document.write as suggested 
by Johnny, then also flushing the sink every time just before mForms's content 
list is generated - sound better?

Comment 29

[Eric Pollmann]

<html>
 <head>
  <script>

   function writeTable() {
    document.open();
    document.write("<table>");
    for (i=0; i<300; i++) {
     document.write("<tr><td>test"+i+"</td></tr>");
    }
    document.write("</table>");
    document.close();
   }

</script>
</head>

<body onload="writeTable();">
 fail
</body>
</html>

The above test case takes 6 seconds to display if we flush after every 
document.write, but just over 1 second to display if we take the second approach 
mentioned above.  The performance hit is obviously unacceptable.  :S  I'll test 
the second approach for correctness and go that route.

Comment 30

[Eric Pollmann]

Came up with another fix for this that doesn't cause performance to suffer like 
the above, and does work for the tricky test case:

No changes to nsHTMLDocument

in nsContentList::Item and nsContentList::NamedItem, flush pending notifications 
on the document before getting the item or named item.  This will cause any 
pending content changes to go through (and the contentList will be rebuild as 
needed), but acts largely as a noop if there are no pending notifications.

Comment 31

[Johnny Stenback (:jst)]

That sounds lile a pretty good solution and I'd say, go for it.

There is a (probably) minor downside to that approach tho, we're slowing down
all access to document.forms[x] a bit since we do the flushing of the document,
which ends up flushing the sink, if ther is one, and the pres shell, IMO we only
need to flush the sink, never the pres shell in this case, but doing that is
AFAIK not straight forward in todays code since there's no easy way to get the
content sink from the document in nsContentList. One possible solution to that
problem could be to add an argument to nsIDocument::FlushPendingNotifications()
that could tell if we want to flush pending content only or if we also want to
flush pending reflows, but that could go into a separate bug...

Comment 32

[Eric Pollmann]

New bug on the content/reflow flushing split is bug 42892.  I have a rough cut 
at this, but it is probably not a big enough gain to worry about for nsbeta2.

Comment 33

[Eric Pollmann]

Attachment: proposed fix

Comment 34

[Johnny Stenback (:jst)]

Eric, I had a look at the patch and it looks ok to me, r=jst
I agree, I wouldn't worry about the content/reflow flushing split for beta2
either...

Comment 35

[Eric Pollmann]

I checked this in.

Unfortunately, I also remembered after commiting that one other change is needed 
to make this do anything.  Basically, FlushPendingNotifications doesn't do 
anything unless we're currently inside a <script> tag.  That means that in the 
onLoad handler, as when this function is called, FlushPendingNotifications will 
have no effect (won't actually flush).  I had put this hack in my tree to work 
around this:  (comment out mInScript check)

NS_IMETHODIMP
HTMLContentSink::FlushPendingNotifications()
{
  nsresult result = NS_OK;
  // Only flush tags if we're not doing the notification ourselves
  // (since we aren't reentrant) and if we're in a script (since 
  // we only care to flush if this is done via script).
  if (mCurrentContext && !mInNotification) { // && mInScript) {
    result = mCurrentContext->FlushTags();
  }

  return result;
}

It's probably wrong, but I haven't yet figured out what to do instead yet.  So 
it's not done quite yet.

Comment 36

[Eric Pollmann]

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

Comment 37

[Eric Pollmann]

Test against bug 42178

Comment 38

[Eric Pollmann]

Attachment: test case

Comment 39

[Eric Pollmann]

Fix checked in.

To verify, view the final test case.  You should see a text input that says "Set 
the value!"  I you see this, the bug was fixed, thanks!

Comment 40

[Chris Petersen]

Fixed in the July 10th build.