characterBreakIterator() is not safe to use reentrantly or from multiple threads

• Component: Text
• Status: RESOLVED FIXED

characterBreakIterator() is not safe to use reentrantly or from multiple threads

<rdar://problem/10189597>

Created attachment 144110 [details]
Add and deply a NonSharedCharacterBreakIterator class

[Gustavo Noronha (kov)]

Comment on attachment 144110 [details]
Add and deply a NonSharedCharacterBreakIterator class

Attachment 144110 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12791590

Created attachment 144118 [details]
Add and deply a NonSharedCharacterBreakIterator class

[Darin Adler]

Comment on attachment 144118 [details]
Add and deply a NonSharedCharacterBreakIterator class

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

> Source/WebCore/platform/text/TextBreakIterator.h:112
> +    TextBreakIterator* get() const { return m_iterator; }

MIght want to consider the type conversion operator for this instead of an explicit get function.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:93
> +    bool createdIterator = m_iterator && weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), m_iterator, 0);

The reinterpret_cast<void**> idiom is particularly unpleasant. Might be worth looking into a refactoring to clean that up and put it in one place.

> Source/WebCore/platform/text/wince/TextBreakIteratorWinCE.cpp:247
> +        m_iterator = new CharBreakIterator();

I normally omit the () in lines of code like this.

Fixed in <http://trac.webkit.org/r118568>.

[Jessie Berlin]

