[webkit-reviews] review denied: [Bug 120328] Don't keep unassociated elements in the past names map : [Attachment 209724] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 120328: Don't keep unassociated elements in the past names map
https://bugs.webkit.org/show_bug.cgi?id=120328

Attachment 209724: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=209724&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.