Bug 101215 – [WK2][EFL][GTK] early return of checkSpellingOfString treats correct words as misspelled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Grzegorz Czajkowski]

Patch from bug 93611 which enables the spelling setting for WebKitTestRunner causes regressions for WebKit2-EFL debug build.
It's related to WK2 only as it (exactly WebProcess) passes references of misspellingLocation and misspellingLength to the UIProcess and it expects (I assume) that those values have to be updated in the UIProcess. In the other case those values store garbage :)
You can see this that code in WebEditorClient.cpp:

void WebEditorClient::checkSpellingOfString(const UChar* text, int length, int* misspellingLocation, int* misspellingLength)
{
    int32_t resultLocation = -1;
    int32_t resultLength = 0;
    // FIXME: It would be nice if we wouldn't have to copy the text here.
    m_page->sendSync(Messages::WebPageProxy::CheckSpellingOfString(String(text, length)),
        Messages::WebPageProxy::CheckSpellingOfString::Reply(resultLocation, resultLength));
    *misspellingLocation = resultLocation;
    *misspellingLength = resultLength;
}

First receiver (WebPageProxy::checkSpellingOfString) doesn't receive -1 and 0 as it was assumed but the random values. If nobody updates them those unexpected values will be used in WebCore and they cause assertions.

There are (at least) two ways of solving this issue:
1) Fix CoreIPC mechanism (not sure if it's possible) to have a possibility to receive the default values (-1, 0) by UIProcess.
2) Fix TextCheckerEnchant::checkSpellingOfString by saving at the beginning of the method the default values (-1, 0) to its output parameters.

Any opinions?

[Grzegorz Czajkowski]

Created attachment 172534 [details]
fix checkSpellingOfString in TextCheckerEnchant class

[Gustavo Noronha (kov)]

Comment on attachment 172534 [details]
fix checkSpellingOfString in TextCheckerEnchant class

View in context: https://bugs.webkit.org/attachment.cgi?id=172534&action=review

As I said on IRC, this makes sense to me, I'd recommend removing the assingment from the original WebProcess call site to make it clearer that those values do not get here.

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:71
> +    // Assume that the words in the @a string are spelled correctly.

This @a in the comment looks like a formatting annotation which doesn't make sense in private/cross-platform comments.

[Grzegorz Czajkowski]

Created attachment 172559 [details]
apply Gustavo's suggestions - patch for landing

[Grzegorz Czajkowski]

(In reply to comment #2)
> (From update of attachment 172534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172534&action=review
> 
> As I said on IRC, this makes sense to me, I'd recommend removing the assingment from the original WebProcess call site to make it clearer that those values do not get here.

Thanks for review. In my opinions those initializations are needed to do not make compilation warnings. I completely agree that they are never seen in UIProcess.

As we discussed on IRC it'd be good to initialize them in WebPageProxy to fix that potential issue for other ports too.  

> 
> > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:71
> > +    // Assume that the words in the @a string are spelled correctly.
> 
> This @a in the comment looks like a formatting annotation which doesn't make sense in private/cross-platform comments.

Right. Removed '@a'.

[Grzegorz Czajkowski]

Comment on attachment 172559 [details]
apply Gustavo's suggestions - patch for landing

Clearing flags on attachment: 172559

Committed r133602: <http://trac.webkit.org/changeset/133602>

[Grzegorz Czajkowski]

All reviewed patches have been landed.  Closing bug.