[FIX]document.write triggers nsHTMLDocument::ResolveName

RESOLVED FIXED in mozilla1.8alpha1

Status

()

P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({perf})

Trunk
mozilla1.8alpha1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

(Assignee)

Description

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

Updated

15 years ago
Keywords: perf
(Assignee)

Comment 1

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

Updated

15 years ago
Whiteboard: The childNodes changes were in bug 121495
(Assignee)

Comment 2

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

Comment 3

15 years ago
Created attachment 147853 [details] [diff] [review]
Proposed fix

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
(Assignee)

Updated

15 years ago
Priority: -- → P1
Summary: document.write triggers nsHTMLDocument::ResolveName → [FIX]document.write triggers nsHTMLDocument::ResolveName
Target Milestone: --- → mozilla1.8alpha
(Assignee)

Comment 4

15 years ago
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+
(Assignee)

Comment 6

15 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

15 years ago
Created attachment 147971 [details] [diff] [review]
Additional patch to make this not crash in some cases

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.
You need to log in before you can comment on or make changes to this bug.