Closed Bug 240633 Opened 21 years ago Closed 20 years ago

[FIX]document.write triggers nsHTMLDocument::ResolveName

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

()

Details

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

Attachments

(2 files)

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 fixSplinter 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

Created:
Updated:
Size: