Summary: | [crash] Renderer crashes when spell checking a disabled input field. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | rniwa, tkent, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Shinya Kawanaka
2012-01-09 23:39:58 PST
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. |