Changeset 118568 in webkit


Ignore:
Timestamp:
May 25, 2012 2:46:57 PM (12 years ago)
Author:
mitz@apple.com
Message:

characterBreakIterator() is not safe to use reentrantly or from multiple threads
​https://bugs.webkit.org/show_bug.cgi?id=87521

Reviewed by Darin Adler.

Replaced characterBreakIterator() with a NonSharedCharacterBreakIterator class, which
obtains a unique TextBreakIterator. Replaced the global shared instance with a single-entry
cache.

  • dom/CharacterData.cpp:

(WebCore::CharacterData::parserAppendData): Changed to use NonSharedCharacterBreakIterator.

  • platform/graphics/StringTruncator.cpp:

(WebCore::centerTruncateToBuffer): Ditto.
(WebCore::rightTruncateToBuffer): Ditto.

  • platform/text/String.cpp:

(WebCore::numGraphemeClusters): Ditto.
(WebCore::numCharactersInGraphemeClusters): Ditto.

  • platform/text/TextBreakIterator.h: Removed the declaration of characterBreakIterator().

(NonSharedCharacterBreakIterator): Added. An instance of this class has a character break
iterator instance that is unique to it for the lifetime of the instance.
(WebCore::NonSharedCharacterBreakIterator::operator TextBreakIterator*): Added.

  • platform/text/TextBreakIteratorICU.cpp:

(WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Added. Tries
to swap the m_iterator member variable with the cached instance. If that fails, initializes
m_iterator to a new character break iterator.
(WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Added. Tries
to put the m_iterator member variable back in the cache. If that fails, meaning there is
already something in the cache, destroys m_iterator.

  • platform/text/gtk/TextBreakIteratorGtk.cpp:

(WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
TextBreakIteratorICU.cpp.
(WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
(WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
here.

  • platform/text/qt/TextBreakIteratorQt.cpp:

(WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
TextBreakIteratorICU.cpp.
(WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
(WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
here.

  • platform/text/wince/TextBreakIteratorWinCE.cpp:

(WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
TextBreakIteratorICU.cpp.
(WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
(WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
here.

Location:
trunk/Source/WebCore
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r118567 r118568  
     12012-05-25  Dan Bernstein  <mitz@apple.com>
     2
     3        characterBreakIterator() is not safe to use reentrantly or from multiple threads
     4        https://bugs.webkit.org/show_bug.cgi?id=87521
     5
     6        Reviewed by Darin Adler.
     7
     8        Replaced characterBreakIterator() with a NonSharedCharacterBreakIterator class, which
     9        obtains a unique TextBreakIterator. Replaced the global shared instance with a single-entry
     10        cache.
     11
     12        * dom/CharacterData.cpp:
     13        (WebCore::CharacterData::parserAppendData): Changed to use NonSharedCharacterBreakIterator.
     14
     15        * platform/graphics/StringTruncator.cpp:
     16        (WebCore::centerTruncateToBuffer): Ditto.
     17        (WebCore::rightTruncateToBuffer): Ditto.
     18
     19        * platform/text/String.cpp:
     20        (WebCore::numGraphemeClusters): Ditto.
     21        (WebCore::numCharactersInGraphemeClusters): Ditto.
     22
     23        * platform/text/TextBreakIterator.h: Removed the declaration of characterBreakIterator().
     24        (NonSharedCharacterBreakIterator): Added. An instance of this class has a character break
     25        iterator instance that is unique to it for the lifetime of the instance.
     26        (WebCore::NonSharedCharacterBreakIterator::operator TextBreakIterator*): Added.
     27
     28        * platform/text/TextBreakIteratorICU.cpp:
     29        (WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Added. Tries
     30        to swap the m_iterator member variable with the cached instance. If that fails, initializes
     31        m_iterator to a new character break iterator.
     32        (WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Added. Tries
     33        to put the m_iterator member variable back in the cache. If that fails, meaning there is
     34        already something in the cache, destroys m_iterator.
     35
     36        * platform/text/gtk/TextBreakIteratorGtk.cpp:
     37        (WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
     38        TextBreakIteratorICU.cpp.
     39        (WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
     40        (WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
     41        here.
     42
     43        * platform/text/qt/TextBreakIteratorQt.cpp:
     44        (WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
     45        TextBreakIteratorICU.cpp.
     46        (WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
     47        (WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
     48        here.
     49
     50        * platform/text/wince/TextBreakIteratorWinCE.cpp:
     51        (WebCore::NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator): Same as in
     52        TextBreakIteratorICU.cpp.
     53        (WebCore::NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator): Ditto.
     54        (WebCore::cursorMovementIterator): Moved the old implementation of characterBreakIterator()
     55        here.
     56
    1572012-05-25  Simon Fraser  <simon.fraser@apple.com>
    258

  • trunk/Source/WebCore/dom/CharacterData.cpp

    r118471 r118568  
    7373    // We need at least two characters look-ahead to account for UTF-16 surrogates.
    7474    if (end < dataLength) {
    75         TextBreakIterator* it = characterBreakIterator(data, (end + 2 > dataLength) ? dataLength : end + 2);
     75        NonSharedCharacterBreakIterator it(data, (end + 2 > dataLength) ? dataLength : end + 2);
    7676        if (!isTextBreak(it, end))
    7777            end = textBreakPreceding(it, end);

  • trunk/Source/WebCore/platform/graphics/StringTruncator.cpp

    r89733 r118568  
    6464   
    6565    unsigned omitStart = (keepCount + 1) / 2;
    66     TextBreakIterator* it = characterBreakIterator(string.characters(), length);
     66    NonSharedCharacterBreakIterator it(string.characters(), length);
    6767    unsigned omitEnd = boundedTextBreakFollowing(it, omitStart + (length - keepCount) - 1, length);
    6868    omitStart = textBreakAtOrPreceding(it, omitStart);
    … …  
    8383    ASSERT(keepCount < STRING_BUFFER_SIZE);
    8484   
    85     TextBreakIterator* it = characterBreakIterator(string.characters(), length);
     85    NonSharedCharacterBreakIterator it(string.characters(), length);
    8686    unsigned keepLength = textBreakAtOrPreceding(it, keepCount);
    8787    unsigned truncatedLength = keepLength + 1;

  • trunk/Source/WebCore/platform/text/String.cpp

    r57904 r118568  
    5252unsigned numGraphemeClusters(const String& s)
    5353{
    54     TextBreakIterator* it = characterBreakIterator(s.characters(), s.length());
     54    NonSharedCharacterBreakIterator it(s.characters(), s.length());
    5555    if (!it)
    5656        return s.length();
    … …  
    6464unsigned numCharactersInGraphemeClusters(const String& s, unsigned numGraphemeClusters)
    6565{
    66     TextBreakIterator* it = characterBreakIterator(s.characters(), s.length());
     66    NonSharedCharacterBreakIterator it(s.characters(), s.length());
    6767    if (!it)
    6868        return min(s.length(), numGraphemeClusters);

  • trunk/Source/WebCore/platform/text/TextBreakIterator.h

    r110965 r118568  
    11/*
    22 * Copyright (C) 2006 Lars Knoll <lars@trolltech.com>
    3  * Copyright (C) 2007 Apple Inc. All rights reserved.
     3 * Copyright (C) 2007, 2011, 2012 Apple Inc. All rights reserved.
    44 *
    55 * This library is free software; you can redistribute it and/or
    … …  
    3131
    3232    // Note: The returned iterator is good only until you get another iterator, with the exception of acquireLineBreakIterator.
    33 
    34     // Iterates over "extended grapheme clusters", as defined in UAX #29.
    35     // Note that platform implementations may be less sophisticated - e.g. ICU prior to
    36     // version 4.0 only supports "legacy grapheme clusters".
    37     // Use this for general text processing, e.g. string truncation.
    38     TextBreakIterator* characterBreakIterator(const UChar*, int length);
    3933
    4034    // This is similar to character break iterator in most cases, but is subject to
    … …  
    10599};
    106100
     101// Iterates over "extended grapheme clusters", as defined in UAX #29.
     102// Note that platform implementations may be less sophisticated - e.g. ICU prior to
     103// version 4.0 only supports "legacy grapheme clusters".
     104// Use this for general text processing, e.g. string truncation.
     105
     106class NonSharedCharacterBreakIterator {
     107    WTF_MAKE_NONCOPYABLE(NonSharedCharacterBreakIterator);
     108public:
     109    NonSharedCharacterBreakIterator(const UChar*, int length);
     110    ~NonSharedCharacterBreakIterator();
     111
     112    operator TextBreakIterator*() const { return m_iterator; }
     113
     114private:
     115    TextBreakIterator* m_iterator;
     116};
     117
    107118}
    108119

  • trunk/Source/WebCore/platform/text/TextBreakIteratorICU.cpp

    r110965 r118568  
    11/*
    22 * Copyright (C) 2006 Lars Knoll <lars@trolltech.com>
    3  * Copyright (C) 2007 Apple Inc. All rights reserved.
     3 * Copyright (C) 2007, 2011, 2012 Apple Inc. All rights reserved.
    44 *
    55 * This library is free software; you can redistribute it and/or
    … …  
    2525#include "LineBreakIteratorPoolICU.h"
    2626#include "PlatformString.h"
    27 
     27#include <wtf/Atomics.h>
     28
     29using namespace WTF;
    2830using namespace std;
    2931
    … …  
    5355}
    5456
    55 TextBreakIterator* characterBreakIterator(const UChar* string, int length)
    56 {
    57     static bool createdCharacterBreakIterator = false;
    58     static TextBreakIterator* staticCharacterBreakIterator;
    59     return setUpIterator(createdCharacterBreakIterator,
    60         staticCharacterBreakIterator, UBRK_CHARACTER, string, length);
    61 }
    62 
    6357TextBreakIterator* wordBreakIterator(const UChar* string, int length)
    6458{
    … …  
    9084
    9185    LineBreakIteratorPool::sharedPool().put(reinterpret_cast<UBreakIterator*>(iterator));
     86}
     87
     88static TextBreakIterator* nonSharedCharacterBreakIterator;
     89
     90NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator(const UChar* buffer, int length)
     91{
     92    m_iterator = nonSharedCharacterBreakIterator;
     93    bool createdIterator = m_iterator && weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), m_iterator, 0);
     94    m_iterator = setUpIterator(createdIterator, m_iterator, UBRK_CHARACTER, buffer, length);
     95}
     96
     97NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator()
     98{
     99    if (!weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), 0, m_iterator))
     100        ubrk_close(reinterpret_cast<UBreakIterator*>(m_iterator));
    92101}
    93102

  • trunk/Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp

    r111354 r118568  
    2424
    2525#include "config.h"
    26 
    2726#include "TextBreakIterator.h"
    2827
     28#include <wtf/Atomics.h>
    2929#include <wtf/gobject/GOwnPtr.h>
    3030#include <pango/pango.h>
     31
     32using namespace WTF;
    3133using namespace std;
    3234
    … …  
    220222}
    221223
    222 TextBreakIterator* characterBreakIterator(const UChar* string, int length)
    223 {
    224     static bool createdCharacterBreakIterator = false;
    225     static TextBreakIterator* staticCharacterBreakIterator;
    226     return setUpIterator(createdCharacterBreakIterator, staticCharacterBreakIterator, UBRK_CHARACTER, string, length);
     224static TextBreakIterator* nonSharedCharacterBreakIterator;
     225
     226NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator(const UChar* buffer, int length)
     227{
     228    m_iterator = nonSharedCharacterBreakIterator;
     229    bool createdIterator = m_iterator && weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), m_iterator, 0);
     230    m_iterator = setUpIterator(createdIterator, m_iterator, UBRK_CHARACTER, buffer, length);
     231}
     232
     233NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator()
     234{
     235    if (!weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), 0, m_iterator))
     236        ubrk_close(m_iterator);
    227237}
    228238
    … …  
    230240{
    231241    // FIXME: This needs closer inspection to achieve behaviour identical to the ICU version.
    232     return characterBreakIterator(string, length);
     242    static bool createdCursorMovementIterator = false;
     243    static TextBreakIterator* staticCursorMovementIterator;
     244    return setUpIterator(createdCursorMovementIterator, staticCursorMovementIterator, UBRK_CHARACTER, string, length);
    233245}
    234246

  • trunk/Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp

    r110965 r118568  
    2525#include <algorithm>
    2626#include <qdebug.h>
     27#include <wtf/Atomics.h>
    2728
    2829// #define DEBUG_TEXT_ITERATORS
    … …  
    3334#endif
    3435
     36using namespace WTF;
    3537using namespace std;
    3638
    … …  
    6769    }
    6870
    69     TextBreakIterator* characterBreakIterator(const UChar* string, int length)
     71    static TextBreakIterator* nonSharedCharacterBreakIterator;
     72
     73    NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator(const UChar* buffer, int length)
    7074    {
    71         static TextBreakIterator staticCharacterBreakIterator;
    72         return setUpIterator(staticCharacterBreakIterator, QTextBoundaryFinder::Grapheme, string, length);
     75        m_iterator = nonSharedCharacterBreakIterator;
     76        bool createdIterator = m_iterator && weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), m_iterator, 0);
     77        if (!createdIterator)
     78            m_iterator = new TextBreakIterator();
     79        setUpIterator(*m_iterator, QTextBoundaryFinder::Grapheme, buffer, length);
     80    }
     81
     82    NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator()
     83    {
     84        if (!weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), 0, m_iterator))
     85            delete m_iterator;
    7386    }
    7487
    7588    TextBreakIterator* cursorMovementIterator(const UChar* string, int length)
    7689    {
    77         return characterBreakIterator(string, length);
     90        static TextBreakIterator staticCursorMovementIterator;
     91        return setUpIterator(staticCursorMovementIterator, QTextBoundaryFinder::Grapheme, string, length);
    7892    }
    7993

  • trunk/Source/WebCore/platform/text/wince/TextBreakIteratorWinCE.cpp

    r110965 r118568  
    2424
    2525#include "PlatformString.h"
     26#include <wtf/Atomics.h>
    2627#include <wtf/StdLibExtras.h>
    2728#include <wtf/unicode/Unicode.h>
    2829
     30using namespace WTF;
     31using namespace WTF::Unicode;
    2932using namespace std;
    30 using namespace WTF::Unicode;
    3133
    3234namespace WebCore {
    … …  
    236238}
    237239
    238 TextBreakIterator* characterBreakIterator(const UChar* string, int length)
    239 {
    240     DEFINE_STATIC_LOCAL(CharBreakIterator, iterator, ());
    241     iterator.reset(string, length);
    242     return &iterator;
     240static CharBreakIterator* nonSharedCharacterBreakIterator;
     241
     242NonSharedCharacterBreakIterator::NonSharedCharacterBreakIterator(const UChar* buffer, int length)
     243{
     244    m_iterator = nonSharedCharacterBreakIterator;
     245    bool createdIterator = m_iterator && weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), m_iterator, 0);
     246    if (!createdIterator)
     247        m_iterator = new CharBreakIterator;
     248    m_iterator.reset(string, length);
     249}
     250
     251NonSharedCharacterBreakIterator::~NonSharedCharacterBreakIterator()
     252{
     253    if (!weakCompareAndSwap(reinterpret_cast<void**>(&nonSharedCharacterBreakIterator), 0, m_iterator))
     254        delete m_iterator;
    243255}
    244256
    … …  
    325337TextBreakIterator* cursorMovementIterator(const UChar* string, int length)
    326338{
    327     return characterBreakIterator(string, length);
     339    DEFINE_STATIC_LOCAL(CharBreakIterator, iterator, ());
     340    iterator.reset(string, length);
     341    return &iterator;
    328342}
    329343
Note: See TracChangeset for help on using the changeset viewer.