Summary: | Don't keep unassociated elements in the past names map | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||
Component: | Forms | Assignee: | 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
Ryosuke Niwa
2013-08-26 15:40:06 PDT
Created attachment 209688 [details]
Demo
Created attachment 209716 [details]
Work in progress
Created attachment 209724 [details]
Fixes the bug
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 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 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 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. Created attachment 209839 [details]
Fixes the bug
Created attachment 209840 [details]
Reverted unintentional changes to HTMLObjectElement.h
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 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>. Committed r154761: <http://trac.webkit.org/changeset/154761> |