Bug 87521

Summary: characterBreakIterator() is not safe to use reentrantly or from multiple threads
Product: WebKit Reporter: mitz
Component: TextAssignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: darin, gustavo, jberlin, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87740    
Bug Blocks:    
Attachments:
Description Flags
Add and deply a NonSharedCharacterBreakIterator class
gustavo: commit-queue-
Add and deply a NonSharedCharacterBreakIterator class
darin: review+
Added a code path for when COMPARE_AND_SWAP is not enabled jberlin: review+

mitz
Reported 2012-05-25 11:28:18 PDT
characterBreakIterator() is not safe to use reentrantly or from multiple threads
Attachments
Add and deply a NonSharedCharacterBreakIterator class (17.44 KB, patch)
2012-05-25 11:43 PDT, mitz
gustavo: commit-queue-
Add and deply a NonSharedCharacterBreakIterator class (17.47 KB, patch)
2012-05-25 11:56 PDT, mitz
darin: review+
Added a code path for when COMPARE_AND_SWAP is not enabled (2.83 KB, patch)
2012-05-25 16:38 PDT, mitz
jberlin: review+
mitz
Comment 1 2012-05-25 11:29:32 PDT
mitz
Comment 2 2012-05-25 11:43:39 PDT
Created attachment 144110 [details] Add and deply a NonSharedCharacterBreakIterator class
Gustavo Noronha (kov)
Comment 3 2012-05-25 11:52:13 PDT
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
mitz
Comment 4 2012-05-25 11:56:19 PDT
Created attachment 144118 [details] Add and deply a NonSharedCharacterBreakIterator class
Darin Adler
Comment 5 2012-05-25 13:30:35 PDT
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.
mitz
Comment 6 2012-05-25 14:47:28 PDT
Jessie Berlin
Comment 7 2012-05-25 16:02:56 PDT
(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 :(
mitz
Comment 8 2012-05-25 16:11:51 PDT
(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.
mitz
Comment 9 2012-05-25 16:17:27 PDT
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.
mitz
Comment 10 2012-05-25 16:38:47 PDT
Created attachment 144168 [details] Added a code path for when COMPARE_AND_SWAP is not enabled
mitz
Comment 11 2012-05-25 16:47:06 PDT
Follow-up patch landed in <http://trac.webkit.org/r118587>.
Note You need to log in before you can comment on or make changes to this bug.