Bug 80883 – REGRESSION: Spell check tests hit assertions on Mac

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

The following tests hit assertions on Mac:
editing/spelling/spellcheck-queue.html
editing/spelling/spellcheck-async-mutation.html
editing/spelling/spellcheck-paste.html
editing/spelling/grammar-paste.html
editing/spelling/spellcheck-sequencenum.html
editing/spelling/spellcheck-async.html

ASSERTION FAILED: m_processingRequest->sequence() == sequence
/Volumes/Data/webkit4/Source/WebCore/editing/SpellChecker.cpp(170) : void WebCore::SpellChecker::didCheck(int, const WTF::Vector<WebCore::TextCheckingResult, 0ul>&)
1   0x1016aac44 WebCore::SpellChecker::didCheck(int, WTF::Vector<WebCore::TextCheckingResult, 0ul> const&)
2   0x100e79863 -[WebEditorSpellCheckResponder perform]
3   0x7fff81172351 __NSFirePerformWithOrder
4   0x7fff836b0b07 __CFRunLoopDoObservers
5   0x7fff8368c434 __CFRunLoopRun
6   0x7fff8368bd8f CFRunLoopRunSpecific
7   0x7fff81181b74 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
8   0x100014535 runTest(std::string const&)
9   0x100014a56 runTestingServerLoop()
10  0x100014c56 dumpRenderTree(int, char const**)
11  0x100014e64 main
12  0x10000177c start

[Ryosuke Niwa]

Also these 5 tests are timing out on bots:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r110496%20(37999)/results.html

[Shinya Kawanaka]

Created attachment 131554 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 131554 [details]
Patch

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

> Source/WebKit/mac/ChangeLog:9
> +        Since WebEditorClient::requestCheckingOfString touches TextCheckingObject in a block statement,
> +        however it won't be correctly managed in Objective-C.

I don't really understand this description. Could you clarify?

[Shinya Kawanaka]

(In reply to comment #3)
> (From update of attachment 131554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131554&action=review
> 
> > Source/WebKit/mac/ChangeLog:9
> > +        Since WebEditorClient::requestCheckingOfString touches TextCheckingObject in a block statement,
> > +        however it won't be correctly managed in Objective-C.
> 
> I don't really understand this description. Could you clarify?

I'm not sure I can describe it well, but let me try.

WebEditorClient::requestCheckingOfString uses a closure (for completionHandler), and it uses |request| object in it. |request| will be destroyed when exiting from requestCheckingOfString.

However, WebCore::TextCheckingRequest is not an NSObject (or a primitive value), it won't be copied correctly when creating a closure (maybe). So we should not touch |request| in a closure.

So I copied necessary fields to stack.

[Ryosuke Niwa]

Comment on attachment 131554 [details]
Patch

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

>>> Source/WebKit/mac/ChangeLog:9
>>> +        however it won't be correctly managed in Objective-C.
>> 
>> I don't really understand this description. Could you clarify?
> 
> I'm not sure I can describe it well, but let me try.
> 
> WebEditorClient::requestCheckingOfString uses a closure (for completionHandler), and it uses |request| object in it. |request| will be destroyed when exiting from requestCheckingOfString.
> 
> However, WebCore::TextCheckingRequest is not an NSObject (or a primitive value), it won't be copied correctly when creating a closure (maybe). So we should not touch |request| in a closure.
> 
> So I copied necessary fields to stack.

Ah I see. I think I get what you mean. I'll paraphrase it like something along the line of:

The bug was caused by the closure object created in requestCheckingOfString accessing test checking request's member variables even though the request object is not an NSObject and allocated in stack. This resulted in the closure not being able to access those variables when invoked. Fixed the bug by making local copies of those member variables.

[Ryosuke Niwa]

Comment on attachment 131554 [details]
Patch

r=me provided the description in the change log is revised.

[Shinya Kawanaka]

(In reply to comment #6)
> (From update of attachment 131554 [details])
> r=me provided the description in the change log is revised.

Thanks, I'll update the ChangeLog.

[Shinya Kawanaka]

Created attachment 131558 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 131558 [details]
Patch for landing

Clearing flags on attachment: 131558

Committed r110546: <http://trac.webkit.org/changeset/110546>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.