Bug 120328

Summary: Don't keep unassociated elements in the past names map
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, eoconnor, ian, rniwa, sam, tkent
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 120279    
Bug Blocks: 120432    
Attachments:
Description Flags
Demo
none
Work in progress
none
Fixes the bug
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Fixes the bug
none
Reverted unintentional changes to HTMLObjectElement.h darin: review+

Description Ryosuke Niwa 2013-08-26 15:40:06 PDT
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.
Comment 1 Ryosuke Niwa 2013-08-26 16:09:45 PDT
Created attachment 209688 [details]
Demo
Comment 2 Ryosuke Niwa 2013-08-26 23:39:59 PDT
Created attachment 209716 [details]
Work in progress
Comment 3 Ryosuke Niwa 2013-08-27 01:39:10 PDT
Created attachment 209724 [details]
Fixes the bug
Comment 4 Build Bot 2013-08-27 03:19:36 PDT
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
Comment 5 Build Bot 2013-08-27 03:19:37 PDT
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
Comment 6 Darin Adler 2013-08-27 09:44:07 PDT
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.
Comment 7 Ryosuke Niwa 2013-08-27 10:57:33 PDT
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.
Comment 8 Ryosuke Niwa 2013-08-27 20:25:31 PDT
Created attachment 209839 [details]
Fixes the bug
Comment 9 Ryosuke Niwa 2013-08-27 20:28:20 PDT
Created attachment 209840 [details]
Reverted unintentional changes to HTMLObjectElement.h
Comment 10 Darin Adler 2013-08-27 22:18:43 PDT
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.
Comment 11 Ryosuke Niwa 2013-08-27 22:32:09 PDT
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>.
Comment 12 Ryosuke Niwa 2013-08-28 11:27:45 PDT
Committed r154761: <http://trac.webkit.org/changeset/154761>