Bug 126606 – ASSERT(!m_textCheckingRequest) on editing/spelling/spellcheck-async-remove-frame.html

• Status: NEW
• Component: HTML Editing

[Alexey Proskuryakov]

I've hit this assertion when running tests locally.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000103a59b2a WTFCrash + 42 (Assertions.cpp:341)
1   com.apple.WebKit              	0x0000000104b9929e WebEditorClient::requestCheckingOfString(WTF::PassRefPtr<WebCore::TextCheckingRequest>) + 94 (WebEditorClient.mm:1156)
2   com.apple.WebKit              	0x0000000104b9970c non-virtual thunk to WebEditorClient::requestCheckingOfString(WTF::PassRefPtr<WebCore::TextCheckingRequest>) + 28 (WebEditorClient.mm:1169)
3   com.apple.WebCore             	0x0000000106ad9252 WebCore::SpellChecker::invokeRequest(WTF::PassRefPtr<WebCore::SpellCheckRequest>) + 210 (SpellChecker.cpp:185)
4   com.apple.WebCore             	0x0000000106ad9592 WebCore::SpellChecker::requestCheckingFor(WTF::PassRefPtr<WebCore::SpellCheckRequest>) + 434 (SpellChecker.cpp:176)
5   com.apple.WebCore             	0x0000000105884fc0 WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges(unsigned int, WebCore::Range*, WebCore::Range*) + 960 (Editor.cpp:2164)
6   com.apple.WebCore             	0x00000001058846f8 WebCore::Editor::markMisspellingsAfterTypingToWord(WebCore::VisiblePosition const&, WebCore::VisibleSelection const&, bool) + 648 (Editor.cpp:2015)
7   com.apple.WebCore             	0x0000000106d908d5 WebCore::TypingCommand::markMisspellingsAfterTyping(WebCore::TypingCommand::ETypingCommand) + 693 (TypingCommand.cpp:309)
8   com.apple.WebCore             	0x0000000106d909d2 WebCore::TypingCommand::typingAddedToOpenCommand(WebCore::TypingCommand::ETypingCommand) + 130 (TypingCommand.cpp:331)
9   com.apple.WebCore             	0x0000000106d90ab6 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 214 (TypingCommand.cpp:351)
10  com.apple.WebCore             	0x0000000106d91472 WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const + 146 (TypingCommand.cpp:60)
11  com.apple.WebCore             	0x0000000106d911b5 void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) + 165 (TextInsertionBaseCommand.h:61)
12  com.apple.WebCore             	0x0000000106d8fe18 WebCore::TypingCommand::insertText(WTF::String const&, bool) + 72 (TypingCommand.cpp:342)
13  com.apple.WebCore             	0x0000000106d905bc WebCore::TypingCommand::doApply() + 348 (TypingCommand.cpp:269)
14  com.apple.WebCore             	0x00000001054525e1 WebCore::CompositeEditCommand::apply() + 257 (CompositeEditCommand.cpp:212)
15  com.apple.WebCore             	0x00000001054524d1 WebCore::applyCommand(WTF::PassRefPtr<WebCore::CompositeEditCommand>) + 17 (CompositeEditCommand.cpp:171)
16  com.apple.WebCore             	0x0000000106d2f007 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WTF::PassRefPtr<WebCore::TextInsertionBaseCommand>, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) + 119 (TextInsertionBaseCommand.cpp:50)
17  com.apple.WebCore             	0x0000000106d8fd6b WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 747 (TypingCommand.cpp:187)
18  com.apple.WebCore             	0x0000000106d8fa0c WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 220 (TypingCommand.cpp:158)
19  com.apple.WebCore             	0x0000000105891587 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 55 (EditorCommand.cpp:560)
20  com.apple.WebCore             	0x000000010588f2a5 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 213 (EditorCommand.cpp:1713)
21  com.apple.WebCore             	0x000000010572006e WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 78 (Document.cpp:4222)
22  com.apple.WebCore             	0x0000000105f476d9 WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 793 (JSDocument.cpp:3364)
23  ???                           	0x00002e321c601045 0 + 50792759300165

[Simon Fraser (smfr)]

Created attachment 393083 [details]
Patch

[Wenson Hsieh]

We could also fix this by making requestCheckingOfString/didCheckSucceed robust in the case where a request comes in before the previous one has finished, but the null check is okay for now.

[Simon Fraser (smfr)]

Comment on attachment 393083 [details]
Patch

Actually we just hit an assertion later with this patch.

[Alexey Proskuryakov]

(In reply to Wenson Hsieh from comment #2)
> We could also fix this by making requestCheckingOfString/didCheckSucceed
> robust in the case where a request comes in before the previous one has
> finished, but the null check is okay for now.

Would the test be flakily failing with a null check? That's not much better than crashing.

[Simon Fraser (smfr)]

There's more to this than meets the eye.

First, WebEditorClient can only handle on in-flight request at a time, but it's quite possible to call WebEditorClient::requestCheckingOfString() twice in a row before NSSpellChecker is done.

Second, the sequence numbers are per-SpellChecker, so there is one per frame, yet WebEditorClient is a singleton so sees requests from different frames.

[Simon Fraser (smfr)]

Created attachment 393100 [details]
Patch

[Wenson Hsieh]

Comment on attachment 393100 [details]
Patch

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:1263
> +    m_requestsInFlight.add(sequence, request.makeRef());

Pretty sure you meant makeRef(request)?

[Ryosuke Niwa]

Comment on attachment 393100 [details]
Patch

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

> Source/WebCore/platform/text/TextChecking.h:80
> +using TextCheckingSequence = int;

Can we use ObjectIdentifier instead?

[Simon Fraser (smfr)]

Created attachment 393101 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 393100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393100&action=review
> 
> > Source/WebCore/platform/text/TextChecking.h:80
> > +using TextCheckingSequence = int;
> 
> Can we use ObjectIdentifier instead?

We could.

[Simon Fraser (smfr)]

Created attachment 393108 [details]
Patch

[Wenson Hsieh]

Comment on attachment 393108 [details]
Patch

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

r=mews

> Source/WebKitLegacy/mac/ChangeLog:37
> [phone]  Simon Fraser  <[e-mail]>
> +
> +        ASSERT(!m_textCheckingRequest) on editing/spelling/spellcheck-async-remove-frame.html
> +        https://bugs.webkit.org/show_bug.cgi?id=126606
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Make WebEditorClient able to handle multiple in-flight spell check requests by storing
> +        a HashMap of sequence numbers to TextCheckingRequests.
> +
> +        * WebCoreSupport/WebEditorClient.h:
> +        * WebCoreSupport/WebEditorClient.mm:
> +        (-[WebEditorSpellCheckResponder initWithClient:sequence:results:]):
> +        (WebEditorClient::didCheckSucceed):
> +        (WebEditorClient::requestCheckingOfString):

Double changelog!

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:1242
> +void WebEditorClient::didCheckSucceed(TextCheckingRequestIdentifier identifier, NSArray* results)

Nit - * on the other side.

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:1269
> +    [[NSSpellChecker sharedSpellChecker] requestCheckingOfString:request.data().text() range:range types:NSTextCheckingAllSystemTypes options:options inSpellDocumentWithTag:0 completionHandler:^(NSInteger, NSArray* results, NSOrthography*, NSInteger) {

Nit - not new in this patch, but there are some *s on the wrong side here.