101215 2012-11-05 06:52:39 -0800 [WK2][EFL][GTK] early return of checkSpellingOfString treats correct words as misspelled 2012-11-06 07:15:22 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 93611 1 g.czajkowski g.czajkowski andersca cgarcia jberlin mario morrita oldest_to_newest 758492 0 g.czajkowski 2012-11-05 06:52:39 -0800 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? 759463 1 172534 g.czajkowski 2012-11-06 02:48:31 -0800 Created attachment 172534 fix checkSpellingOfString in TextCheckerEnchant class 759540 2 172534 gustavo 2012-11-06 04:52:40 -0800 Comment on attachment 172534 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. 759564 3 172559 g.czajkowski 2012-11-06 05:38:08 -0800 Created attachment 172559 apply Gustavo's suggestions - patch for landing 759567 4 g.czajkowski 2012-11-06 05:44:15 -0800 (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'. 759645 5 172559 g.czajkowski 2012-11-06 07:15:17 -0800 Comment on attachment 172559 apply Gustavo's suggestions - patch for landing Clearing flags on attachment: 172559 Committed r133602: <http://trac.webkit.org/changeset/133602> 759646 6 g.czajkowski 2012-11-06 07:15:22 -0800 All reviewed patches have been landed. Closing bug. 172534 2012-11-06 02:48:31 -0800 2012-11-06 05:38:08 -0800 fix checkSpellingOfString in TextCheckerEnchant class spelling-checkSpellingOfString_Fix.patch text/plain 1712 g.czajkowski ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAyYjYwNjIxLi43NTU3ZWQ5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTItMTEtMDYgIEdyemVnb3J6IEN6YWprb3dza2kgIDxnLmN6YWprb3dza2lAc2Ftc3Vu Zy5jb20+CisKKyAgICAgICAgW1dLMl1bRUZMXVtHVEtdIGVhcmx5IHJldHVybiBvZiBjaGVja1Nw ZWxsaW5nT2ZTdHJpbmcgdHJlYXRzIGNvcnJlY3Qgd29yZHMgYXMgbWlzc3BlbGxlZAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAxMjE1CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgU2F2ZSB0aGUgbWlzc3Bl bGxlZCBsb2NhdGlvbiBhbmQgbGVuZ3RoIG9mIHRoZSBzdHJpbmcgdG8gdGhlIGRlZmF1bHQgdmFs dWVzICgtMSwgMCkKKyAgICAgICAgYXQgdGhlIGJlZ2lubmluZyBvZiB0aGUgY2hlY2tTcGVsbGlu Z09mU3RyaW5nIG1ldGhvZC4KKyAgICAgICAgSXQgYXNzdW1lcyB0aGF0IHRoZSB3b3JkcyBhcmUg c3BlbGxlZCBjb3JyZWN0bHkgYW5kIGVhcmx5IHJldHVybiBvZiBtZXRob2QgZG9lc24ndAorICAg ICAgICBjYXVzZSBhc3NlcnRpb25zIGluIGZpbmRGaXJzdE1pc3NwZWxsaW5nIChXZWJDb3JlL2Vk aXRpbmcvVGV4dENoZWNraW5nSGVscGVyLmNwcCkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS90ZXh0 L2VuY2hhbnQvVGV4dENoZWNrZXJFbmNoYW50LmNwcDoKKyAgICAgICAgKFRleHRDaGVja2VyRW5j aGFudDo6Y2hlY2tTcGVsbGluZ09mU3RyaW5nKToKKwogMjAxMi0xMC0yOSAgQWxleGFuZGVyIFBh dmxvdiAgPGFwYXZsb3ZAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IFtT dHlsZXNdIEhhbmRsZSBub24tcGFyc2VkT2sgcHJvcGVydGllcyBhcyBpbmFjdGl2ZSBvbmVzCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQvVGV4dENoZWNr ZXJFbmNoYW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvZW5jaGFudC9UZXh0 Q2hlY2tlckVuY2hhbnQuY3BwCmluZGV4IGQzMTVlZTkuLjBiZTQ3YTcgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvZW5jaGFudC9UZXh0Q2hlY2tlckVuY2hhbnQuY3Bw CisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvZW5jaGFudC9UZXh0Q2hlY2tlckVu Y2hhbnQuY3BwCkBAIC02OCw2ICs2OCwxMCBAQCB2b2lkIFRleHRDaGVja2VyRW5jaGFudDo6bGVh cm5Xb3JkKGNvbnN0IFN0cmluZyYgd29yZCkKIAogdm9pZCBUZXh0Q2hlY2tlckVuY2hhbnQ6OmNo ZWNrU3BlbGxpbmdPZlN0cmluZyhjb25zdCBTdHJpbmcmIHN0cmluZywgaW50JiBtaXNzcGVsbGlu Z0xvY2F0aW9uLCBpbnQmIG1pc3NwZWxsaW5nTGVuZ3RoKQogeworICAgIC8vIEFzc3VtZSB0aGF0 IHRoZSB3b3JkcyBpbiB0aGUgQGEgc3RyaW5nIGFyZSBzcGVsbGVkIGNvcnJlY3RseS4KKyAgICBt aXNzcGVsbGluZ0xvY2F0aW9uID0gLTE7CisgICAgbWlzc3BlbGxpbmdMZW5ndGggPSAwOworCiAg ICAgaWYgKG1fZW5jaGFudERpY3Rpb25hcmllcy5pc0VtcHR5KCkpCiAgICAgICAgIHJldHVybjsK IAo= 172559 2012-11-06 05:38:08 -0800 2012-11-06 07:15:17 -0800 apply Gustavo's suggestions - patch for landing spelling-checkSpellingOfString_Fix.patch text/plain 1722 g.czajkowski ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAyMDc1MDM4Li42YmE5MTEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTItMTEtMDYgIEdyemVnb3J6IEN6YWprb3dza2kgIDxnLmN6YWprb3dza2lAc2Ftc3Vu Zy5jb20+CisKKyAgICAgICAgW1dLMl1bRUZMXVtHVEtdIGVhcmx5IHJldHVybiBvZiBjaGVja1Nw ZWxsaW5nT2ZTdHJpbmcgdHJlYXRzIGNvcnJlY3Qgd29yZHMgYXMgbWlzc3BlbGxlZAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAxMjE1CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgR3VzdGF2byBOb3JvbmhhIFNpbHZhLgorCisgICAgICAgIFNhdmUgdGhl IG1pc3NwZWxsZWQgbG9jYXRpb24gYW5kIGxlbmd0aCBvZiB0aGUgc3RyaW5nIHRvIHRoZSBkZWZh dWx0IHZhbHVlcyAoLTEsIDApCisgICAgICAgIGF0IHRoZSBiZWdpbm5pbmcgb2YgdGhlIGNoZWNr U3BlbGxpbmdPZlN0cmluZyBtZXRob2QuCisgICAgICAgIEl0IGFzc3VtZXMgdGhhdCB0aGUgd29y ZHMgYXJlIHNwZWxsZWQgY29ycmVjdGx5IGFuZCBlYXJseSByZXR1cm4gb2YgbWV0aG9kIGRvZXNu J3QKKyAgICAgICAgY2F1c2UgYXNzZXJ0aW9ucyBpbiBmaW5kRmlyc3RNaXNzcGVsbGluZyAoV2Vi Q29yZS9lZGl0aW5nL1RleHRDaGVja2luZ0hlbHBlci5jcHApLgorCisgICAgICAgICogcGxhdGZv cm0vdGV4dC9lbmNoYW50L1RleHRDaGVja2VyRW5jaGFudC5jcHA6CisgICAgICAgIChUZXh0Q2hl Y2tlckVuY2hhbnQ6OmNoZWNrU3BlbGxpbmdPZlN0cmluZyk6CisKIDIwMTItMTEtMDYgIEtlaXNo aSBIYXR0b3JpICA8a2Vpc2hpQHdlYmtpdC5vcmc+CiAKICAgICAgICAgVGhlICJSZWN0IiBjbGFz cyBpbiBXZWJDb3JlL1Jlc291cmNlcy9wYWdlcG9wdXBzL3BpY2tlckNvbW1vbi5qcyBzaG91bGQg YmUgcmVuYW1lZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9lbmNo YW50L1RleHRDaGVja2VyRW5jaGFudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0 L2VuY2hhbnQvVGV4dENoZWNrZXJFbmNoYW50LmNwcAppbmRleCBkMzE1ZWU5Li4yODYzM2YxIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQvVGV4dENoZWNr ZXJFbmNoYW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQv VGV4dENoZWNrZXJFbmNoYW50LmNwcApAQCAtNjgsNiArNjgsMTAgQEAgdm9pZCBUZXh0Q2hlY2tl ckVuY2hhbnQ6OmxlYXJuV29yZChjb25zdCBTdHJpbmcmIHdvcmQpCiAKIHZvaWQgVGV4dENoZWNr ZXJFbmNoYW50OjpjaGVja1NwZWxsaW5nT2ZTdHJpbmcoY29uc3QgU3RyaW5nJiBzdHJpbmcsIGlu dCYgbWlzc3BlbGxpbmdMb2NhdGlvbiwgaW50JiBtaXNzcGVsbGluZ0xlbmd0aCkKIHsKKyAgICAv LyBBc3N1bWUgdGhhdCB0aGUgd29yZHMgaW4gdGhlIHN0cmluZyBhcmUgc3BlbGxlZCBjb3JyZWN0 bHkuCisgICAgbWlzc3BlbGxpbmdMb2NhdGlvbiA9IC0xOworICAgIG1pc3NwZWxsaW5nTGVuZ3Ro ID0gMDsKKwogICAgIGlmIChtX2VuY2hhbnREaWN0aW9uYXJpZXMuaXNFbXB0eSgpKQogICAgICAg ICByZXR1cm47CiAK