Bug 75941 – [crash] Renderer crashes when spell checking a disabled input field.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Shinya Kawanaka]

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

[Shinya Kawanaka]

Created attachment 121806 [details]
Patch

[Kent Tamura]

(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/.

[Kent Tamura]

(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()?

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 121832 [details]
Patch

[Ryosuke Niwa]

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.

[Shinya Kawanaka]

Created attachment 122144 [details]
Patch

[Shinya Kawanaka]

> 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.

[Hajime Morrita]

Comment on attachment 122144 [details]
Patch

r+, seeing Ryosuke's points are addressed in the latest patch.

[WebKit Review Bot]

Comment on attachment 122144 [details]
Patch

Clearing flags on attachment: 122144

Committed r105050: <http://trac.webkit.org/changeset/105050>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.