(In reply to comment #6)
> Fixed in <http://trac.webkit.org/r118568>.

I think this caused 19+ crashes to start happening on the Win WK2 Release bots:

http://build.webkit.org/results/Windows%207%20Release%20(WebKit2%20Tests)/r118568%20(18598)/results.html


INVALID_POINTER_WRITE
    Tid    [0x12c4]
    Frame  [0x00]: ntdll!ZwRaiseException


PRIMARY_PROBLEM_CLASS:  INVALID_POINTER_WRITE

BUGCHECK_STR:  APPLICATION_FAULT_INVALID_POINTER_WRITE

STACK_TEXT:  
0031ed6c 6f98ccd5 00019d87 7e1740c0 0031ee44 WebKit!WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator+0xf [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\text\textbreakiteratoricu.cpp @ 99]
0031ed84 6f98f25e 7e03c014 003e9388 00010000 WebKit!WebCore::CharacterData::parserAppendData+0x75 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\dom\characterdata.cpp @ 80]
0031eda4 6fe5481f 0031ede0 7e190800 0031ee44 WebKit!WebCore::Text::createWithLengthLimit+0xde [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\dom\text.cpp @ 292]
0031ee00 6fe2de9f 0031ee44 00000002 0031ee98 WebKit!WebCore::HTMLConstructionSite::insertTextNode+0x15f [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmlconstructionsite.cpp @ 392]
0031ee50 6fe2e483 0031ee5c 7e038b0e 7e038b0e WebKit!WebCore::HTMLTreeBuilder::processCharacterBuffer+0x38f [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmltreebuilder.cpp @ 2384]
0031ee64 6fe30411 0031ee98 0031ee84 6fe30f23 WebKit!WebCore::HTMLTreeBuilder::processCharacter+0x23 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmltreebuilder.cpp @ 2258]
0031ee70 6fe30f23 0031ee98 7e168f20 7e19405c WebKit!WebCore::HTMLTreeBuilder::processToken+0x61 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmltreebuilder.cpp @ 516]
0031ee84 6fe311a0 0031ee98 7e19405c 7e194000 WebKit!WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken+0x23 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmltreebuilder.cpp @ 477]
0031eebc 6fd89119 7e19405c 7e3b13d8 7e194000 WebKit!WebCore::HTMLTreeBuilder::constructTreeFromToken+0x30 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmltreebuilder.cpp @ 461]
0031eefc 6fd89498 00000000 7e194000 0031ef58 WebKit!WebCore::HTMLDocumentParser::pumpTokenizer+0x119 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmldocumentparser.cpp @ 278]
0031ef0c 6f97cb88 0031ef20 7e3b1380 7e3b13d8 WebKit!WebCore::HTMLDocumentParser::append+0xc8 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\html\parser\htmldocumentparser.cpp @ 372]
0031ef58 6fd74435 7e3b13d8 05110048 7dfd0000 WebKit!WebCore::DecodedDataDocumentParser::appendBytes+0x58 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\dom\decodeddatadocumentparser.cpp @ 50]
0031ef70 6fa8aa8e 05110048 0001a346 7eeb0808 WebKit!WebCore::DocumentWriter::addData+0x55 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\documentwriter.cpp @ 218]
0031efbc 6f74435c 05110048 0001a346 7eec4700 WebKit!WebCore::DocumentLoader::commitData+0xee [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\documentloader.cpp @ 349]
0031eff4 6fa8afd3 7e3b1380 05110048 0001a346 WebKit!WebKit::WebFrameLoaderClient::committedLoad+0x2c [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit2\webprocess\webcoresupport\webframeloaderclient.cpp @ 870]
0031f014 6fa8b06e 05110048 0001a346 0001a346 WebKit!WebCore::DocumentLoader::commitLoad+0x93 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\documentloader.cpp @ 322]
0031f02c 6fdaf843 05110048 0001a346 0001a346 WebKit!WebCore::DocumentLoader::receivedData+0x4e [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\documentloader.cpp @ 360]
0031f048 6fc54e25 05110048 0001a346 00000000 WebKit!WebCore::MainResourceLoader::addData+0x23 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\mainresourceloader.cpp @ 192]
0031f068 6fdb0b08 05110048 0001a346 0001a346 WebKit!WebCore::ResourceLoader::didReceiveData+0x25 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\resourceloader.cpp @ 276]
0031f184 6fc54d30 05110048 0001a346 0001a346 WebKit!WebCore::MainResourceLoader::didReceiveData+0x188 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\mainresourceloader.cpp @ 512]
0031f1b4 6f8e1a73 7e15d2d0 05110048 0001a346 WebKit!WebCore::ResourceLoader::didReceiveData+0x60 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\loader\resourceloader.cpp @ 430]
0031f1d8 72d46581 03e03a30 03e03c38 0001a346 WebKit!WebCore::didReceiveData+0x43 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\network\cf\resourcehandlecfnet.cpp @ 265]
WARNING: Stack unwind information not available. Following frames may be wrong.
0031f2f4 72d48f20 0040aa20 0031f398 003ce8c8 CFNetwork!CFReadStreamCreateWithFormArray+0x6671
0031f480 72d492e5 0031f53c 00000004 03e98b68 CFNetwork!CFReadStreamCreateWithFormArray+0x9010
0031f60c 72d426e2 03e98b9c 00000004 03e03a30 CFNetwork!CFReadStreamCreateWithFormArray+0x93d5
0031f67c 72d438e4 03e9aa80 000004cf 6f7c3956 CFNetwork!CFReadStreamCreateWithFormArray+0x27d2
0031f6a0 75e362fa 018103d6 000004cf 03e03a30 CFNetwork!CFReadStreamCreateWithFormArray+0x39d4
0031f6cc 75e36d3a 72d43860 018103d6 000004cf USER32!InternalCallWinProc+0x23
0031f744 75e377c4 00000000 72d43860 018103d6 USER32!UserCallWinProcCheckWow+0x109
0031f7a4 75e3788a 72d43860 00000000 0031f7e8 USER32!DispatchMessageWorker+0x3bc
0031f7b4 6f7c3611 0031f7cc 0031f830 00000000 USER32!DispatchMessageW+0xf
0031f7e8 6f760eee 76051222 7ee90488 7ee913c0 WebKit!WebCore::RunLoop::run+0x41 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\win\runloopwin.cpp @ 76]
0031f7fc 6f736766 0031f830 00000000 7ee951f0 WebKit!WebKit::WebProcessMain+0xde [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit2\webprocess\win\webprocessmainwin.cpp @ 84]
0031f81c 6f73680c 00000000 00800000 005a14ce WebKit!WebKitMain+0x116 [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit2\webprocess\webkitmain.cpp @ 59]
0031f848 00801098 00800000 00000000 005a14ce WebKit!WebKitMain+0x9c [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit2\webprocess\webkitmain.cpp @ 187]
0031fa78 00801258 00800000 00000000 005a14ce WebKit2WebProcess!wWinMain+0x98 [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit2\win\mainwin.cpp @ 67]
0031fb0c 7605339a 7efde000 0031fb58 77d89ef2 WebKit2WebProcess!__tmainCRTStartup+0x150 [f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 589]
0031fb18 77d89ef2 7efde000 0cf64ed5 00000000 kernel32!BaseThreadInitThunk+0xe
0031fb58 77d89ec5 008013c4 7efde000 00000000 ntdll!__RtlUserThreadStart+0x70
0031fb70 00000000 008013c4 7efde000 00000000 ntdll!_RtlUserThreadStart+0x1b

If I don't hear from you or darin soon, I will have to roll it out :(

(In reply to comment #7)
> (In reply to comment #6)
> > Fixed in <http://trac.webkit.org/r118568>.
> 
> I think this caused 19+ crashes to start happening on the Win WK2 Release bots:

This is happening because that build configuration doesn’t enable COMPARE_AND_SWAP.

Since COMPARE_AND_SWAP is an optional feature, we can’t require it for any use of character break iterators. We can either make NonSharedCharacterBreakIterator silently unsafe when COMPARE_AND_SWAP is not enabled, or try to write an alternative, safe implementation that doesn’t rely on COMPARE_AND_SWAP.

Created attachment 144168 [details]
Added a code path for when COMPARE_AND_SWAP is not enabled

Follow-up patch landed in <http://trac.webkit.org/r118587>.