ReplaceSelectionCommand::doApply() crashes if input element is disabled. This won't happen if a command is invoked from execCommand or something though, because the command invocation is prevented if an input element is disabled. Repro on chromium: 1. Open a page with <input disabled value=foobar> 2. Right click on the word "foobar" 3. Correct it to "footer" http://crbug.com/109622
Created attachment 121806 [details] Patch
(In reply to comment #0) > This won't happen if a command is invoked from execCommand or something though, because the command invocation is prevented if an input element is disabled. Don't non-Chromium ports have this problem? If not, we should put the test to LayoutTests/platform/chromium/.
(In reply to comment #0) > This won't happen if a command is invoked from execCommand or something though, because the command invocation is prevented if an input element is disabled. Can we add similar check to WebFrameImpl::replaceSelection()?
(In reply to comment #2) > (In reply to comment #0) > > This won't happen if a command is invoked from execCommand or something though, because the command invocation is prevented if an input element is disabled. > > Don't non-Chromium ports have this problem? > If not, we should put the test to LayoutTests/platform/chromium/. This crash might happen, but in the other ports currently some checks seems performed before calling replaceSelection. We should add LaytoutTestController::replaceSelection in the other ports to perform tests, but I just have prioritized fixing chrome crash problem.
Created attachment 121832 [details] Patch
Comment on attachment 121832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121832&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1127 > + if (!frame()->selection()->isContentEditable()) > + return; > + This check should be done inside Editor::replaceSelectionWithText > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:809 > +void LayoutTestController::replaceSelection(const CppArgumentList& arguments, CppVariant* result) > +{ > + if (arguments.size() <= 0 || !arguments[0].isString()) { > + result->setNull(); > + return; > + } > + > + std::string text = arguments[0].toString(); > + m_shell->webView()->focusedFrame()->replaceSelection(WebString::fromUTF8(text)); > +} I'm not certain it's such a good idea to add a new method on layoutTestController for this one specific test. It's probably okay not to add a test for this fix or add a manual test.
Created attachment 122144 [details] Patch
> I'm not certain it's such a good idea to add a new method on layoutTestController for this one specific test. It's probably okay not to add a test for this fix or add a manual test. I've added a manual test.
Comment on attachment 122144 [details] Patch r+, seeing Ryosuke's points are addressed in the latest patch.
Comment on attachment 122144 [details] Patch Clearing flags on attachment: 122144 Committed r105050: <http://trac.webkit.org/changeset/105050>
All reviewed patches have been landed. Closing bug.