Bug 126606

Summary:

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

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

HTML Editing

Assignee:

Simon Fraser (smfr) <simon.fraser>

Status:

NEW

Severity:

Normal

CC:

ews-watchlist, mifenton, rniwa, simon.fraser, wenson_hsieh

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=158401

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	wenson_hsieh: review+

Alexey Proskuryakov

Reported 2014-01-07 16:52:33 PST

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

Attachments
Patch (1.54 KB, patch) 2020-03-09 15:19 PDT, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch (15.31 KB, patch) 2020-03-09 17:44 PDT, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch (15.31 KB, patch) 2020-03-09 17:55 PDT, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch (25.44 KB, patch) 2020-03-09 19:02 PDT, Simon Fraser (smfr)	wenson_hsieh: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2020-03-09 15:19:41 PDT

Created attachment 393083 [details] Patch

Wenson Hsieh

Comment 2 2020-03-09 15:28:20 PDT

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 3 2020-03-09 15:33:50 PDT

Comment on attachment 393083 [details] Patch Actually we just hit an assertion later with this patch.

Alexey Proskuryakov

Comment 4 2020-03-09 15:40:38 PDT

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

Comment 5 2020-03-09 17:24:20 PDT

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)

Comment 6 2020-03-09 17:44:50 PDT

Created attachment 393100 [details] Patch

Wenson Hsieh

Comment 7 2020-03-09 17:53:37 PDT

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 8 2020-03-09 17:54:35 PDT

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)

Comment 9 2020-03-09 17:55:21 PDT

Created attachment 393101 [details] Patch

Simon Fraser (smfr)

Comment 10 2020-03-09 17:59:42 PDT

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

Comment 11 2020-03-09 19:02:36 PDT

Created attachment 393108 [details] Patch

Wenson Hsieh

Comment 12 2020-03-09 19:37:05 PDT

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 > +2020-03-09 Simon Fraser <simon.fraser@apple.com> > + > + 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.

Note You need to log in before you can comment on or make changes to this bug.