Bug 75941 - [crash] Renderer crashes when spell checking a disabled input field.
Summary: [crash] Renderer crashes when spell checking a disabled input field.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 23:39 PST by Shinya Kawanaka
Modified: 2012-01-16 03:31 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.31 KB, patch)
2012-01-10 00:44 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2012-01-10 05:15 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (3.75 KB, patch)
2012-01-11 17:39 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-01-09 23:39:58 PST
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
Comment 1 Shinya Kawanaka 2012-01-10 00:44:08 PST
Created attachment 121806 [details]
Patch
Comment 2 Kent Tamura 2012-01-10 00:52:46 PST
(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/.
Comment 3 Kent Tamura 2012-01-10 01:21:11 PST
(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()?
Comment 4 Shinya Kawanaka 2012-01-10 05:06:57 PST
(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.
Comment 5 Shinya Kawanaka 2012-01-10 05:15:31 PST
Created attachment 121832 [details]
Patch
Comment 6 Ryosuke Niwa 2012-01-10 10:29:52 PST
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.
Comment 7 Shinya Kawanaka 2012-01-11 17:39:37 PST
Created attachment 122144 [details]
Patch
Comment 8 Shinya Kawanaka 2012-01-11 17:40:21 PST
> 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 9 Hajime Morrita 2012-01-16 02:03:53 PST
Comment on attachment 122144 [details]
Patch

r+, seeing Ryosuke's points are addressed in the latest patch.
Comment 10 WebKit Review Bot 2012-01-16 03:31:53 PST
Comment on attachment 122144 [details]
Patch

Clearing flags on attachment: 122144

Committed r105050: <http://trac.webkit.org/changeset/105050>
Comment 11 WebKit Review Bot 2012-01-16 03:31:58 PST
All reviewed patches have been landed.  Closing bug.