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

VERIFIED FIXED in M17

Status

()

Core
DOM: Core & HTML
P3
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: Akkana Peck, Assigned: Eric Pollmann)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2+] fix in hand, URL)

Attachments

(4 attachments)

(Reporter)

Description

19 years ago
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).

Updated

19 years ago
Assignee: troy → pollmann

Comment 1

19 years ago
Eric, forms related

Comment 2

19 years ago
Linux specific, or all platforms?
(Reporter)

Updated

19 years ago
OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 3

19 years ago
All platforms.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M6
(Assignee)

Comment 4

19 years ago
Redistributing to M8...

Comment 5

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

Comment 6

19 years ago
Didn't make M8
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WORKSFORME
Target Milestone: M10 → M15
(Assignee)

Comment 7

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

Updated

18 years ago
Status: RESOLVED → REOPENED
(Assignee)

Updated

18 years ago
Resolution: WORKSFORME → ---
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 8

18 years ago
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.
(Assignee)

Comment 9

18 years ago
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.
Target Milestone: M13 → M16
Moving to M16. Ugly but doesn't prevent use of page.
(Assignee)

Updated

18 years ago
Component: Layout → DOM Level 0
Summary: text field shows up twice if contents set through JS → document.forms[x] not defined, text field shows up twice if contents set through JS
(Assignee)

Comment 11

18 years ago
Hey Vidur, here's another backwards compatability issue - wondered if you had
any thoughts on it?
(Assignee)

Comment 12

18 years ago
Rescheduling
Target Milestone: M16 → M19
(Reporter)

Comment 13

18 years ago
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? 
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.
Keywords: nsbeta2
(Assignee)

Comment 15

18 years ago
Text field no longer shows up twice.
Summary: document.forms[x] not defined, text field shows up twice if contents set through JS → document.forms[x] not defined
(Assignee)

Comment 16

18 years ago
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.
Summary: document.forms[x] not defined → document.forms[x] not defined if form is document.written out
(Assignee)

Comment 17

18 years ago
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.
Whiteboard: fix in hand
Target Milestone: M19 → M17
(Assignee)

Comment 18

18 years ago
Created attachment 8856 [details] [diff] [review]
Preliminary fix
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.
(Assignee)

Comment 20

18 years ago
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.
Whiteboard: fix in hand
(Assignee)

Comment 21

18 years ago
Created attachment 8888 [details]
test case
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?
(Assignee)

Comment 23

18 years ago
> 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?
(Assignee)

Comment 24

18 years ago
See also bug 32022, which is related.

Comment 25

18 years ago
Sent over to rickg for PDT call.
Whiteboard: [NEED INFO]

Comment 26

18 years ago
I think it's time to nail this one down; approving for nsbeta2. 
Eric: flushing the sink after every document.write() would be unwise.
Whiteboard: [NEED INFO] → [NEED INFO] [nsbeta2+]

Comment 27

18 years ago
Removing [NEED INFO] from Status Whiteboard.
Whiteboard: [NEED INFO] [nsbeta2+] → [nsbeta2+]
(Assignee)

Comment 28

18 years ago
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?
(Assignee)

Comment 29

18 years ago
<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.
(Assignee)

Comment 30

18 years ago
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.
Whiteboard: [nsbeta2+] → [nsbeta2+] fix in hand
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...
(Assignee)

Comment 32

18 years ago
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.
(Assignee)

Comment 33

18 years ago
Created attachment 10282 [details] [diff] [review]
proposed fix
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...
(Assignee)

Comment 35

18 years ago
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.
(Assignee)

Comment 36

18 years ago
*** Bug 40019 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 37

18 years ago
Test against bug 42178
(Assignee)

Comment 38

18 years ago
Created attachment 11130 [details]
test case
(Assignee)

Comment 39

18 years ago
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 40

18 years ago
Fixed in the July 10th build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.