Bug 120328 – Don't keep unassociated elements in the past names map

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

It doesn't make much sense to keep elements accessible via the name getter of a form element if they're no longer associated with the form element.

I've proposed the change in the specification:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-August/040586.html

Since I expect this bug to have a very low-compatibilty concern, I'm going to pre-emptively post a patch here pending the specification change.

[Ryosuke Niwa]

Created attachment 209688 [details]
Demo

[Ryosuke Niwa]

Created attachment 209716 [details]
Work in progress

[Ryosuke Niwa]

Created attachment 209724 [details]
Fixes the bug

[Build Bot]

Comment on attachment 209724 [details]
Fixes the bug

Attachment 209724 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1589222

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209740 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Darin Adler]

Comment on attachment 209724 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=209724&action=review

> Source/WebCore/html/HTMLFormElement.cpp:503
> +    removeElementFromPastNamesMap(static_cast<HTMLFormControlElement*>(e));

What makes this type cast safe? I am really concerned about this. If the argument is always an HTMLFormControlElement, then the function signature should change. If not, then this can do a bad cast.

> Source/WebCore/html/HTMLFormElement.cpp:636
> +bool HTMLFormElement::elementCanBeInPastNamesMap(HTMLElement* element) const

Since the point of this is to be used in assertions, it should do the assertions, not return a boolean. We don’t want to have to guess about which check failed.

> Source/WebCore/html/HTMLFormElement.cpp:639
> +    if (!element->refCount() || element->form() != this)
> +        return false;

I don’t understand the !element->refCount() check here. Needs a why comment because it’s so unusual to do a check like that. Remember that elements can have a reference count of zero but be kept alive because they have a parent that is non-zero. This might be correct, but I am not sure it is, especially without a comment.

> Source/WebCore/html/HTMLFormElement.cpp:656
> +    PastNamesMap::iterator end = m_pastNamesMap->end();
> +    for (PastNamesMap::iterator it = m_pastNamesMap->begin(); it != end; ++it) {
> +        if (it->value == element)
> +            it->value = 0; // Don't break. Single element can have multiple names.
> +    }

It’s a theoretical problem, at least, that this is O(n) the total number of past names. We would normally use a data structure that makes removal fast. Typically we’d use a map that goes in both directions, although I can see that would be a bit complex.

If O(n) is OK we should at least have a comment explaining why.

> Source/WebCore/html/HTMLFormElement.cpp:672
> +        addElementToPastNamesMap(static_cast<HTMLElement*>(namedItems.first().get()), name);

What guarantees that this typecast is safe? If namedItems contains only HTML elements, then it should be typed that way.

> Source/WebCore/html/HTMLFormElement.h:147
> +#ifndef NDEBUG
> +    bool elementCanBeInPastNamesMap(HTMLElement*) const;
> +#else
> +    inline bool elementCanBeInPastNamesMap(HTMLElement*) const { return true; }
> +#endif

I strongly prefer that this inline NDEBUG empty function definition be done separately after the class. It’s easier to read if this just has the declaration, and the one after the class can still be marked inline.

[Ryosuke Niwa]

Comment on attachment 209724 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=209724&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:503
>> +    removeElementFromPastNamesMap(static_cast<HTMLFormControlElement*>(e));
> 
> What makes this type cast safe? I am really concerned about this. If the argument is always an HTMLFormControlElement, then the function signature should change. If not, then this can do a bad cast.

removeElementFromPastNamesMap takes either HTMLFormControlElement, HTMLImageElement, or HTMLObjectElement.
But you're right, e could be an object element so I need to check that.

>> Source/WebCore/html/HTMLFormElement.cpp:639
>> +        return false;
> 
> I don’t understand the !element->refCount() check here. Needs a why comment because it’s so unusual to do a check like that. Remember that elements can have a reference count of zero but be kept alive because they have a parent that is non-zero. This might be correct, but I am not sure it is, especially without a comment.

Hm... actually, this check might be bogus because of image element case. Will fix.

>> Source/WebCore/html/HTMLFormElement.cpp:656
>> +    }
> 
> It’s a theoretical problem, at least, that this is O(n) the total number of past names. We would normally use a data structure that makes removal fast. Typically we’d use a map that goes in both directions, although I can see that would be a bit complex.
> 
> If O(n) is OK we should at least have a comment explaining why.

removeFromVector called in removeFormElement and removeImgElement is already O(n).

>> Source/WebCore/html/HTMLFormElement.cpp:672
>> +        addElementToPastNamesMap(static_cast<HTMLElement*>(namedItems.first().get()), name);
> 
> What guarantees that this typecast is safe? If namedItems contains only HTML elements, then it should be typed that way.

Yes, namedItems only contain HTML elements but it's hard to change the type due to dependencies.  I'll add a FIXME.

>> Source/WebCore/html/HTMLFormElement.h:147
>> +#endif
> 
> I strongly prefer that this inline NDEBUG empty function definition be done separately after the class. It’s easier to read if this just has the declaration, and the one after the class can still be marked inline.

Makes sense.

[Ryosuke Niwa]

Created attachment 209839 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 209840 [details]
Reverted unintentional changes to HTMLObjectElement.h

[Darin Adler]

Comment on attachment 209840 [details]
Reverted unintentional changes to HTMLObjectElement.h

View in context: https://bugs.webkit.org/attachment.cgi?id=209840&action=review

> Source/WebCore/html/HTMLFormElement.cpp:520
> +    removeFromPastNamesMap(e->asFormNamedItem());

The call to asFormNamedItem should not be needed here. Passing the variable e directly should work; HTMLImageElement is derived from FormNamedItem.

> Source/WebCore/html/HTMLFormElement.cpp:632
> +{ }

I’d prefer to see these on consecutive lines.

> Source/WebCore/html/HTMLFormElement.cpp:665
> +            it->value = 0; // Don't break. Single element can have multiple names.

I think “break” is a little unclear in this comment. I would say “Keep looping” rather than “Don't break”.

> Source/WebCore/html/HTMLFormElement.cpp:682
> +        addToPastNamesMap(toHTMLElement(namedItems.first().get())->asFormNamedItem(), name); // FIXME: Use RefPtr<Element> in namedItems

What guarantees that namedItems.first() is an HTMLElement?

Also, what does the FIXME mean? If we are going to use Element, then maybe asFormNamedItem should be moved to Element instead of put in HTMLElement.

[Ryosuke Niwa]

Comment on attachment 209840 [details]
Reverted unintentional changes to HTMLObjectElement.h

View in context: https://bugs.webkit.org/attachment.cgi?id=209840&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:682
>> +        addToPastNamesMap(toHTMLElement(namedItems.first().get())->asFormNamedItem(), name); // FIXME: Use RefPtr<Element> in namedItems
> 
> What guarantees that namedItems.first() is an HTMLElement?
> 
> Also, what does the FIXME mean? If we are going to use Element, then maybe asFormNamedItem should be moved to Element instead of put in HTMLElement.

elements()->namedItems() never put non-HTMLElement into namedItems so I meant to say to use Use RefPtr<HTMLElement>.

[Ryosuke Niwa]

Committed r154761: <http://trac.webkit.org/changeset/154761>