588990 - Remove lazy hashing of names to avoid walking full DOM

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We currently don't store elements in the name table until someone has looked up a given name. However this means that once someone looks up a specific name for the first time, we can't check the hash for it, but have to walk the full DOM.

It should be more efficient to simply hash all names as they come in. Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jonas, did this patch ever happen?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

This has been sitting in my tree for a while and should work. However it needs tests so not requesting review just yet.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This'll do it. I found two other bugs while writing these tests:

1. Bug 307415
2. Bug 649415

Comment 4

[(no longer active)]

Comment on attachment 525441 [details] [diff] [review]
Patch to fix

r=me on the specialpowers patch, which I'm going to use in my patch in bug 650543.

Also, could you please let extensions/spellcheck/hunspell/tests/suggestiontest/Makefile.orig remain in the tree?  :-)

Comment 5

[(no longer active)]

Landed the gc API: http://hg.mozilla.org/mozilla-central/rev/61ab7fa02e26

Comment 6

[Boris Zbarsky [:bzbarsky]]

Why is nsHTMLDocument::ResolveName using PutEntry instead of GetEntry?

This patch changes our behavior on this testcase:

  data:text/html,<img name="write"><script>alert(document.write)</script>

but I _think_ it changes towards better compat with other browsers... Please check IE?

Is there a point to populating the name map for non-HTML documents?  If not, is it worth optimizing out the add/remove bits for non-HTML documents (I realize we want them for XHTML, so mIsRegularHTML is the wrong thing to check, but...)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #6)
> Why is nsHTMLDocument::ResolveName using PutEntry instead of GetEntry?

When I made similar code refactoring for the id code I tried changing PutEntry to GetEntry. However that caused a measurable slowdown on the relevant Dromaeo tests. Apparently our hash table is faster when a lookup succeeds. 

> This patch changes our behavior on this testcase:
> 
>   data:text/html,<img name="write"><script>alert(document.write)</script>
> 
> but I _think_ it changes towards better compat with other browsers... Please
> check IE?

I checked with other browsers and no others had the same behavior as us. IIRC this patch aligned us with everyone else, and definitely with IE.

> Is there a point to populating the name map for non-HTML documents?  If not,
> is it worth optimizing out the add/remove bits for non-HTML documents (I
> realize we want them for XHTML, so mIsRegularHTML is the wrong thing to
> check, but...)

We could, I don't feel strongly. It does seem somewhat unlikely that there will be significant amounts of named non-form-control HTML contents in non-HTML documents (note that no form controls show up in the CanHaveName list).

Comment 8

[Boris Zbarsky [:bzbarsky]]

> However that caused a measurable slowdown on the relevant Dromaeo tests.

See bug 572614 comment 2 and bug 572614 comment 3.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There you go :) I'll switch to GetEntry then :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 525441 [details] [diff] [review]
Patch to fix

OK, r=me with the change to GetEntry and corresponding comment adjustments.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like you could have removed the NAME_NOT_VALID define?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in. Thanks for the review!

Of course, I forgot to remove the part of the patch that killed Makefile.orig, so I had to land a followup which added it back.

http://hg.mozilla.org/mozilla-central/rev/ea254208e04c
http://hg.mozilla.org/mozilla-central/rev/de123d952abd