| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=158401 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alexey Proskuryakov
2014-01-07 16:52:33 PST
Created attachment 393083 [details]
Patch
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. Comment on attachment 393083 [details]
Patch
Actually we just hit an assertion later with this patch.
(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. 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. Created attachment 393100 [details]
Patch
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)? 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? Created attachment 393101 [details]
Patch
(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. Created attachment 393108 [details]
Patch
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. |