Bug 34930

Created attachment 48728 [details] Proposed patch

Attachment 48728 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/266857

(In reply to comment #2) > Attachment 48728 [details] did not build on chromium: > Build output: http://webkit-commit-queue.appspot.com/results/266857 > ../../WebCore/html/HTMLFormElement.cpp:319: error: invalid use of incomplete type ‘struct WebCore::DOMWindow’ I'll fix this on the next patch update or landing.

Comment on attachment 48728 [details] Proposed patch This looks reasonable, but you need to spin up a new patch because this break the Chromium build and there are few nits: + contorol ^^ Typo + unsigned i We usually use int for these sorts of loops. + message.replace("%name", unhandled->name()); Please use String::format() I'm also not super excited about adding a bunch of random arguments to these functions. Can we abstract this better somehow? Also, the loop in HTMLFormElement::prepareSubmit is pretty complicated, mostly just so we can print more messages to the console. Can we simplify this somehow? Sorry for the delay in reviewing your patch (and the lame review).

(In reply to comment #4) > + message.replace("%name", unhandled->name()); > > Please use String::format() I'm not sure that's better. A replace is safer and possibly faster. And String::format doesn't handle non-ASCII characters well.

Created attachment 51414 [details] Proposed patch (rev.2)

Thank you for reviewing! (In reply to comment #4) > + contorol > ^^ Typo Fixed. > + unsigned i > We usually use int for these sorts of loops. I don't agree with it. 'int' makes a warning about signed vs. unsigned in this case, and the number of "for (unsigned" is greater than "for (size_t" in WebCore. > I'm also not super excited about adding a bunch of random arguments to these > functions. Can we abstract this better somehow? I have no good idea. As for the additional parameter of checkValidity(), it was planed when checkValidity() was added. The proposed patch for the original checkValidity() had the parameter for the preparation of this change, but it was removed during the code review. > Also, the loop in > HTMLFormElement::prepareSubmit is pretty complicated, mostly just so we can > print more messages to the console. Can we simplify this somehow? I improved it a little.

(In reply to comment #5) > (In reply to comment #4) > > + message.replace("%name", unhandled->name()); > > > > Please use String::format() > > I'm not sure that's better. A replace is safer and possibly faster. And > String::format doesn't handle non-ASCII characters well. Ah, interesting. The other parts of the codebase I've seen have used format, but it's good to understand the trade-offs.

Created attachment 52630 [details] Proposed patch (rev.3)

(In reply to comment #9) > Created an attachment (id=52630) [details] > Proposed patch (rev.3) - Fixed whitespace in a comment

Comment on attachment 52630 [details] Proposed patch (rev.3) > - form()->prepareSubmit(evt); > + form()->prepareSubmit(evt, this); Why is this needed? I would think that either event->target() or event->currentTarget() would give us the element. > -bool HTMLFormControlElement::checkValidity() > +bool HTMLFormControlElement::checkValidity(Vector<HTMLFormControlElement*>* unhandledInvalidControls) This vector should hold RefPtr<HTMLFormControlElement> instead of a raw pointer. Dispatching to arbitrary JavaScript code could easily make these elements go away, so we can't just hold a raw pointer to them. We should construct test cases with tricky event handlers for the invalid event to see if we can reproduce these kinds of bad behavior. It's critical we not introduce something that could be exploited as a security vulnerability. > - dispatchEvent(Event::create(eventNames().invalidEvent, false, true)); > + bool needsDefaultAction = dispatchEvent(Event::create(eventNames().invalidEvent, false, true)); > + if (needsDefaultAction && unhandledInvalidControls) > + unhandledInvalidControls->append(this); What guarantees that "this" is still good? I think it's possible that the event dispatch could remove the last reference to this form control element and so it could be deallocated by the time we return. Same comment as above. > + HTMLFormControlElement* unhandled = unhandledInvalidControls[i]; > + if (unhandled->isFocusable()) { > + unhandled->scrollIntoViewIfNeeded(false); > + unhandled->focus(); > + break; > + } We should think through the case where this control is no longer in the document, or not visible. > + // Warn about all of unforcusable controls. Typo: "unforcusable". > + Frame* frame = document()->frame(); Scrolling and focusing may have triggered an event handler, which may have deleted this form element. So calling document() at this point may not work. Need to double check that we handle that case. Similar issue with the code later on that sets m_insubmit to false. > + for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) { > + HTMLFormControlElement* unhandled = unhandledInvalidControls[i]; > + if (unhandled->isFocusable()) > + continue; > + String message("An invalid form control with name='%name' is not user-editable."); > + message.replace("%name", unhandled->name()); > + frame->domWindow()->console()->addMessage(HTMLMessageSource, LogMessageType, ErrorMessageLevel, message, 0, document()->url().string()); > + } I don't completely understand the goal here. You say "not user-editable" even though the code checks "isFocusable". Could you give a concrete exmple so I can understand why this is useful? > + return !unhandledInvalidControls.size(); I think this would read better if you used the isEmpty function.

Thank you for reviewing! Your review is definitely helpful. (In reply to comment #11) > (From update of attachment 52630 [details]) > > - form()->prepareSubmit(evt); > > + form()->prepareSubmit(evt, this); > > Why is this needed? I would think that either event->target() or > event->currentTarget() would give us the element. You're right. Removed the parameter. > > -bool HTMLFormControlElement::checkValidity() > > +bool HTMLFormControlElement::checkValidity(Vector<HTMLFormControlElement*>* unhandledInvalidControls) > > This vector should hold RefPtr<HTMLFormControlElement> instead of a raw > pointer. Dispatching to arbitrary JavaScript code could easily make these > elements go away, so we can't just hold a raw pointer to them. I see. I have added a test which removed a node in an event handler, and confirmed it caused a crash. I changed it to RefPtr<HTMLFormControlElement>. > > - dispatchEvent(Event::create(eventNames().invalidEvent, false, true)); > > + bool needsDefaultAction = dispatchEvent(Event::create(eventNames().invalidEvent, false, true)); > > + if (needsDefaultAction && unhandledInvalidControls) > > + unhandledInvalidControls->append(this); > > What guarantees that "this" is still good? I think it's possible that the event > dispatch could remove the last reference to this form control element and so it > could be deallocated by the time we return. Same comment as above. Added protections for 'this' by RefPtr. > > + HTMLFormControlElement* unhandled = unhandledInvalidControls[i]; > > + if (unhandled->isFocusable()) { > > + unhandled->scrollIntoViewIfNeeded(false); > > + unhandled->focus(); > > + break; > > + } > > We should think through the case where this control is no longer in the > document, or not visible. Added another check for isFocusable() before focus(). > > + // Warn about all of unforcusable controls. > > Typo: "unforcusable". Fixed. > > > + Frame* frame = document()->frame(); > > Scrolling and focusing may have triggered an event handler, which may have > deleted this form element. So calling document() at this point may not work. > Need to double check that we handle that case. Similar issue with the code > later on that sets m_insubmit to false. Added protection for the form element by RefPtr. > > + for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) { > > + HTMLFormControlElement* unhandled = unhandledInvalidControls[i]; > > + if (unhandled->isFocusable()) > > + continue; > > + String message("An invalid form control with name='%name' is not user-editable."); > > + message.replace("%name", unhandled->name()); > > + frame->domWindow()->console()->addMessage(HTMLMessageSource, LogMessageType, ErrorMessageLevel, message, 0, document()->url().string()); > > + } > > I don't completely understand the goal here. You say "not user-editable" even > though the code checks "isFocusable". Could you give a concrete exmple so I can > understand why this is useful? I changed "not user-editable" to "not focusable". It was confusing. HTML5 spec asks to warn about this case. It is useful for page authors to notify that users can not make this form valid. > > + return !unhandledInvalidControls.size(); > > I think this would read better if you used the isEmpty function. Fixed.

Created attachment 52689 [details] Proposed patch (rev.4)

Created attachment 52691 [details] Proposed patch (rev.5)

Comment on attachment 52691 [details] Proposed patch (rev.5) Getting better, but I still think this needs work. > + if (needsDefaultAction && unhandledInvalidControls && inDocument()) > + unhandledInvalidControls->append(this); Besides checking inDocument() I think you might want to check that it's still in the same document, and hasn't been moved into another. > + // Interactive validation must be done before dispatching the submit event. > + HTMLFormControlElement* submitElement = 0; > + Node* targetNode = event->target()->toNode(); > + if (targetNode && targetNode->isElementNode()) { > + Element* targetElement = static_cast<Element*>(targetNode); > + if (targetElement->isFormControlElement()) > + submitElement = static_cast<HTMLFormControlElement*>(targetElement); > + } Seems like all that logic could go after the !noValidate() check, inside an if statement. > + ASSERT(unhandled->hasOneRef()); This assertion is wrong. How can you be sure this has exactly one ref? I think perhaps you misunderstand the meaning of the hasOneRef function. There's really no way to assert that the pointer is in a RefPtr, but there's no need to assert that either. > + if (unhandled->isFocusable()) { > + unhandled->scrollIntoViewIfNeeded(false); > + // scrollIntoViewIfNeeded() dispatches events, so the state > + // of 'unhandled' might be changed. > + if (unhandled->isFocusable()) { > + unhandled->focus(); > + break; > + } > + } I think you should go further and say "the state of 'unhandled' might be changed so it's no longer focusable." But further, is isFocusable the only check you need to make? Don't we need to check other things like inDocument, too? Probably also need to check it's still in the same document and hasn't been moved into another. > + // Warn about all of unforcusable controls. Typo: "unforcusable". > +bool HTMLFormElement::checkValidity(Vector<RefPtr<HTMLFormControlElement> >& unhandledInvalidControls) > +{ > + RefPtr<HTMLFormElement> protector(this); > for (unsigned i = 0; i < formElements.size(); ++i) { > HTMLFormControlElement* control = formElements[i]; > + control->checkValidity(&unhandledInvalidControls); > } > + return unhandledInvalidControls.isEmpty(); > } The name "checkValidity" doesn't seem quite right for this function. I would call it something more like collectUnhandledInvalidControls. I also think there's no need to have it return a boolean. The caller can check if the vector is empty or not. I don't understand, though, why these are "unhandled invalid controls" rather than just "invalid controls". What are "handled invalid controls"?

Created attachment 52717 [details] Proposed patch (rev.6)

(In reply to comment #15) > > + if (needsDefaultAction && unhandledInvalidControls && inDocument()) > > + unhandledInvalidControls->append(this); > > Besides checking inDocument() I think you might want to check that it's still > in the same document, and hasn't been moved into another. Oh, I thought a node couldn't be moved to another document. Right, Webkit supports node adopting. I added document() check. > > > + // Interactive validation must be done before dispatching the submit event. > > + HTMLFormControlElement* submitElement = 0; > > + Node* targetNode = event->target()->toNode(); > > + if (targetNode && targetNode->isElementNode()) { > > + Element* targetElement = static_cast<Element*>(targetNode); > > + if (targetElement->isFormControlElement()) > > + submitElement = static_cast<HTMLFormControlElement*>(targetElement); > > + } > > Seems like all that logic could go after the !noValidate() check, inside an if > statement. Right. Fixed. And I moved the additional code in prepareSubmit() to another function. > > + ASSERT(unhandled->hasOneRef()); > > This assertion is wrong. How can you be sure this has exactly one ref? I think > perhaps you misunderstand the meaning of the hasOneRef function. There's really > no way to assert that the pointer is in a RefPtr, but there's no need to assert > that either. I misunderstood it was refCount() > 0. Removed. > > + if (unhandled->isFocusable()) { > > + unhandled->scrollIntoViewIfNeeded(false); > > + // scrollIntoViewIfNeeded() dispatches events, so the state > > + // of 'unhandled' might be changed. > > + if (unhandled->isFocusable()) { > > + unhandled->focus(); > > + break; > > + } > > + } > > I think you should go further and say "the state of 'unhandled' might be > changed so it's no longer focusable." > > But further, is isFocusable the only check you need to make? Don't we need to > check other things like inDocument, too? Probably also need to check it's still > in the same document and hasn't been moved into another. Add document() check and updated the comment. I don't think inDocument check is required because a focusable element must be inDocument(). > > + // Warn about all of unforcusable controls. > > Typo: "unforcusable". Oops, fixed. > > +bool HTMLFormElement::checkValidity(Vector<RefPtr<HTMLFormControlElement> >& unhandledInvalidControls) > > +{ > > + RefPtr<HTMLFormElement> protector(this); > > for (unsigned i = 0; i < formElements.size(); ++i) { > > HTMLFormControlElement* control = formElements[i]; > > + control->checkValidity(&unhandledInvalidControls); > > } > > + return unhandledInvalidControls.isEmpty(); > > } > > The name "checkValidity" doesn't seem quite right for this function. I would > call it something more like collectUnhandledInvalidControls. I also think > there's no need to have it return a boolean. The caller can check if the vector > is empty or not. I changed it to "void collectUnhandledInvalidControls()". > I don't understand, though, why these are "unhandled invalid controls" rather > than just "invalid controls". What are "handled invalid controls"? It means, invalid controls of which 'invalid' event was canceled are not registered into the vector. "Unhandled invalid controls" is a term in the HTML5 specification. http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#constraint-validation

Created attachment 52900 [details] Proposed patch (rev.7)

(In reply to comment #18) > Created an attachment (id=52900) [details] > Proposed patch (rev.7) * Fixed a typo in a comment.

Comment on attachment 52900 [details] Proposed patch (rev.7) Basics look great here, but the edge cases are still not quite right. > +description('Test if the form is submitted when an "invalid" evnet for a control is canceled.'); Typo: "evnet" > + Document* originalDocument = document(); This should be a RefPtr. I don't know what guarantees the document will still be around after dispatching the event in pathological cases. There there may be some subtle guarantee I can't spot, but I think it would be best to not make the code depend on something that subtle when using a RefPtr would eliminate the risk. Also, the document == originalDocument check does not replace the inDocument check. I'm not sure why you removed the inDocument check. That covers elements that are no longer in the document tree. I think you should have a test case that covers that and restore the inDocument check. > + HTMLFormControlElement* submitElement = 0; > + Node* targetNode = event->target()->toNode(); > + if (targetNode && targetNode->isElementNode()) { > + Element* targetElement = static_cast<Element*>(targetNode); > + if (targetElement->isFormControlElement()) > + submitElement = static_cast<HTMLFormControlElement*>(targetElement); > + } Breaking this out into a helper function would make the code easier to read. Just a suggestion -- the function this code is in is getting a bit long. > + Document* originalDocument = unhandled->document(); May need to be a RefPtr for the same reason as above. Same inDocument issue as above. > + Frame* frame = document()->frame(); > + for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) { Doesn't make sense to have "frame" in the loop condition. The value of "frame" does not change as we iterate through the loop. > + RefPtr<HTMLFormElement> protector(this); > + for (unsigned i = 0; i < formElements.size(); ++i) > + formElements[i]->checkValidity(&unhandledInvalidControls); > } The protector here doesn't seem to cover all the issues. The code iterates the form elements vector while it may be changing. This means that we could end up iterating the same element more than once or skipping an element if an element is removed or added earlier in the vector than where we are in the iteration. We should figure out how we want to handle cases where the event dispatch adds or removes elements from the form. One approach would be to copy the elements into a Vector<RefPtr<...>> before we begin and iterate that.

Created attachment 52964 [details] Proposed patch (rev.8)

Created attachment 52966 [details] Proposed patch (rev.9)

Thank you for really helpful review! (In reply to comment #20) > > +description('Test if the form is submitted when an "invalid" evnet for a control is canceled.'); > Typo: "evnet" Fixed. Fixed another "evnet" too. > > + Document* originalDocument = document(); > > This should be a RefPtr. I don't know what guarantees the document will still > be around after dispatching the event in pathological cases. There there may be > some subtle guarantee I can't spot, but I think it would be best to not make > the code depend on something that subtle when using a RefPtr would eliminate > the risk. I see. Changed to RefPtr<Document>. > Also, the document == originalDocument check does not replace the inDocument > check. I'm not sure why you removed the inDocument check. That covers elements > that are no longer in the document tree. I think you should have a test case > that covers that and restore the inDocument check. Fixed. > > + HTMLFormControlElement* submitElement = 0; > > + Node* targetNode = event->target()->toNode(); > > + if (targetNode && targetNode->isElementNode()) { > > + Element* targetElement = static_cast<Element*>(targetNode); > > + if (targetElement->isFormControlElement()) > > + submitElement = static_cast<HTMLFormControlElement*>(targetElement); > > + } > > Breaking this out into a helper function would make the code easier to read. > Just a suggestion -- the function this code is in is getting a bit long. Made a function. > > + Document* originalDocument = unhandled->document(); > May need to be a RefPtr for the same reason as above. Same inDocument issue as > above. Fixed. > > + Frame* frame = document()->frame(); > > + for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) { > > Doesn't make sense to have "frame" in the loop condition. The value of "frame" > does not change as we iterate through the loop. Fixed. > > + RefPtr<HTMLFormElement> protector(this); > > + for (unsigned i = 0; i < formElements.size(); ++i) > > + formElements[i]->checkValidity(&unhandledInvalidControls); > > } > > The protector here doesn't seem to cover all the issues. The code iterates the > form elements vector while it may be changing. This means that we could end up > iterating the same element more than once or skipping an element if an element > is removed or added earlier in the vector than where we are in the iteration. > We should figure out how we want to handle cases where the event dispatch adds > or removes elements from the form. One approach would be to copy the elements > into a Vector<RefPtr<...>> before we begin and iterate that. Fixed.

Comment on attachment 52966 [details] Proposed patch (rev.9) > +static inline HTMLFormControlElement* submitElementFrom(const Event* event) I'd call it submitElementFromEvent instead of leaving the proposition hanging here. > + // Copy formElments because event handlers called from Typo: formElments.

Could still use some more test coverage.

> Also, the document == originalDocument check does not replace the inDocument > check. I'm not sure why you removed the inDocument check. That covers elements > that are no longer in the document tree. I think you should have a test case > that covers that and restore the inDocument check. I had no idea to make a condition of "isFocusable() && !inDocument()". interactive-validation-remove-node-in-handler.html removes an invalid control from the document, and it becomes !isFocusable().

(In reply to comment #24) > (From update of attachment 52966 [details]) > > +static inline HTMLFormControlElement* submitElementFrom(const Event* event) > > I'd call it submitElementFromEvent instead of leaving the proposition hanging > here. Fixed. > > + // Copy formElments because event handlers called from > Typo: formElments. Fixed. (In reply to comment #25) > Could still use some more test coverage. Filed Bug#37345. I'll commit the patch with the above fixed. Thanks!

(In reply to comment #26) > > Also, the document == originalDocument check does not replace the inDocument > > check. I'm not sure why you removed the inDocument check. That covers elements > > that are no longer in the document tree. I think you should have a test case > > that covers that and restore the inDocument check. > > I had no idea to make a condition of "isFocusable() && !inDocument()". > interactive-validation-remove-node-in-handler.html removes an invalid control > from the document, and it becomes !isFocusable(). Good point. I'm not sure that condition exists. There are 7 different isFocusable functions, so you'd have to read all of them to double check and figure out what test cases to construct.

Landed as r57346.