Closed
Bug 240633
Opened 21 years ago
Closed 20 years ago
[FIX]document.write triggers nsHTMLDocument::ResolveName
Categories
(Core :: DOM: Core & HTML, defect, P1)
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)
7.49 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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....
Assignee | ||
Comment 1•21 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•21 years ago
|
Whiteboard: The childNodes changes were in bug 121495
Assignee | ||
Comment 2•21 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•20 years ago
|
||
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•20 years ago
|
Priority: -- → P1
Summary: document.write triggers nsHTMLDocument::ResolveName → [FIX]document.write triggers nsHTMLDocument::ResolveName
Target Milestone: --- → mozilla1.8alpha
Assignee | ||
Comment 4•20 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 5•20 years ago
|
||
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•20 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•20 years ago
|
||
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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•