Closed Bug 240633 Opened 20 years ago Closed 20 years ago

[FIX]document.write triggers nsHTMLDocument::ResolveName

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: perf, Whiteboard: The childNodes changes were in bug 121495)

Attachments

(2 files)

Proposed fix
7.49 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
Additional patch to make this not crash in some cases
1.43 KB, patch
Details | Diff | Splinter Review 
It seems that when a script does

  document.write('foo')

we end up inside nsHTMLDocument::ResolveName looking for "write".  Is that the
behavior we want?  If yes, that's fine, but it seemed a little odd to me.

The fact that nsHTMLDocument::ResolveName has to flush makes loading the page in
the URL field pretty nastily slow....
Keywords: perf
> jst: so...  when I do document.write, should we end up looking for nodes with
name="write"?
<jst> bz: yeah
<jst> bz: see nsHTMLDocument::PrePopulateHashTables()
> jst: looking
> jst: hmm... so if there is such an element then it'll actually override
document.write?
<jst> bz: no, it would, if it wasn't for that code
> ah
> jst: here is the problem
> jst: we _know_ we're not going to get a real element
> jst: but we flush the content sink anyway
> jst: or rather, we _could_ know that we won't get a real element....
<jst> bz: true, we could do a hash lookup before flushing
<jst> bz: and if we find an empty list, don't flush, just move on...
> jst: yeah... lemme try that....
<jst> bz: though theoretically that might break too, if an element is removed
before we hit that code, we could have an empty list in the hash, and the sink
could have more that it didn't feed us yet
<botbot> Firefox: 'Linux redwood Depend' has changed state from Horked to Success.
> hrm....
> maybe we need a better way of handling the "special" names?
> than sticking them into the table?
<jst> bz: maybe
<jst> bz: or maybe we need another special entry in the hash, and not an empty list?
> jst: also possible, yes...
> jst: Just having null list and null id is no good, right?
> jst: which is not the same as an empty list, note
<jst> bz: should be doable, unless there are assumptions in the code that are
hard to fix
Whiteboard: The childNodes changes were in bug 121495
By the way, the following trivial code breaks the code we have now:

<body onload="document.body.firstChild.setAttribute('name', 'write');
              alert(document.write);"><form></form>
</body>

This will clobber document.write with the form node.  So we need a better
solution to keep track of the non-replaceable things anyway.  Maybe a magic
value like ID_NOT_IN_DOCUMENT?
Attached patch Proposed fix — — Splinter Review 
The idea here is that we can still have something with id="write", and we don't
want to block that.  So setting both name and id to null won't work.  This just
uses a magic value for "not a valid name".

On the testcase in the URL field this reduces pageload by about 10-12% (6-7
seconds).  There seem to mostly be bugs filed for the other things that are
showing up in the profile (deCOM of fontmetrics, etc).
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: document.write triggers nsHTMLDocument::ResolveName → [FIX]document.write triggers nsHTMLDocument::ResolveName
Target Milestone: --- → mozilla1.8alpha
Comment on attachment 147853 [details] [diff] [review]
Proposed fix

I did check that this does not regress bug 69826.
Attachment #147853 - Flags: superreview?(jst)
Attachment #147853 - Flags: review?(jst)
Comment on attachment 147853 [details] [diff] [review]
Proposed fix

r+sr=jst
Attachment #147853 - Flags: superreview?(jst)
Attachment #147853 - Flags: superreview+
Attachment #147853 - Flags: review?(jst)
Attachment #147853 - Flags: review+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
If the table is modified by the flush, our pointer can become invalid...  We
need to handle that case.  I just checked this in, with r+sr=brendan.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: