Changeset 117658 in webkit


Ignore:
Timestamp:
May 18, 2012 4:47:08 PM (12 years ago)
Author:
eric@webkit.org
Message:
Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate()
m_currentExplicitEmbeddingSequence.isEmpty()) at wikipedia.org

​https://bugs.webkit.org/show_bug.cgi?id=76574

Reviewed by Levi Weintraub.

Source/WebCore:

Consider this example:
<span style="unicode-bidi: embed"><span style="unicode-bidi: isolate">a</span></span>
Before this patch, we would ASSERT when computing the text runs, as we would have encountered
the "embed LTR" directive from the outer span, but not try to "commit" this embedding until
we encountered the first charater (an optimization to avoid creating unnecessary bidi embedding contexts).
The ASSERT we were hitting was to ensure that embedding directives inside an isolated span
did not bleed out and effect the surrounding text.

bidi "isolate" support uses a multi-pass Unicode Bidi Algorithm (UBA), which when encountering
"isolated" sections of text ignores them in the first pass, and then goes back and runs
a separate instance of the UBA on those isolated sections.

Once we scan into an "isolate" section (during an outer UBA application) we should not respect
any embedding directives inside that "isolate" section.

However, in the above example, our "don't commit embeddings until we need them" and
"assert we don't respect embeddings inside isolated spans" were conflicting.
The fix is to make sure we always commit any pending embedding directives *before*
we enter an isolate section.

Luckly there was no functional bug here as we were still respecting
the embedding directives we were belatedly committing. After this change we're commiting
those directives at a more appropriate time, thus avoiding the ASSERT.

Test: fast/text/bidi-isolate-embedding-crash.html

  • platform/text/BidiResolver.h:

(WebCore::::commitExplicitEmbedding):

  • rendering/InlineIterator.h:

(WebCore::notifyObserverEnteredObject):
(WebCore::IsolateTracker::commitExplicitEmbedding):

LayoutTests:

  • fast/text/bidi-isolate-embedding-crash-expected.txt: Added.
  • fast/text/bidi-isolate-embedding-crash.html: Added.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r117657 r117658  
     12012-05-18  Eric Seidel  <eric@webkit.org>
     2
     3        Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty()) at wikipedia.org
     4        https://bugs.webkit.org/show_bug.cgi?id=76574
     5
     6        Reviewed by Levi Weintraub.
     7
     8        * fast/text/bidi-isolate-embedding-crash-expected.txt: Added.
     9        * fast/text/bidi-isolate-embedding-crash.html: Added.
     10
    1112012-05-18  Levi Weintraub  <leviw@chromium.org>
    212

  • trunk/Source/WebCore/ChangeLog

    r117656 r117658  
     12012-05-18  Eric Seidel  <eric@webkit.org>
     2
     3        Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty()) at wikipedia.org
     4        https://bugs.webkit.org/show_bug.cgi?id=76574
     5
     6        Reviewed by Levi Weintraub.
     7
     8        Consider this example:
     9        <span style="unicode-bidi: embed"><span style="unicode-bidi: isolate">a</span></span>
     10        Before this patch, we would ASSERT when computing the text runs, as we would have encountered
     11        the "embed LTR" directive from the outer span, but not try to "commit" this embedding until
     12        we encountered the first charater (an optimization to avoid creating unnecessary bidi embedding contexts).
     13        The ASSERT we were hitting was to ensure that embedding directives inside an isolated span
     14        did not bleed out and effect the surrounding text.
     15
     16        bidi "isolate" support uses a multi-pass Unicode Bidi Algorithm (UBA), which when encountering
     17        "isolated" sections of text ignores them in the first pass, and then goes back and runs
     18        a separate instance of the UBA on those isolated sections.
     19
     20        Once we scan into an "isolate" section (during an outer UBA application) we should not respect
     21        any embedding directives inside that "isolate" section.
     22
     23        However, in the above example, our "don't commit embeddings until we need them" and
     24        "assert we don't respect embeddings inside isolated spans" were conflicting.
     25        The fix is to make sure we always commit any pending embedding directives *before*
     26        we enter an isolate section.
     27
     28        Luckly there was no functional bug here as we were still respecting
     29        the embedding directives we were belatedly committing. After this change we're commiting
     30        those directives at a more appropriate time, thus avoiding the ASSERT.
     31
     32        Test: fast/text/bidi-isolate-embedding-crash.html
     33
     34        * platform/text/BidiResolver.h:
     35        (WebCore::::commitExplicitEmbedding):
     36        * rendering/InlineIterator.h:
     37        (WebCore::notifyObserverEnteredObject):
     38        (WebCore::IsolateTracker::commitExplicitEmbedding):
     39
    1402012-05-18  Pratik Solanki  <psolanki@apple.com>
    241

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

    r116691 r117658  
    403403bool BidiResolver<Iterator, Run>::commitExplicitEmbedding()
    404404{
    405     // This gets called from bidiFirst when setting up our start position.
    406     // FIXME: Re-enable this assert once https://bugs.webkit.org/show_bug.cgi?id=76574 is fixed.
    407     // ASSERT(!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty());
     405    // When we're "inIsolate()" we're resolving the parent context which
     406    // ignores (skips over) the isolated content, including embedding levels.
     407    // We should never accrue embedding levels while skipping over isolated content.
     408    ASSERT(!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty());
    408409
    409410    using namespace WTF::Unicode;

  • trunk/Source/WebCore/rendering/InlineIterator.h

    r107000 r117658  
    133133    }
    134134    if (isIsolated(unicodeBidi)) {
     135        // Make sure that explicit embeddings are committed before we enter the isolated content.
     136        observer->commitExplicitEmbedding();
    135137        observer->enterIsolate();
    136         // Embedding/Override characters implied by dir= are handled when
     138        // Embedding/Override characters implied by dir= will be handled when
    137139        // we process the isolated span, not when laying out the "parent" run.
    138140        return;
    … …  
    452454    // We don't care if we encounter bidi directional overrides.
    453455    void embed(WTF::Unicode::Direction, BidiEmbeddingSource) { }
     456    void commitExplicitEmbedding() { }
    454457
    455458    void addFakeRunIfNecessary(RenderObject* obj, unsigned pos, InlineBidiResolver& resolver)
Note: See TracChangeset for help on using the changeset viewer.